linux-kernel - Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 8 Sep 2021 14:32:39 +0300
From:   Dan Carpenter <dan.carpenter@...cle.com>
To:     Marion et Christophe JAILLET <christophe.jaillet@...adoo.fr>
Cc:     minyard@....org, zweiss@...inix.com, andrew@...id.au,
        openipmi-developer@...ts.sourceforge.net,
        linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
Subject: Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling
 path of 'kcs_bmc_serio_add_device()'

On Wed, Sep 08, 2021 at 08:50:14AM +0200, Marion et Christophe JAILLET wrote:
> 
>  
> 
> > Message du 08/09/21 08:28
> > De : "Dan Carpenter" 
> > A : "Christophe JAILLET" 
> > Copie à : minyard@....org, zweiss@...inix.com, andrew@...id.au, openipmi-developer@...ts.sourceforge.net, linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org
> > Objet : Re: [PATCH] ipmi: kcs_bmc: Fix a memory leak in the error handling path of 'kcs_bmc_serio_add_device()'
> > 
> > On Tue, Sep 07, 2021 at 11:06:32PM +0200, Christophe JAILLET wrote:
> > > In the unlikely event where 'devm_kzalloc()' fails and 'kzalloc()'
> > > succeeds, 'port' would be leaking.
> > > 
> > > Test each allocation separately to avoid the leak.
> > > 
> > > Fixes: 3a3d2f6a4c64 ("ipmi: kcs_bmc: Add serio adaptor")
> > > Signed-off-by: Christophe JAILLET 
> > > ---
> > > drivers/char/ipmi/kcs_bmc_serio.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c
> > > index 7948cabde50b..7e2067628a6c 100644
> > > --- a/drivers/char/ipmi/kcs_bmc_serio.c
> > > +++ b/drivers/char/ipmi/kcs_bmc_serio.c
> > > @@ -73,10 +73,12 @@ static int kcs_bmc_serio_add_device(struct kcs_bmc_device *kcs_bmc)
> > > struct serio *port;
> > > 
> > > priv = devm_kzalloc(kcs_bmc->dev, sizeof(*priv), GFP_KERNEL);
> > > + if (!priv)
> > > + return -ENOMEM;
> > > 
> > > /* Use kzalloc() as the allocation is cleaned up with kfree() via serio_unregister_port() */
> > 
> > The serio_unregister_port() calls serio_destroy_port() which calls
> > put_device(&serio->dev). But I wasn't able to track it further than
> > that to the actual kfree().
> 
> Hi Dan,
> 
> Checking this release path was not the goal of this patch.

Yeah.  I was just curious.

> It was only about the VERRYYYY unlikely memory leak.
> 
> However my understanding is:
> kcs_bmc_serio_add_device
> --> serio_register_port
> --> __serio_register_port
> --> serio_init_port
> --> serio->dev.release = serio_release_port
> 
> And in serio_release_port:
> struct serio *serio = to_serio_port(dev);
> kfree(serio);
> 
> For me, this 'serio' looks to the one allocated by 'kcs_bmc_serio_add_device'.
> I think that the comment is correct.

Thanks.  This really helps me actually.  I could just make a list of
the functions which take a container_of(dev) get a struct serio and then
free it.  Then if there is only one function that matches that, I could
assume it's what put_device() will call.

regards,
dan carpenter