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

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

[Bitan Biswas]

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.

[    8.076461] Unable to handle kernel paging request at virtual address ffffffffffe44888
[    8.084762] Mem abort info:
[    8.087694]   ESR = 0x96000006
[    8.090906]   EC = 0x25: DABT (current EL), IL = 32 bits
[    8.096476]   SET = 0, FnV = 0
[    8.099683]   EA = 0, S1PTW = 0
[    8.102976] Data abort info:
[    8.106004]   ISV = 0, ISS = 0x00000006
[    8.110026]   CM = 0, WnR = 0
[    8.113154] swapper pgtable: 64k pages, 48-bit VAs, pgdp=00000000815d0000
[    8.120279] [ffffffffffe44888] pgd=0000000081d30803, pud=0000000081d30803, pmd=0000000000000000
[    8.129429] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[    8.135257] Modules linked in:
[    8.138456] CPU: 2 PID: 43 Comm: kworker/2:1 Tainted: G S                5.5.0-rc3-tegra-00051-g6989dd3-dirty #3   [    8.149098] Hardware name: quill (DT)
[    8.152968] Workqueue: events deferred_probe_work_func
[    8.158350] pstate: a0000005 (NzCv daif -PAN -UAO)
[    8.163386] pc : kfree+0x38/0x278
[    8.166873] lr : nvmem_cell_drop+0x68/0x80
[    8.171154] sp : ffff80001284f9d0
[    8.174620] x29: ffff80001284f9d0 x28: ffff0001f677e830
[    8.180189] x27: ffff800011b0b000 x26: ffff0001c36e1008
[    8.185755] x25: ffff8000112ad000 x24: ffff8000112c9000
[    8.191311] x23: ffffffffffffffea x22: ffff800010adc7f0
[    8.196865] x21: ffffffffffe44880 x20: ffff800011b0b068
[    8.202424] x19: ffff80001122d380 x18: ffffffffffffffff
[    8.207987] x17: 00000000d5cb4756 x16: 0000000070b193b8
[    8.213550] x15: ffff8000119538c8 x14: 0720072007200720
[    8.219120] x13: 07200720076e0772 x12: 07750762072d0765
[    8.224685] x11: 0773077507660765 x10: 072f073007300730
[    8.230253] x9 : 0730073207380733 x8 : 0000000000000151
[    8.235818] x7 : 07660765072f0720 x6 : ffff0001c00e0f00
[    8.241382] x5 : 0000000000000000 x4 : ffff0001c0b43800
[    8.247007] x3 : ffff800011b0b068 x2 : 0000000000000000
[    8.252567] x1 : 0000000000000000 x0 : ffffffdfffe00000
[    8.258126] Call trace:
[    8.260705]  kfree+0x38/0x278
[    8.263827]  nvmem_cell_drop+0x68/0x80
[    8.267773]  nvmem_device_remove_all_cells+0x2c/0x50
[    8.272988]  nvmem_register.part.9+0x520/0x628
[    8.277655]  devm_nvmem_register+0x48/0xa0
[    8.281966]  tegra_fuse_probe+0x140/0x1f0
[    8.286181]  platform_drv_probe+0x50/0xa0
[    8.290397]  really_probe+0x108/0x348
[    8.294243]  driver_probe_device+0x58/0x100
[    8.298618]  __device_attach_driver+0x90/0xb0
[    8.303172]  bus_for_each_drv+0x64/0xc8
[    8.307184]  __device_attach+0xd8/0x138
[    8.311195]  device_initial_probe+0x10/0x18
[    8.315562]  bus_probe_device+0x90/0x98
[    8.319572]  deferred_probe_work_func+0x74/0xb0
[    8.324304]  process_one_work+0x1e0/0x358
[    8.328490]  worker_thread+0x208/0x488
[    8.332411]  kthread+0x118/0x120
[    8.335783]  ret_from_fork+0x10/0x18
[    8.339561] Code: d350feb5 f2dffbe0 aa1e03f6 8b151815 (f94006a0)
[    8.345939] ---[ end trace 49b1303c6b83198e ]---

Fixes: badcdff107cbf ("nvmem: Convert to using %pOFn instead of device_node.name")

Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
---
 drivers/nvmem/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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);
 
 	cell->bit_offset = info->bit_offset;
 	cell->nbits = info->nbits;
