linux-kernel - Re: [PATCH V1] nvmem: core: fix memory abort in cleanup path

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]

Message-ID: <20200103071152.GA1933715@ulmo>
Date:   Fri, 3 Jan 2020 08:11:52 +0100
From:   Thierry Reding <treding@...dia.com>
To:     Bitan Biswas <bbiswas@...dia.com>
CC:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Rob Herring <robh@...nel.org>, <linux-kernel@...r.kernel.org>,
        Jonathan Hunter <jonathanh@...dia.com>
Subject: Re: [PATCH V1] nvmem: core: fix memory abort in cleanup path

On Thu, Jan 02, 2020 at 10:51:24AM -0800, Bitan Biswas wrote:
> 
> Hi Thierry,
> 
> On 1/2/20 4:44 AM, Thierry Reding wrote:
> > On Sat, Dec 28, 2019 at 08:02:42PM -0800, Bitan Biswas wrote:
> > > nvmem_cell_info_to_nvmem_cell implementation has static
> > > allocation of name. nvmem_add_cells_from_of() call may
> > > return error and kfree name results in memory abort. Use
> > > kasprintf() instead of assigning pointer and prevent kfree crash.
> > > 
> > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > > index 9f1ee9c..0fc66e1 100644
> > > --- a/drivers/nvmem/core.c
> > > +++ b/drivers/nvmem/core.c
> > > @@ -110,7 +110,7 @@ static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
> > >   	cell->nvmem = nvmem;
> > >   	cell->offset = info->offset;
> > >   	cell->bytes = info->bytes;
> > > -	cell->name = info->name;
> > > +	cell->name = kasprintf(GFP_KERNEL, "%s", info->name);
> 
> > 
> > kstrdup() seems more appropriate here.
> Thanks. I shall update the patch as suggested.
> 
> > 
> > A slightly more efficient way to do this would be to use a combination
> > of kstrdup_const() and kfree_const(), which would allow read-only
> > strings to be replicated by simple assignment rather than duplication.
> > Note that in that case you'd need to carefully replace all kfree() calls
> > on cell->name by a kfree_const() to ensure they do the right thing.
> kfree(cell->name) is also called for allocations in function
> nvmem_add_cells_from_of() through below call
> kasprintf(GFP_KERNEL, "%pOFn", child);
> 
> My understanding is kfree_const may not work for above allocation.

kfree_const() checks the location that the pointer passed to it points
to. If it points to the kernel's .rodata section, it returns and only
calls kfree() otherwise. Similarily, kstrdup_const() returns its
argument if it points to the .rodata section and duplicates the string
otherwise. On the other hand, pointers returned by kasprintf() will
never point to the .rodata section, so kfree_const() will result in
kfree() getting called.

That said, the savings here are fairly minimal, so I don't feel very
strongly about this. Feel free to go with the kstrdup() variant.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)