Re: gadgetfs regression (NULL ptr deref) since v4.4-rc7

[Ruslan Bilovol]

Hi Vegard,

On Mon, Feb 8, 2016 at 1:27 AM, Vegard Nossum <vegard.nossum@xxxxxxxxxx> wrote:
> Hi,
>
> Using gadgetfs on latest mainline, I get the following NULL pointer
> dereference:
>
> BUG: unable to handle kernel NULL pointer dereference at           (null)
> IP: [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
> PGD 34f067 PUD 355067 PMD 0
> Oops: 0000 [#1] DEBUG_PAGEALLOC
> CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1
> task: ffff8800002b1ec0 ti: ffff88000033c000 task.ti: ffff88000033c000
> RIP: 0010:[<ffffffff81138e89>]  [<ffffffff81138e89>]
> __list_del_entry+0x29/0xc0
> RSP: 0018:ffff88000033fe30  EFLAGS: 00010207
> RAX: 0000000000000000 RBX: ffffffff8138c108 RCX: dead000000000200
> RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff8138c108
> RBP: ffff88000033fe30 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: ffffffff8138b928
> R13: ffffffff8138c040 R14: ffff88000ec0da20 R15: ffff88000033ff58
> FS:  00007ffff7ff2740(0000) GS:ffffffff8135d000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000000000000000 CR3: 0000000000335000 CR4: 00000000001406b0
> Stack:
>  ffff88000033fe48 ffffffff81138f2d ffffffff8138bbd0 ffff88000033fe70
>  ffffffff811c880b ffff8800002f3000 ffff88000ec109a0 ffff880000298aa0
>  ffff88000033fe88 ffffffff811ce040 ffff88000034cdc0 ffff88000033feb8
> Call Trace:
>  [<ffffffff81138f2d>] list_del+0xd/0x30
>  [<ffffffff811c880b>] usb_gadget_unregister_driver+0xdb/0xf0
>  [<ffffffff811ce040>] dev_release+0x20/0x60
>  [<ffffffff810b464c>] __fput+0x4c/0x130
>  [<ffffffff810b4769>] ____fput+0x9/0x10
>  [<ffffffff81048577>] task_work_run+0x67/0xa0
>  [<ffffffff81000dcf>] exit_to_usermode_loop+0x8f/0xa0
>  [<ffffffff8100105d>] syscall_return_slowpath+0x3d/0x40
>  [<ffffffff81278b89>] int_ret_from_sys_call+0x25/0x8f
> Code: ff ff 55 48 8b 07 48 b9 00 01 00 00 00 00 ad de 48 8b 57 08 48 89 e5
> 48 39 c8 74 29 48 b9 00 02 00 00 00 00 ad de 48 39 ca 74 3a <4c> 8b 02 4c 39
> c7 75 52 4c 8b 40 08 4c 39 c7 75 66 48 89 50 08
> RIP  [<ffffffff81138e89>] __list_del_entry+0x29/0xc0
>  RSP <ffff88000033fe30>
> CR2: 0000000000000000
> ---[ end trace e6cfe1de661dcffe ]---
>
> Reverting this commit fixes the problem for me:
>
> commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7
> Author: Ruslan Bilovol <ruslan.bilovol@xxxxxxxxx>
> Date:   Mon Nov 23 09:56:38 2015 +0100
>
>     usb: gadget: udc-core: independent registration of gadgets and gadget
> drivers
>
> Though I am still seeing some occasional lockups. Thanks,
>

Original version of this patch had checking of input parameters
(see change in usb_gadget_unregister_driver at
https://lkml.org/lkml/2015/6/22/559) but this approach was
rejected by Alan Stern many times so final version pushed by
Marek Szyprowski doesn't have it, and we have this NULL pointer
dereference.
But this means there is a bug in GadgetFS that was hidden
previously: it tries to deregister gadget driver that was not previously
registered. Previous implementation was returning -ENODEV
retval, current one just does NULL pointer dereference.

At least need to fix GadgetFS that seems to by buggy in
gadget driver unregistering, and I still say we need to nave
input parameters checking in externally visible functions like
usb_gadget_unregister_driver().

Meanwhile you can try following patch that returns checking of input
parameters back and see if it helps.

Best regards,
Ruslan

--------

diff --git a/drivers/usb/gadget/udc/udc-core.c
b/drivers/usb/gadget/udc/udc-core.c
index fd73a3e..f1d375f 100644
--- a/drivers/usb/gadget/udc/udc-core.c
+++ b/drivers/usb/gadget/udc/udc-core.c
@@ -597,9 +597,16 @@ int usb_gadget_unregister_driver(struct
usb_gadget_driver *driver)
                }

        if (ret) {
-               list_del(&driver->pending);
-               ret = 0;
+               struct usb_gadget_driver *tmp;
+
+               list_for_each_entry(tmp, &gadget_driver_pending_list, pending)
+                       if (tmp == driver) {
+                               list_del(&driver->pending);
+                               ret = 0;
+                               break;
+                       }
        }
+
        mutex_unlock(&udc_lock);
        return ret;
 }