linux-kernel - Re: [PATCH v2] nvmem: core: fix possibly memleak when use nvmem_cell_info_to_nvmem

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, 23 Sep 2020 19:02:26 +0300
From:   Vadym Kochan <vadym.kochan@...ision.eu>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] nvmem: core: fix possibly memleak when use
 nvmem_cell_info_to_nvmem_cell()

On Wed, Sep 23, 2020 at 04:51:06PM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 23/09/2020 15:51, Vadym Kochan wrote:
> > > -       return nvmem_cell_write(&cell, buf, cell.bytes);
> > > +       rc = nvmem_cell_write(&cell, buf, cell.bytes);
> > > +       if (rc)
> > > +               kfree_const(cell->name);
> > > +
> > > +       return rc;
> > >   }
> > >   EXPORT_SYMBOL_GPL(nvmem_device_cell_write);
> > >   ------------------------>cut<---------------------------
> > > 
> > > --srini
> > > 
> > But is it really needed to kstrdup(cell->name) for nvmem_device_cell_{read,write} ?
> This boils down to if we want to use same api to parse nvmem_cell_info or
> not!
> 
> If we want to keep this simple, we can either explicitly add free for
> successful caller to nvmem_cell_info_to_nvmem_cell()!
> 

I think that such additional kfree_const(cell->name) handling adds more
complexity for error handling, also to my understanding usually
resource allocation should be done in the called func in case of error
was returned.

> Or
> 
> use something like what you did, but new api needs more clarity!
> May be renaming __nvmem_cell_info_to_nvmem_cell to
> nvmem_cell_info_to_nvmem_cell_no_alloc would clarify that a bit!
> 

Yes, I agree that naming should be better, actually "__" already points
to it's unsafety (no kstrdup() is used), but of course additional suffix
would be better.

> Also can you make sure that linewrapping on function names be inline with
> existing code.

You mean do not do such func attributes breaking as I did (moved them
line upper) ?

> 
> Please send v3 with that changes!
> 
> 
> --srini
> > It is used only for log error in case the unaligned access did not
> > pass the check