Messages in this thread | | | Date | Mon, 25 Apr 2022 09:09:35 +0200 | Subject | Re: [PATCH] tty: vt: consolemap: Add missing kfree() in con_do_clear_unimap() | From | Jiri Slaby <> |
| |
On 25. 04. 22, 8:59, Jiri Slaby wrote: > Hi, > > On 09. 03. 22, 13:34, 聂江磊 wrote: >> I found this bug by using clang static analyse checkers. I found that >> function con_release_unimap() is only called in this >> file(drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c). There >> are totally 5 times that con_release_unimap() is called >> (line 430, 466, 522, 599, 673) while con_release_unimap() is not >> followed by kfree() only in line 522. So I think it is a bug >> and make this patch. >> >> >> At 2022-03-03 10:06:30, "Jianglei Nie" <niejianglei2021@163.com> wrote: >>> We should free p after con_release_unimap(p) like the call points of >>> con_release_unimap() do in the same file. > > But this one does not free it on purpose, right? See below. > >>> This patch adds the missing kfree() after con_release_unimap(p). >>> >>> Signed-off-by: Jianglei Nie <niejianglei2021@163.com> >>> --- >>> drivers/tty/vt/consolemap.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/tty/vt/consolemap.c b/drivers/tty/vt/consolemap.c >>> index d815ac98b39e..5279c3d27720 100644 >>> --- a/drivers/tty/vt/consolemap.c >>> +++ b/drivers/tty/vt/consolemap.c >>> @@ -520,6 +520,7 @@ static int con_do_clear_unimap(struct vc_data *vc) >>> p->refcount++; >>> p->sum = 0; >>> con_release_unimap(p); >>> + kfree(p); > > You've just broken con_set_unimap(), or do I miss something?
No, you did not. The interface is terrible and deserves cleanup.
I found this, likely related, syzkaller report in my INBOX: https://lore.kernel.org/all/000000000000ee58d305bbe9197a@google.com/
Care to test the reproducer both with and without your change? Does your patch fixes the issue. And if it does, could you add this to your patch: Reported-by: syzbot+bcc922b19ccc64240b42@syzkaller.appspotmail.com ? So that syzbot verifies the patch.
Once you do all this, I will re-review the patch and the code. The code is really very hard to follow, so I cannot decide whether your patch is correct or not ATM.
And provided the above, I put a note to my TODO list to restructure the code, so that people know what's going on there.
thanks, -- js
| |