| Message ID | 1454929965-19897-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | Superseded |
| Headers | show |
Hello, On 2016-02-08 12:31, Vegard Nossum wrote: > On 02/08/2016 12:12 PM, Marek Szyprowski wrote: >> Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget: >> udc-core: independent registration of gadgets and gadget drivers") >> gadget >> drivers can not assume that UDC drivers are already available on their >> initialization. This broke the HACK, which was used in gadgetfs driver, >> to get UDC controller name. This patch removes this hack and replaces it >> by additional function in the UDC core (which is usefully only for >> legacy >> drivers, please don't use it in the new code). >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> --- >> Vegard: Could you check if this patch fixes your issue with gadgetfs >> and NULL >> pointer dereference? >> >> Best regards, >> Marek Szyprowski >> --- > > [snip patch] > > Thanks for the patch, I gave it a try. > > Firstly, it changes /dev/gadget/dummy_udc into /dev/gadget/dummy_udc.0 > so it may break some userspace expectations (I don't really know). Right, thanks for pointing this issue. > > Secondly, I still get this crash which looks a lot like what I > originally reported: > > kasan: GPF could be caused by NULL-ptr deref or user memory > accessgeneral protection fault: 0000 [#1] DEBUG_PAGEALLOC KASAN > CPU: 0 PID: 35 Comm: afl-fuzz Not tainted 4.5.0-rc2 #1 > task: ffff8800003b6900 ti: ffff88000c840000 task.ti: ffff88000c840000 > RIP: 0010:[<ffffffff81388536>] [<ffffffff81388536>] > __list_del_entry+0x86/0x1d0 > RSP: 0018:ffff88000c847da8 EFLAGS: 00010246 > RAX: dffffc0000000000 RBX: 0000000000000000 RCX: ffffffff81a13f08 > RDX: 0000000000000000 RSI: 0000000000000061 RDI: ffffffff81a13f10 > RBP: ffff88000c847dc0 R08: 0000000000000246 R09: 0000000000000000 > R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000 > R13: dffffc0000000000 R14: ffffffff81a13e40 R15: ffff88000c83c500 > FS: 00007ffff7ff2740(0000) GS:ffffffff8193f000(0000) > knlGS:0000000000000000 > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > CR2: 00007ffff78c53a0 CR3: 000000000c850000 CR4: 00000000001406b0 > Stack: > ffffffff81a13e40 ffffffff81a13f08 ffffffff81a118e0 ffff88000c847dd8 > ffffffff8138868d ffffffff81a11638 ffff88000c847e10 ffffffff81523a5d > ffffffff817f62a0 ffff880000277a40 ffff88000c83c510 ffff88000c83c520 > Call Trace: > [<ffffffff8138868d>] list_del+0xd/0x70 > [<ffffffff81523a5d>] usb_gadget_unregister_driver+0x11d/0x240 > [<ffffffff81533c34>] dev_release+0x44/0x110 > [<ffffffff811f0ccb>] __fput+0x11b/0x490 > [<ffffffff811f10a9>] ____fput+0x9/0x10 > [<ffffffff810c8881>] task_work_run+0xf1/0x190 > [<ffffffff811ea9ea>] ? filp_close+0x8a/0xe0 > [<ffffffff81001c3c>] exit_to_usermode_loop+0xec/0x100 > [<ffffffff81002531>] syscall_return_slowpath+0x91/0xc0 > [<ffffffff817a4389>] int_ret_from_sys_call+0x25/0x8f > Code: c4 0f 84 94 00 00 00 48 b8 00 02 00 00 00 00 ad de 48 39 c3 0f > 84 a5 00 00 00 48 89 da 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 <80> > 3c 02 00 0f 85 e8 00 00 00 4c 8b 03 4c 39 c1 0f 85 9b 00 00 > RIP [<ffffffff81388536>] __list_del_entry+0x86/0x1d0 > RSP <ffff88000c847da8> > ---[ end trace 9a6416535ca1ec01 ]--- > > I am more than happy to try other patches :-) Thanks, Okay, now I managed to reproduce it and I've sent a fix for gadgetfs driver a few minutes ago. When both patches are applied, no more issue should be observed. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c index 7e179f81d05c..7a62a2f7bc18 100644 --- a/drivers/usb/gadget/legacy/inode.c +++ b/drivers/usb/gadget/legacy/inode.c @@ -227,7 +227,7 @@ static void put_ep (struct ep_data *data) * implicitly, starting with the driver name and then endpoint names. */ -static const char *CHIP; +static char CHIP[32]; /*----------------------------------------------------------------------*/ @@ -1697,28 +1697,6 @@ static struct usb_gadget_driver gadgetfs_driver = { }; /*----------------------------------------------------------------------*/ - -static void gadgetfs_nop(struct usb_gadget *arg) { } - -static int gadgetfs_probe(struct usb_gadget *gadget, - struct usb_gadget_driver *driver) -{ - CHIP = gadget->name; - return -EISNAM; -} - -static struct usb_gadget_driver probe_driver = { - .max_speed = USB_SPEED_HIGH, - .bind = gadgetfs_probe, - .unbind = gadgetfs_nop, - .setup = (void *)gadgetfs_nop, - .disconnect = gadgetfs_nop, - .driver = { - .name = "nop", - }, -}; - - /* DEVICE INITIALIZATION * * fd = open ("/dev/gadget/$CHIP", O_RDWR) @@ -1968,10 +1946,7 @@ gadgetfs_fill_super (struct super_block *sb, void *opts, int silent) if (the_device) return -ESRCH; - /* fake probe to determine $CHIP */ - CHIP = NULL; - usb_gadget_probe_driver(&probe_driver); - if (!CHIP) + if (usb_get_gadget_udc_name(CHIP, sizeof(CHIP)) != 0) return -ENODEV; /* superblock */ diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c index fd73a3ea07c2..eb70051d5949 100644 --- a/drivers/usb/gadget/udc/udc-core.c +++ b/drivers/usb/gadget/udc/udc-core.c @@ -442,6 +442,39 @@ err1: EXPORT_SYMBOL_GPL(usb_add_gadget_udc_release); /** + * usb_get_gadget_udc_name - get the name of the first UDC controller + * @dst_name + * @len + * This functions returns the name of the first UDC controller in the system. + * Please note that this interface is usefull only for legacy drivers which + * assume that there is only one UDC controller in the system and they need to + * get its name before initialization. There is no guarantee that the UDC + * of the returned name will be still available, when gadget driver registers + * itself. + * + * Returns zero on success, negative errno otherwise. + */ +int usb_get_gadget_udc_name(char *dst_name, int len) +{ + struct usb_udc *udc; + int ret = -ENODEV; + + mutex_lock(&udc_lock); + list_for_each_entry(udc, &udc_list, list) { + const char *name = dev_name(&udc->dev); + /* For now we take the first one */ + if (!udc->driver && strlen(name) + 1 <= len) { + strcpy(dst_name, name); + ret = 0; + break; + } + } + mutex_unlock(&udc_lock); + return ret; +} +EXPORT_SYMBOL_GPL(usb_get_gadget_udc_name); + +/** * usb_add_gadget_udc - adds a new gadget to the udc class driver list * @parent: the parent device to this udc. Usually the controller * driver's device. diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index d82d0068872b..0a8f08302a34 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -1126,6 +1126,7 @@ extern int usb_add_gadget_udc_release(struct device *parent, struct usb_gadget *gadget, void (*release)(struct device *dev)); extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget); extern void usb_del_gadget_udc(struct usb_gadget *gadget); +extern int usb_get_gadget_udc_name(char *dst_name, int len); /*-------------------------------------------------------------------------*/
Since commit 855ed04a3758b205e84b269f92d26ab36ed8e2f7 ("usb: gadget: udc-core: independent registration of gadgets and gadget drivers") gadget drivers can not assume that UDC drivers are already available on their initialization. This broke the HACK, which was used in gadgetfs driver, to get UDC controller name. This patch removes this hack and replaces it by additional function in the UDC core (which is usefully only for legacy drivers, please don't use it in the new code). Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> --- Vegard: Could you check if this patch fixes your issue with gadgetfs and NULL pointer dereference? Best regards, Marek Szyprowski --- drivers/usb/gadget/legacy/inode.c | 29 ++--------------------------- drivers/usb/gadget/udc/udc-core.c | 33 +++++++++++++++++++++++++++++++++ include/linux/usb/gadget.h | 1 + 3 files changed, 36 insertions(+), 27 deletions(-) -- 1.9.2 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html