-- 
2.7.4

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 4487 bytes --]

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.
> 
> [    8.076461] Unable to handle kernel paging request at virtual address ffffffffffe44888
> [    8.084762] Mem abort info:
> [    8.087694]   ESR = 0x96000006
> [    8.090906]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    8.096476]   SET = 0, FnV = 0
> [    8.099683]   EA = 0, S1PTW = 0
> [    8.102976] Data abort info:
> [    8.106004]   ISV = 0, ISS = 0x00000006
> [    8.110026]   CM = 0, WnR = 0
> [    8.113154] swapper pgtable: 64k pages, 48-bit VAs, pgdp=00000000815d0000
> [    8.120279] [ffffffffffe44888] pgd=0000000081d30803, pud=0000000081d30803, pmd=0000000000000000
> [    8.129429] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [    8.135257] Modules linked in:
> [    8.138456] CPU: 2 PID: 43 Comm: kworker/2:1 Tainted: G S                5.5.0-rc3-tegra-00051-g6989dd3-dirty #3   [    8.149098] Hardware name: quill (DT)
> [    8.152968] Workqueue: events deferred_probe_work_func
> [    8.158350] pstate: a0000005 (NzCv daif -PAN -UAO)
> [    8.163386] pc : kfree+0x38/0x278
> [    8.166873] lr : nvmem_cell_drop+0x68/0x80
> [    8.171154] sp : ffff80001284f9d0
> [    8.174620] x29: ffff80001284f9d0 x28: ffff0001f677e830
> [    8.180189] x27: ffff800011b0b000 x26: ffff0001c36e1008
> [    8.185755] x25: ffff8000112ad000 x24: ffff8000112c9000
> [    8.191311] x23: ffffffffffffffea x22: ffff800010adc7f0
> [    8.196865] x21: ffffffffffe44880 x20: ffff800011b0b068
> [    8.202424] x19: ffff80001122d380 x18: ffffffffffffffff
> [    8.207987] x17: 00000000d5cb4756 x16: 0000000070b193b8
> [    8.213550] x15: ffff8000119538c8 x14: 0720072007200720
> [    8.219120] x13: 07200720076e0772 x12: 07750762072d0765
> [    8.224685] x11: 0773077507660765 x10: 072f073007300730
> [    8.230253] x9 : 0730073207380733 x8 : 0000000000000151
> [    8.235818] x7 : 07660765072f0720 x6 : ffff0001c00e0f00
> [    8.241382] x5 : 0000000000000000 x4 : ffff0001c0b43800
> [    8.247007] x3 : ffff800011b0b068 x2 : 0000000000000000
> [    8.252567] x1 : 0000000000000000 x0 : ffffffdfffe00000
> [    8.258126] Call trace:
> [    8.260705]  kfree+0x38/0x278
> [    8.263827]  nvmem_cell_drop+0x68/0x80
> [    8.267773]  nvmem_device_remove_all_cells+0x2c/0x50
> [    8.272988]  nvmem_register.part.9+0x520/0x628
> [    8.277655]  devm_nvmem_register+0x48/0xa0
> [    8.281966]  tegra_fuse_probe+0x140/0x1f0
> [    8.286181]  platform_drv_probe+0x50/0xa0
> [    8.290397]  really_probe+0x108/0x348
> [    8.294243]  driver_probe_device+0x58/0x100
> [    8.298618]  __device_attach_driver+0x90/0xb0
> [    8.303172]  bus_for_each_drv+0x64/0xc8
> [    8.307184]  __device_attach+0xd8/0x138
> [    8.311195]  device_initial_probe+0x10/0x18
> [    8.315562]  bus_probe_device+0x90/0x98
> [    8.319572]  deferred_probe_work_func+0x74/0xb0
> [    8.324304]  process_one_work+0x1e0/0x358
> [    8.328490]  worker_thread+0x208/0x488
> [    8.332411]  kthread+0x118/0x120
> [    8.335783]  ret_from_fork+0x10/0x18
> [    8.339561] Code: d350feb5 f2dffbe0 aa1e03f6 8b151815 (f94006a0)
> [    8.345939] ---[ end trace 49b1303c6b83198e ]---
> 
> Fixes: badcdff107cbf ("nvmem: Convert to using %pOFn instead of device_node.name")
> 
> Signed-off-by: Bitan Biswas <bbiswas@nvidia.com>
> ---
>  drivers/nvmem/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> 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.

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.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

[Bitan Biswas]

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.



-regards,
  Bitan

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2247 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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