Re: [PATCH] tty: vt: consolemap: Add missing kfree() in con_do_clear_unimap()

[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@xxxxxxx> 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@xxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx/

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@xxxxxxxxxxxxxxxxxxxxxxxxx
? 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