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

[Bitan Biswas]

Hi Thierry,

On 1/2/20 11:11 PM, Thierry Reding wrote:
> 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.
Thanks for the explanation. I would test the implementation with the 
_const functions you suggested and send updated patch.

-regards,
 Bitan