Messages in this thread | | | Date | Fri, 11 Nov 2022 09:19:00 +0800 | Subject | Re: [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle | From | Chen Zhongjin <> |
| |
Hi,
On 2022/11/11 1:49, Greg KH wrote: > On Mon, Oct 17, 2022 at 05:53:42PM +0800, Chen Zhongjin wrote: >> KASAN reported a null-ptr-deref error: >> >> KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f] >> CPU: 0 PID: 1373 Comm: modprobe >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996) >> RIP: 0010:dmi_sysfs_entry_release >> ... >> Call Trace: >> <TASK> >> kobject_put >> dmi_sysfs_register_handle (drivers/firmware/dmi-sysfs.c:540) dmi_sysfs >> dmi_decode_table (drivers/firmware/dmi_scan.c:133) >> dmi_walk (drivers/firmware/dmi_scan.c:1115) >> dmi_sysfs_init (drivers/firmware/dmi-sysfs.c:149) dmi_sysfs >> do_one_initcall (init/main.c:1296) >> ... >> Kernel panic - not syncing: Fatal exception >> Kernel Offset: 0x4000000 from 0xffffffff81000000 >> ---[ end Kernel panic - not syncing: Fatal exception ]--- >> >> It is because previous patch added kobject_put() to release the memory >> which will call dmi_sysfs_entry_release() and list_del(). >> >> However, list_add_tail(entry->list) is called after the error block, >> so the list_head is uninitialized and cannot be deleted. >> >> Because entry is allocated by kzalloc() so the list.prev is NULL in >> the error path. Check it in dmi_sysfs_entry_release() to avoid >> deleting uninitialized list_head. >> >> Fixes: 660ba678f999 ("firmware: dmi-sysfs: Fix memory leak in dmi_sysfs_register_handle") >> >> Signed-off-by: Chen Zhongjin <chenzhongjin@huawei.com> >> --- >> drivers/firmware/dmi-sysfs.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/firmware/dmi-sysfs.c b/drivers/firmware/dmi-sysfs.c >> index 66727ad3361b..f8815eeed00c 100644 >> --- a/drivers/firmware/dmi-sysfs.c >> +++ b/drivers/firmware/dmi-sysfs.c >> @@ -557,9 +557,12 @@ static void dmi_sysfs_entry_release(struct kobject *kobj) >> { >> struct dmi_sysfs_entry *entry = to_entry(kobj); >> >> - spin_lock(&entry_list_lock); >> - list_del(&entry->list); >> - spin_unlock(&entry_list_lock); >> + if (entry->list.prev != NULL) { > You should not be poking around in a lock structure like this at all. > Also the lock isn't held, so how do you know this is going to work? > > I suggest fixing up the original patch, perhaps reverting that instead?
It makes sense, but in fact the original patch did fix the memory leak.
I'm going to move kobject_put behind list_add_tail to keep consistency, because only after that the entry initialization is complete.
Will send v2, thanks for your review!
Best,
Chen
> thanks, > > greg k-h
| |