[PATCH] class: fix use-after-free in class_register()

[PATCH] class: fix use-after-free in class_register()

[Chunyan Zhang]

From: Jing Xia <jing.xia@unisoc.com>

The lock_class_key is still registered and can be found in
lock_keys_hash hlist after subsys_private is freed in error
handler path.A task who iterate over the lock_keys_hash
later may cause use-after-free.So fix that up and unregister
the lock_class_key before kfree(cp).

Signed-off-by: Jing Xia <jing.xia@unisoc.com>
Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
---
 drivers/base/class.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/base/class.c b/drivers/base/class.c
index 7e78aee0fd6c..7b38fdf8e1d7 100644
--- a/drivers/base/class.c
+++ b/drivers/base/class.c
@@ -213,6 +213,7 @@ int class_register(const struct class *cls)
 	return 0;
 
 err_out:
+	lockdep_unregister_key(key);
 	kfree(cp);
 	return error;
 }
-- 
2.25.1

Re: [PATCH] class: fix use-after-free in class_register()

[Greg Kroah-Hartman]

On Mon, Dec 18, 2023 at 10:44:03AM +0800, Chunyan Zhang wrote:
> From: Jing Xia <jing.xia@unisoc.com>
> 
> The lock_class_key is still registered and can be found in
> lock_keys_hash hlist after subsys_private is freed in error
> handler path.A task who iterate over the lock_keys_hash
> later may cause use-after-free.So fix that up and unregister
> the lock_class_key before kfree(cp).

What task iterates over all hashes?

And can you put ' ' after your '.'?

And how was this found?

> 
> Signed-off-by: Jing Xia <jing.xia@unisoc.com>
> Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>

What commit id does this fix?

Also note in the changelog that this only can happen if lockdep is
enabled, which is not true for normal systems.

thanks,

greg k-h

Re: [PATCH] class: fix use-after-free in class_register()

[Greg Kroah-Hartman]

On Mon, Dec 18, 2023 at 07:52:18AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 18, 2023 at 10:44:03AM +0800, Chunyan Zhang wrote:
> > From: Jing Xia <jing.xia@unisoc.com>
> > 
> > The lock_class_key is still registered and can be found in
> > lock_keys_hash hlist after subsys_private is freed in error
> > handler path.A task who iterate over the lock_keys_hash
> > later may cause use-after-free.So fix that up and unregister
> > the lock_class_key before kfree(cp).
> 
> What task iterates over all hashes?
> 
> And can you put ' ' after your '.'?
> 
> And how was this found?

And more importantly, how was this tested?

thanks,

greg k-h

Re: [PATCH] class: fix use-after-free in class_register()

[jing xia]

On Mon, Dec 18, 2023 at 2:52 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Mon, Dec 18, 2023 at 10:44:03AM +0800, Chunyan Zhang wrote:
> > From: Jing Xia <jing.xia@unisoc.com>
> >
> > The lock_class_key is still registered and can be found in
> > lock_keys_hash hlist after subsys_private is freed in error
> > handler path.A task who iterate over the lock_keys_hash
> > later may cause use-after-free.So fix that up and unregister
> > the lock_class_key before kfree(cp).
>
> What task iterates over all hashes?
>
> And can you put ' ' after your '.'?
>
> And how was this found?
>
Thanks for your comments. I'll add more information in the changelog.

On our platform, a driver fails to kset_register because of creating
duplicate filename '/class/xxx'.And we got serval kernel panic issues
about alignment fault on stability tests.All backtraces show that it is
manipulating the corrupted lock_keys_hash hlish when a workqueue is
created or released.

Here is one backtrace:

Unable to handle kernel paging request at virtual address ffffffc081480bae
 Mem abort info:
   ESR = 0x0000000096000021
   EC = 0x25: DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
   FSC = 0x21: alignment fault
...
 Call trace:
  lockdep_register_key+0x128/0x22c
  alloc_workqueue+0x190/0x640
  loop_configure+0x2f0/0x560
  lo_ioctl+0x70c/0xf60
  blkdev_ioctl+0x290/0x88c
  __arm64_sys_ioctl+0xa8/0xe4

And we are aware of the fact that it might be a use-after-free issue.
So we enable Kasan and it gives a invalid-access bug report.

BUG: KASAN: invalid-access in lockdep_register_key+0x19c/0x1bc
Write of size 8 at addr 15ffff808b8c0368 by task modprobe/252
Pointer tag: [15], memory tag: [fe]

CPU: 7 PID: 252 Comm: modprobe Tainted: G        W
6.6.0-mainline-maybe-dirty #1

Call trace:
dump_backtrace+0x1b0/0x1e4
show_stack+0x2c/0x40
dump_stack_lvl+0xac/0xe0
print_report+0x18c/0x4d8
kasan_report+0xe8/0x148
__hwasan_store8_noabort+0x88/0x98
lockdep_register_key+0x19c/0x1bc
class_register+0x94/0x1ec
init_module+0xbc/0xf48 [rfkill]
do_one_initcall+0x17c/0x72c
do_init_module+0x19c/0x3f8
load_module+0x2300/0x2394
__arm64_sys_finit_module+0x350/0x4d8
invoke_syscall+0x88/0x1f0
el0_svc_common+0xe8/0x1c0
do_el0_svc+0x34/0x44
el0_svc+0x50/0xb8
el0t_64_sync_handler+0x68/0xbc
el0t_64_sync+0x19c/0x1a0

The buggy address belongs to the object at ffffff808b8c0000
which belongs to the cache kmalloc-1k of size 1024
The buggy address is located 232 bytes to the right of
640-byte region [ffffff808b8c0000, ffffff808b8c0280)

The buggy address belongs to the physical page:
page:00000000918c4834 refcount:1 mapcount:0 mapping:0000000000000000
index:0x0 pfn:0x10b8c0
head:00000000918c4834 order:3 entire_mapcount:0 nr_pages_mapped:0 pincount:0
anon flags: 0x4000000000000840(slab|head|zone=1|kasantag=0x0)
page_type: 0xffffffff()
raw: 4000000000000840 80ffff8080002a00 0000000000000000 0000000000000001
raw: 0000000000000000 0000000000200020 00000001ffffffff 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffffff808b8c0100: 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a 8a
ffffff808b8c0200: 8a 8a 8a 8a 8a 8a 8a 8a fe fe fe fe fe fe fe fe
>ffffff808b8c0300: fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe fe
                                     ^

> >
> > Signed-off-by: Jing Xia <jing.xia@unisoc.com>
> > Signed-off-by: Xuewen Yan <xuewen.yan@unisoc.com>
>
> What commit id does this fix?
>
> Also note in the changelog that this only can happen if lockdep is
> enabled, which is not true for normal systems.
>
I'll fix it on v1.Thanks.

> thanks,
>
> greg k-h