* [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle
@ 2022-10-17 9:53 Chen Zhongjin
2022-11-10 17:49 ` Greg KH
0 siblings, 1 reply; 3+ messages in thread
From: Chen Zhongjin @ 2022-10-17 9:53 UTC (permalink / raw)
To: linux-kernel; +Cc: gregkh, linmq006, chenzhongjin
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) {
+ spin_lock(&entry_list_lock);
+ list_del(&entry->list);
+ spin_unlock(&entry_list_lock);
+ }
+
kfree(entry);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle
2022-10-17 9:53 [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle Chen Zhongjin
@ 2022-11-10 17:49 ` Greg KH
2022-11-11 1:19 ` Chen Zhongjin
0 siblings, 1 reply; 3+ messages in thread
From: Greg KH @ 2022-11-10 17:49 UTC (permalink / raw)
To: Chen Zhongjin; +Cc: linux-kernel, linmq006
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?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle
2022-11-10 17:49 ` Greg KH
@ 2022-11-11 1:19 ` Chen Zhongjin
0 siblings, 0 replies; 3+ messages in thread
From: Chen Zhongjin @ 2022-11-11 1:19 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, linmq006
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-11-11 1:19 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-17 9:53 [PATCH] firmware: dmi-sysfs: Fix null-ptr-deref in dmi_sysfs_register_handle Chen Zhongjin
2022-11-10 17:49 ` Greg KH
2022-11-11 1:19 ` Chen Zhongjin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).