[PATCH] locking/lockdep: fix null-ptr-deref in check_prev_add()

[PATCH] locking/lockdep: fix null-ptr-deref in check_prev_add()

[Yang Yingliang]

I got a null-ptr-deref report as following:

general protection fault, probably for non-canonical address 0xdffffc0000000008: 0000 [#1] PREEMPT SMP KASAN PTI
KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
CPU: 0 PID: 500 Comm: systemd-udevd Tainted: G        W          6.1.0-rc5-00144-gabd8ea84ca72-dirty #1320
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
RIP: 0010:check_prevs_add+0x2f8/0x2780
Call Trace:
 <TASK>
 __lock_acquire+0x2ae8/0x3d60
 lock_acquire+0x195/0x4e0
 fs_reclaim_acquire+0x119/0x160
 kmem_cache_alloc_node+0x47/0x310
 __alloc_skb+0x205/0x2d0
 devlink_compat_running_version+0x10b/0x6a0
 dev_ethtool+0x285/0x380
 dev_ioctl+0x16c/0xff0
 sock_do_ioctl+0x1ae/0x220
 sock_ioctl+0x55f/0x600
 __x64_sys_ioctl+0x156/0x1d0
 do_syscall_64+0x37/0x90
 entry_SYSCALL_64_after_hwframe+0x63/0xcd

If in use bit of lock_class is not set, hlock_class() returns NULL,
it causes null-ptr-deref while using it in check_prev_add(). Fix this
by adding null pointer check for it.

Fixes: a0b0fd53e1e6 ("locking/lockdep: Free lock classes that are no longer in use")
Reported-by: Zhengchao Shao <shaozhengchao@huawei.com>
Signed-off-by: Yang Yingliang <yangyingliang@huawei.com>
---
 kernel/locking/lockdep.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
index e3375bc40dad..f0a28f9ed426 100644
--- a/kernel/locking/lockdep.c
+++ b/kernel/locking/lockdep.c
@@ -3063,10 +3063,13 @@ check_prev_add(struct task_struct *curr, struct held_lock *prev,
 	       struct held_lock *next, u16 distance,
 	       struct lock_trace **const trace)
 {
+	struct lock_class *prev_class = hlock_class(prev);
+	struct lock_class *next_class = hlock_class(next);
 	struct lock_list *entry;
 	enum bfs_result ret;
 
-	if (!hlock_class(prev)->key || !hlock_class(next)->key) {
+	if ((prev_class && !prev_class->key) ||
+	    (next_class && !next_class->key)) {
 		/*
 		 * The warning statements below may trigger a use-after-free
 		 * of the class name. It is better to trigger a use-after free
-- 
2.25.1

Re: [PATCH] locking/lockdep: fix null-ptr-deref in check_prev_add()

[Peter Zijlstra]

On Fri, Nov 18, 2022 at 04:31:02PM +0800, Yang Yingliang wrote:
> I got a null-ptr-deref report as following:
> 
> general protection fault, probably for non-canonical address 0xdffffc0000000008: 0000 [#1] PREEMPT SMP KASAN PTI
> KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
> CPU: 0 PID: 500 Comm: systemd-udevd Tainted: G        W          6.1.0-rc5-00144-gabd8ea84ca72-dirty #1320
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> RIP: 0010:check_prevs_add+0x2f8/0x2780
> Call Trace:
>  <TASK>
>  __lock_acquire+0x2ae8/0x3d60
>  lock_acquire+0x195/0x4e0
>  fs_reclaim_acquire+0x119/0x160
>  kmem_cache_alloc_node+0x47/0x310
>  __alloc_skb+0x205/0x2d0
>  devlink_compat_running_version+0x10b/0x6a0
>  dev_ethtool+0x285/0x380
>  dev_ioctl+0x16c/0xff0
>  sock_do_ioctl+0x1ae/0x220
>  sock_ioctl+0x55f/0x600
>  __x64_sys_ioctl+0x156/0x1d0
>  do_syscall_64+0x37/0x90
>  entry_SYSCALL_64_after_hwframe+0x63/0xcd
> 
> If in use bit of lock_class is not set, hlock_class() returns NULL,

Why is that a valid premise?

That is; you think it is OK for a held_lock to not have an in-use
lock_class?

I'm thinking there's more to this problem you found.

Re: [PATCH] locking/lockdep: fix null-ptr-deref in check_prev_add()

[Yang Yingliang]

On 2022/11/18 18:17, Peter Zijlstra wrote:
> On Fri, Nov 18, 2022 at 04:31:02PM +0800, Yang Yingliang wrote:
>> I got a null-ptr-deref report as following:
>>
>> general protection fault, probably for non-canonical address 0xdffffc0000000008: 0000 [#1] PREEMPT SMP KASAN PTI
>> KASAN: null-ptr-deref in range [0x0000000000000040-0x0000000000000047]
>> CPU: 0 PID: 500 Comm: systemd-udevd Tainted: G        W          6.1.0-rc5-00144-gabd8ea84ca72-dirty #1320
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
>> RIP: 0010:check_prevs_add+0x2f8/0x2780
>> Call Trace:
>>   <TASK>
>>   __lock_acquire+0x2ae8/0x3d60
>>   lock_acquire+0x195/0x4e0
>>   fs_reclaim_acquire+0x119/0x160
>>   kmem_cache_alloc_node+0x47/0x310
>>   __alloc_skb+0x205/0x2d0
>>   devlink_compat_running_version+0x10b/0x6a0
>>   dev_ethtool+0x285/0x380
>>   dev_ioctl+0x16c/0xff0
>>   sock_do_ioctl+0x1ae/0x220
>>   sock_ioctl+0x55f/0x600
>>   __x64_sys_ioctl+0x156/0x1d0
>>   do_syscall_64+0x37/0x90
>>   entry_SYSCALL_64_after_hwframe+0x63/0xcd
>>
>> If in use bit of lock_class is not set, hlock_class() returns NULL,
> Why is that a valid premise?
>
> That is; you think it is OK for a held_lock to not have an in-use
> lock_class?
It's not OK, before this crash, there is a WARN_ON in hlock_class().
But I think it shouldn't make system crash, so I add this check.
>
> I'm thinking there's more to this problem you found.
I will debug more to find out why in-use bit is not set.

Thanks,
Yang
>
>
> .