linux-kernel - Re: [PATCH] nvdimm: Fix devs leaks in scan

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: <916631e6-6acf-4aa5-a406-4655a6517f78@fujitsu.com>
Date: Fri, 7 Jun 2024 02:22:29 +0000
From: "Zhijian Li (Fujitsu)" <lizhijian@...itsu.com>
To: Dave Jiang <dave.jiang@...el.com>, "nvdimm@...ts.linux.dev"
	<nvdimm@...ts.linux.dev>
CC: "dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"vishal.l.verma@...el.com" <vishal.l.verma@...el.com>, "ira.weiny@...el.com"
	<ira.weiny@...el.com>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] nvdimm: Fix devs leaks in scan_labels()



On 07/06/2024 00:27, Dave Jiang wrote:
> 
> 
> On 6/3/24 8:16 PM, Li Zhijian wrote:
>> Don't allocate devs again when it's valid pointer which has pionted to
>> the memory allocated above with size (count + 2 * sizeof(dev)).
>>
>> A kmemleak reports:
>> unreferenced object 0xffff88800dda1980 (size 16):
>>    comm "kworker/u10:5", pid 69, jiffies 4294671781
>>    hex dump (first 16 bytes):
>>      00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>>    backtrace (crc 0):
>>      [<00000000c5dea560>] __kmalloc+0x32c/0x470
>>      [<000000009ed43c83>] nd_region_register_namespaces+0x6fb/0x1120 [libnvdimm]
>>      [<000000000e07a65c>] nd_region_probe+0xfe/0x210 [libnvdimm]
>>      [<000000007b79ce5f>] nvdimm_bus_probe+0x7a/0x1e0 [libnvdimm]
>>      [<00000000a5f3da2e>] really_probe+0xc6/0x390
>>      [<00000000129e2a69>] __driver_probe_device+0x78/0x150
>>      [<000000002dfed28b>] driver_probe_device+0x1e/0x90
>>      [<00000000e7048de2>] __device_attach_driver+0x85/0x110
>>      [<0000000032dca295>] bus_for_each_drv+0x85/0xe0
>>      [<00000000391c5a7d>] __device_attach+0xbe/0x1e0
>>      [<0000000026dabec0>] bus_probe_device+0x94/0xb0
>>      [<00000000c590d936>] device_add+0x656/0x870
>>      [<000000003d69bfaa>] nd_async_device_register+0xe/0x50 [libnvdimm]
>>      [<000000003f4c52a4>] async_run_entry_fn+0x2e/0x110
>>      [<00000000e201f4b0>] process_one_work+0x1ee/0x600
>>      [<000000006d90d5a9>] worker_thread+0x183/0x350
>>
>> Fixes: 1b40e09a1232 ("libnvdimm: blk labels and namespace instantiation")
>> Signed-off-by: Li Zhijian <lizhijian@...itsu.com>
>> ---
>>   drivers/nvdimm/namespace_devs.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvdimm/namespace_devs.c b/drivers/nvdimm/namespace_devs.c
>> index d6d558f94d6b..56b016dbe307 100644
>> --- a/drivers/nvdimm/namespace_devs.c
>> +++ b/drivers/nvdimm/namespace_devs.c
>> @@ -1994,7 +1994,9 @@ static struct device **scan_labels(struct nd_region *nd_region)
>>   		/* Publish a zero-sized namespace for userspace to configure. */
>>   		nd_mapping_free_labels(nd_mapping);
>>   
>> -		devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
>> +		/* devs probably has been allocated */
>> +		if (!devs)
>> +			devs = kcalloc(2, sizeof(dev), GFP_KERNEL);
> 
> This changes the behavior of this code and possibly corrupting the previously allocated memory at times when 'devs' is valid.

If we reach here, that means count == 0 is true. so we can deduce that
the previously allocated memory was not touched at all in previous loop.
and its size was (2 * sizeof(dev)) too.

Was the 'devs' leaked out from the previous loop and should be freed instead?

this option is also fine to me. we can also fix this by free(devs) before allocate it again.:

+             kfree(devs); // kfree(NULL) is safe.
               devs = kcalloc(2, sizeof(dev), GFP_KERNEL);


Thanks
Zhijian

> 
>>   		if (!devs)
>>   			goto err;
>>   
>