Reply: A null-ptr crash in linux-6.4 usb driver

Reply: A null-ptr crash in linux-6.4 usb driver

[柳菁峰]

I have made a patch that simply checks for null pointer, but I am not sure if this will affect certain functions or logic. I hope you can check it carefully.

1006c1006
<       if (dev->p->dead) {
---
>       if (!dev->p||dev->p->dead) {



>Try it and see!  You have the reproducer, so you are in the best position to work on this.

Re: Reply: A null-ptr crash in linux-6.4 usb driver

[gregkh]

On Wed, Nov 08, 2023 at 07:40:08AM +0000, 柳菁峰 wrote:
> I have made a patch that simply checks for null pointer, but I am not sure if this will affect certain functions or logic. I hope you can check it carefully.
> 
> 1006c1006
> <       if (dev->p->dead) {
> ---
> >       if (!dev->p||dev->p->dead) {

Can you take a look at the file,
Documentation/process/submitting-patches.rst for how to properly
generate a patch?  This does not give us any context to even know what
file you are changing.

Also, we have no context at all to even know what this is about at all,
what is this for?

confused,

greg k-h

Reply: A null-ptr crash in linux-6.4 usb driver

[柳菁峰]

A null-ptr crash was found in usb driver and it crashed with fault-inject. I have made a patch that simply checks for this null pointer, but I am not sure if this will affect certain functions or logic. I hope you can check it carefully please and the format meets the requirements this time.



The crash info:
general protection fault, probably for non-canonical address 0xdffffc0000000021: 0000 [#1] PREEMPT SMP KASAN NOPTI
KASAN: null-ptr-deref in range [0x0000000000000108-0x000000000000010f]
CPU: 0 PID: 4280 Comm: syz-executor.3 Not tainted 6.4.0 #8
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
RIP: 0010:__device_attach+0xa9/0x450
Code: e8 03 42 80 3c 20 00 0f 85 3f 03 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 65 48 49 8d bc 24 08 01 00 00 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 06 0f 8e f8 02 00 00 41 f6 84 24 08 01 00 00
RSP: 0018:ffff888116927b98 EFLAGS: 00010206
RAX: dffffc0000000000 RBX: 1ffff11022d24f73 RCX: 0000000000000000
RDX: 0000000000000021 RSI: ffffffff847c3300 RDI: 0000000000000108
RBP: ffff88811959b078 R08: 0000000000000000 R09: ffffffff860a0097
R10: ffff888116927b98 R11: 0000000000000001 R12: 0000000000000000
R13: ffff88811959b0f8 R14: 00000000fffffff0 R15: 0000000000000000
FS:  00007f00232a3700(0000) GS:ffff88811ae00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fff79859fb0 CR3: 000000010a18a005 CR4: 0000000000770ef0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
PKRU: 55555554
Call Trace:
  <TASK>
  ? die_addr+0x38/0xa0
  ? exc_general_protection+0x144/0x220
  ? asm_exc_general_protection+0x22/0x30
  ? __device_attach+0xa9/0x450
  ? __device_attach+0x76/0x450
  ? __pfx___device_attach+0x10/0x10
  ? usb_ifnum_to_if+0x140/0x1d0
  proc_ioctl.part.0+0x3ff/0x4a0
  usbdev_ioctl+0x178a/0x3f70
  ? __pfx_usbdev_ioctl+0x10/0x10
  ? __pfx___lock_acquire+0x10/0x10
  ? do_vfs_ioctl+0x120/0x1480
  ? __pfx_do_vfs_ioctl+0x10/0x10
  ? find_held_lock+0x2c/0x110
  ? __fget_files+0x1f8/0x420
  ? lock_release+0x3c3/0x6a0
  ? __pfx_lock_release+0x10/0x10
  ? lock_release+0x3c3/0x6a0
  ? __fget_files+0x21a/0x420
  ? __pfx_usbdev_ioctl+0x10/0x10
  __x64_sys_ioctl+0x171/0x1e0
  do_syscall_64+0x38/0x90
  entry_SYSCALL_64_after_hwframe+0x72/0xdc
RIP: 0033:0x4699cd
Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007f00232a2c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 000000000057c008 RCX: 00000000004699cd
RDX: 0000000020000180 RSI: 00000000c0105512 RDI: 0000000000000003
RBP: 00000000004d4a17 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 000000000057c008
R13: 00007ffe93c75b1f R14: 00007ffe93c75cc0 R15: 00007f00232a2dc0
  </TASK>



---
drivers/base/dd.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9c09ca5c4..fcd83226a 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1003,7 +1003,7 @@ static int __device_attach(struct device *dev, bool allow_async)
        bool async = false;

        device_lock(dev);
-       if (dev->p->dead) {
+       if (!dev->p||dev->p->dead) {
                goto out_unlock;
        } else if (dev->driver) {
                if (device_is_bound(dev)) { 

base-commit: 6995e2de6891c724bfeb2db33d7b87775f913ad1 (grafted, HEAD, tag: v6.4)

Re: Reply: A null-ptr crash in linux-6.4 usb driver

[gregkh]

On Wed, Nov 08, 2023 at 08:56:39AM +0000, 柳菁峰 wrote:
> A null-ptr crash was found in usb driver and it crashed with
> fault-inject.

Then don't do fault injection :)

> I have made a patch that simply checks for this null
> pointer, but I am not sure if this will affect certain functions or
> logic. I hope you can check it carefully please and the format meets
> the requirements this time.
> 
> 
> 
> The crash info:
> general protection fault, probably for non-canonical address 0xdffffc0000000021: 0000 [#1] PREEMPT SMP KASAN NOPTI
> KASAN: null-ptr-deref in range [0x0000000000000108-0x000000000000010f]
> CPU: 0 PID: 4280 Comm: syz-executor.3 Not tainted 6.4.0 #8
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.13.0-1ubuntu1.1 04/01/2014
> RIP: 0010:__device_attach+0xa9/0x450
> Code: e8 03 42 80 3c 20 00 0f 85 3f 03 00 00 48 b8 00 00 00 00 00 fc ff df 4c 8b 65 48 49 8d bc 24 08 01 00 00 48 89 fa 48 c1 ea 03 <0f> b6 04 02 84 c0 74 06 0f 8e f8 02 00 00 41 f6 84 24 08 01 00 00
> RSP: 0018:ffff888116927b98 EFLAGS: 00010206
> RAX: dffffc0000000000 RBX: 1ffff11022d24f73 RCX: 0000000000000000
> RDX: 0000000000000021 RSI: ffffffff847c3300 RDI: 0000000000000108
> RBP: ffff88811959b078 R08: 0000000000000000 R09: ffffffff860a0097
> R10: ffff888116927b98 R11: 0000000000000001 R12: 0000000000000000
> R13: ffff88811959b0f8 R14: 00000000fffffff0 R15: 0000000000000000
> FS:  00007f00232a3700(0000) GS:ffff88811ae00000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fff79859fb0 CR3: 000000010a18a005 CR4: 0000000000770ef0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> PKRU: 55555554
> Call Trace:
>   <TASK>
>   ? die_addr+0x38/0xa0
>   ? exc_general_protection+0x144/0x220
>   ? asm_exc_general_protection+0x22/0x30
>   ? __device_attach+0xa9/0x450
>   ? __device_attach+0x76/0x450
>   ? __pfx___device_attach+0x10/0x10
>   ? usb_ifnum_to_if+0x140/0x1d0
>   proc_ioctl.part.0+0x3ff/0x4a0
>   usbdev_ioctl+0x178a/0x3f70
>   ? __pfx_usbdev_ioctl+0x10/0x10
>   ? __pfx___lock_acquire+0x10/0x10
>   ? do_vfs_ioctl+0x120/0x1480
>   ? __pfx_do_vfs_ioctl+0x10/0x10
>   ? find_held_lock+0x2c/0x110
>   ? __fget_files+0x1f8/0x420
>   ? lock_release+0x3c3/0x6a0
>   ? __pfx_lock_release+0x10/0x10
>   ? lock_release+0x3c3/0x6a0
>   ? __fget_files+0x21a/0x420
>   ? __pfx_usbdev_ioctl+0x10/0x10
>   __x64_sys_ioctl+0x171/0x1e0
>   do_syscall_64+0x38/0x90
>   entry_SYSCALL_64_after_hwframe+0x72/0xdc
> RIP: 0033:0x4699cd
> Code: 02 b8 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 bc ff ff ff f7 d8 64 89 01 48
> RSP: 002b:00007f00232a2c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 000000000057c008 RCX: 00000000004699cd
> RDX: 0000000020000180 RSI: 00000000c0105512 RDI: 0000000000000003
> RBP: 00000000004d4a17 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 000000000057c008
> R13: 00007ffe93c75b1f R14: 00007ffe93c75cc0 R15: 00007f00232a2dc0
>   </TASK>
> 
> 
> 
> ---
> drivers/base/dd.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

You need a proper signed-off-by and description of what this patch does
here too.

> 
> 
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9c09ca5c4..fcd83226a 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1003,7 +1003,7 @@ static int __device_attach(struct device *dev, bool allow_async)
>         bool async = false;
> 
>         device_lock(dev);
> -       if (dev->p->dead) {
> +       if (!dev->p||dev->p->dead) {

How can p be NULL?

When p was assigned, why isn't it checked and handled properly then?

This isn't a change to the usb core, it's a change to the driver core.

I see this being properly checked in device_private_init(), so how can
this code path ever actually be hit?  Are you rewriting kernel memory
randomly with your fault injection code?

confused,

greg k-h