Messages in this thread | | | Subject | Re: [PATCH V1] nvmem: core: fix memory abort in cleanup path | From | Bitan Biswas <> | Date | Fri, 3 Jan 2020 04:50:10 -0800 |
| |
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
| |