Re: [Question] do we need fail __device_add_disk when functions in it return error

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Wed, Apr 07, 2021 at 07:50:20PM +0800, Yufen Yu wrote:
> Hi, all
> 
> Recently, our syzbot test reported NULL pointer dereference in device_del()
> by injecting memory allocation fail in device_add() invoking from register_disk(),
> as following:
> 
> general protection fault, probably for non-canonical address 0xdffffc0000000021: 0000 [#1] PREEMPT SMP KASAN
> KASAN: null-ptr-deref in range [0x0000000000000108-0x000000000000010f]
> CPU: 0 PID: 9441 Comm: syz-executor348 Tainted: G        W         5.10.0+ #6
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Ubuntu-1.8.2-1ubuntu1 04/01/2014
> RIP: 0010:kill_device+0x9b/0x160 drivers/base/core.c:3050
> Code: fd 49 83 c6 48 4c 89 f3 48 c1 eb 03 42 80 3c 3b 00 74 08 4c 89 f7 e8 94 ef 8c fd bd 08 01 00 00 49 03 2e 48 89 e8 48 c1 e8 03 <42> 8a 04 38 84 c0 0f 85 89 00 00 00 0f b6 6d 00 83 e5 01 31 ff 89
> RSP: 0018:ffffc900032bfd38 EFLAGS: 00010206
> RAX: 0000000000000021 RBX: 1ffff1100299b816 RCX: ffff8881115cc200
> RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000005
> RBP: 0000000000000108 R08: dffffc0000000000 R09: ffff8881115cc200
> R10: ffffed100299b830 R11: 0000000000000000 R12: dffffc0000000000
> R13: 0000000000000008 R14: ffff888014cdc0b0 R15: dffffc0000000000
> FS:  0000000000e29880(0000) GS:ffff888061000000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00000000006cc098 CR3: 0000000018764000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  device_del+0x62/0xa00 drivers/base/core.c:3078
>  del_gendisk+0x725/0x880 block/genhd.c:952
>  loop_remove+0x42/0xa0 drivers/block/loop.c:2193
>  loop_control_ioctl+0x31d/0x3a0 drivers/block/loop.c:2292
>  vfs_ioctl+0xa2/0xf0 fs/ioctl.c:48
>  __do_sys_ioctl fs/ioctl.c:753 [inline]
>  __se_sys_ioctl fs/ioctl.c:739 [inline]
>  __x64_sys_ioctl+0xfe/0x140 fs/ioctl.c:739
>  do_syscall_64+0x2d/0x70 arch/x86/entry/common.c:46
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> When device_add() fail caused by some memory allocation fail, it will free
> 'dev->p' and set the pointer as NULL. Since add_disk() will ignore the functions
> fail and go on working, it will cause various NULL pointer dereference in del_gendisk(),
> including 'dev->p' is NULL in kill_device() from device_del(), 'name' is NULL in
> sysfs_remove_link() from del_gendisk(), kobj->sd is 'NULL' when call dpm_sysfs_remove()
> from device_del(), and so on.
> 
> I try to fix the the problem by passing device_del() when device_add() fail[1], which is
> is not complete. And Greg thinks we should fix up users of device_del().
> 
> My question is do we need to check return value of register_disk() and fail __device_add_disk()
> when functions in it fail.
> Is there some historical reason that let us ignore various fail from __device_add_disk()
> and register_disk()?

Probably because the only way you can ever hit this is if you are
injecting faults into the system.  Can you ever normally fail the memory
allocation any other way under a normal system operation?

Try making a patch and see what it looks like...

good luck!

greg k-h