Message ID | 20240409143634.33230-1-n.zhandarovich@fintech.ru (mailing list archive) |
---|---|
State | New |
Delegated to: | Mauro Carvalho Chehab |
Headers |
Received: from sv.mirrors.kernel.org ([139.178.88.99]) by linuxtv.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.96) (envelope-from <linux-media+bounces-8934-patchwork=linuxtv.org@vger.kernel.org>) id 1ruCam-0001JQ-2K for patchwork@linuxtv.org; Tue, 09 Apr 2024 14:36:58 +0000 Received: from smtp.subspace.kernel.org (wormhole.subspace.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by sv.mirrors.kernel.org (Postfix) with ESMTPS id 180CC281D07 for <patchwork@linuxtv.org>; Tue, 9 Apr 2024 14:36:56 +0000 (UTC) Received: from localhost.localdomain (localhost.localdomain [127.0.0.1]) by smtp.subspace.kernel.org (Postfix) with ESMTP id E244A383A1; Tue, 9 Apr 2024 14:36:50 +0000 (UTC) X-Original-To: linux-media@vger.kernel.org Received: from exchange.fintech.ru (exchange.fintech.ru [195.54.195.159]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 414832C695; Tue, 9 Apr 2024 14:36:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=195.54.195.159 ARC-Seal: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712673410; cv=none; b=nDWnW8YJvg31LeO2iwO6xG+m67wd8+tOeLQwR3p1JyUitrjZ3Xw2mQQ1mfw/U/t+/22Bs6h2YtOgoiSLYY2JP0eWmqRYHcYn/rao3GF9dhIqfRbVqgtmsXbdHVDuLzp5m7Hceq4jMkkdZuyRsh4bf6o73rkAGmD1H/iG2tM4T3Q= ARC-Message-Signature: i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1712673410; c=relaxed/simple; bh=V48YeIPzcwVGjA7uCl+5dsWCWJXiaVFOuOZsOxK/qMw=; h=From:To:CC:Subject:Date:Message-ID:MIME-Version:Content-Type; b=oMs+khCXxhdWv+s0Y/ZyoUR7xe1ew3mf4u3ucdNXb4lTD3dLJS+/CeUWJ1sCXATJNk6h1su9MedUHvyE1Y1l00qlPZlAm692OGKcNUzMlioerlDEH8jDiXAy8dE2vr7mdYjA+03ah7svTFfiAoihdk+R1osY55LzR8G9gPm9ka0= ARC-Authentication-Results: i=1; smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=fintech.ru; spf=pass smtp.mailfrom=fintech.ru; arc=none smtp.client-ip=195.54.195.159 Authentication-Results: smtp.subspace.kernel.org; dmarc=none (p=none dis=none) header.from=fintech.ru Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=fintech.ru Received: from Ex16-01.fintech.ru (10.0.10.18) by exchange.fintech.ru (195.54.195.159) with Microsoft SMTP Server (TLS) id 14.3.498.0; Tue, 9 Apr 2024 17:36:37 +0300 Received: from localhost (10.0.253.138) by Ex16-01.fintech.ru (10.0.10.18) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2242.4; Tue, 9 Apr 2024 17:36:37 +0300 From: Nikita Zhandarovich <n.zhandarovich@fintech.ru> To: Mauro Carvalho Chehab <mchehab@kernel.org> CC: Nikita Zhandarovich <n.zhandarovich@fintech.ru>, Hans Verkuil <hverkuil-cisco@xs4all.nl>, Dongliang Mu <dzm91@hust.edu.cn>, Andrew Morton <akpm@linux-foundation.org>, Alan Stern <stern@rowland.harvard.edu>, <linux-media@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <lvc-project@linuxtesting.org>, <syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com> Subject: [PATCH] media: usb: siano: fix endpoint checks in smsusb_init_device() Date: Tue, 9 Apr 2024 07:36:34 -0700 Message-ID: <20240409143634.33230-1-n.zhandarovich@fintech.ru> X-Mailer: git-send-email 2.25.1 Precedence: bulk X-Mailing-List: linux-media@vger.kernel.org List-Id: <linux-media.vger.kernel.org> List-Subscribe: <mailto:linux-media+subscribe@vger.kernel.org> List-Unsubscribe: <mailto:linux-media+unsubscribe@vger.kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-ClientProxiedBy: Ex16-02.fintech.ru (10.0.10.19) To Ex16-01.fintech.ru (10.0.10.18) X-LSpam-Score: -4.8 (----) X-LSpam-Report: No, score=-4.8 required=5.0 tests=ARC_SIGNED=0.001,ARC_VALID=-0.1,BAYES_00=-1.9,DMARC_MISSING=0.001,HEADER_FROM_DIFFERENT_DOMAINS=0.5,MAILING_LIST_MULTI=-1,RCVD_IN_DNSWL_MED=-2.3,SPF_HELO_NONE=0.001,SPF_PASS=-0.001 autolearn=ham autolearn_force=no |
Series |
media: usb: siano: fix endpoint checks in smsusb_init_device()
|
|
Commit Message
Nikita Zhandarovich
April 9, 2024, 2:36 p.m. UTC
Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs
if an attempt is made to send a bulk URB using the wrong endpoint
type. The current approach to perform endpoint checking does not
explicitly check if an endpoint in question has its type set to bulk.
Fix this issue by using functions usb_endpoint_is_bulk_XXX() to
enable testing for correct ep types.
This patch has not been tested on real hardware.
[1] Syzkaller report:
usb 1-1: string descriptor 0 read error: -71
smsusb:smsusb_probe: board id=2, interface number 0
smsusb:siano_media_device_register: media controller created
------------[ cut here ]------------
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493
...
Call Trace:
smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195
smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475
smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566
usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396
really_probe+0x3cb/0x1020 drivers/base/dd.c:580
driver_probe_device+0x178/0x350 drivers/base/dd.c:763
...
hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644
process_one_work+0x833/0x10c0 kernel/workqueue.c:2276
worker_thread+0xac1/0x1300 kernel/workqueue.c:2422
kthread+0x39a/0x3c0 kernel/kthread.c:313
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com
Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
drivers/media/usb/siano/smsusb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
Comments
Em Tue, 9 Apr 2024 07:36:34 -0700 Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs > if an attempt is made to send a bulk URB using the wrong endpoint > type. The current approach to perform endpoint checking does not > explicitly check if an endpoint in question has its type set to bulk. > > Fix this issue by using functions usb_endpoint_is_bulk_XXX() to > enable testing for correct ep types. > > This patch has not been tested on real hardware. > > [1] Syzkaller report: > usb 1-1: string descriptor 0 read error: -71 > smsusb:smsusb_probe: board id=2, interface number 0 > smsusb:siano_media_device_register: media controller created > ------------[ cut here ]------------ > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493 > ... > Call Trace: > smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195 > smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475 > smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566 > usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396 > really_probe+0x3cb/0x1020 drivers/base/dd.c:580 > driver_probe_device+0x178/0x350 drivers/base/dd.c:763 > ... > hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644 > process_one_work+0x833/0x10c0 kernel/workqueue.c:2276 > worker_thread+0xac1/0x1300 kernel/workqueue.c:2422 > kthread+0x39a/0x3c0 kernel/kthread.c:313 > ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > > Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com > Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > drivers/media/usb/siano/smsusb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > index 723510520d09..daaac121c670 100644 > --- a/drivers/media/usb/siano/smsusb.c > +++ b/drivers/media/usb/siano/smsusb.c > @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > struct usb_endpoint_descriptor *desc = > &intf->cur_altsetting->endpoint[i].desc; > > - if (desc->bEndpointAddress & USB_DIR_IN) { > + if (usb_endpoint_is_bulk_in(desc)) { > dev->in_ep = desc->bEndpointAddress; > align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); > - } else { > + } else if (usb_endpoint_is_bulk_out(desc)) { Did you test it on what devices? I'm not sure if all siano devices are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out) instead of usb_endpoint_dir_(in|out)? Regards, Mauro > dev->out_ep = desc->bEndpointAddress; > } > } >
Hi On 5/3/24 08:58, Mauro Carvalho Chehab wrote: > Em Tue, 9 Apr 2024 07:36:34 -0700 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > >> Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs >> if an attempt is made to send a bulk URB using the wrong endpoint >> type. The current approach to perform endpoint checking does not >> explicitly check if an endpoint in question has its type set to bulk. >> >> Fix this issue by using functions usb_endpoint_is_bulk_XXX() to >> enable testing for correct ep types. >> >> This patch has not been tested on real hardware. >> >> [1] Syzkaller report: >> usb 1-1: string descriptor 0 read error: -71 >> smsusb:smsusb_probe: board id=2, interface number 0 >> smsusb:siano_media_device_register: media controller created >> ------------[ cut here ]------------ >> usb 1-1: BOGUS urb xfer, pipe 3 != type 1 >> WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493 >> ... >> Call Trace: >> smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195 >> smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475 >> smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566 >> usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396 >> really_probe+0x3cb/0x1020 drivers/base/dd.c:580 >> driver_probe_device+0x178/0x350 drivers/base/dd.c:763 >> ... >> hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644 >> process_one_work+0x833/0x10c0 kernel/workqueue.c:2276 >> worker_thread+0xac1/0x1300 kernel/workqueue.c:2422 >> kthread+0x39a/0x3c0 kernel/kthread.c:313 >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 >> >> Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com >> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> --- >> drivers/media/usb/siano/smsusb.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c >> index 723510520d09..daaac121c670 100644 >> --- a/drivers/media/usb/siano/smsusb.c >> +++ b/drivers/media/usb/siano/smsusb.c >> @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) >> struct usb_endpoint_descriptor *desc = >> &intf->cur_altsetting->endpoint[i].desc; >> >> - if (desc->bEndpointAddress & USB_DIR_IN) { >> + if (usb_endpoint_is_bulk_in(desc)) { >> dev->in_ep = desc->bEndpointAddress; >> align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); >> - } else { >> + } else if (usb_endpoint_is_bulk_out(desc)) { > > Did you test it on what devices? I'm not sure if all siano devices > are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out) > instead of usb_endpoint_dir_(in|out)? I did not have the hardware required to test it properly, I'm afraid. I made sure to point that out in the patch description. As for siano devices possibly having different endpoints type apart from bulk, I was relying on the rest of the driver code to determine just that (which is maybe somewhat flawed in this case). Since smsusb_submit_urb() (and some other functions like usb_rcvbulkpipe()) works exclusively with bulk types, I did not feel the need to even expect int types, for instance. > > Regards, > Mauro > >> dev->out_ep = desc->bEndpointAddress; >> } >> } >> With regards, Nikita
Em Fri, 3 May 2024 09:14:37 -0700 Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > Hi > > On 5/3/24 08:58, Mauro Carvalho Chehab wrote: > > Em Tue, 9 Apr 2024 07:36:34 -0700 > > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > > > >> Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs > >> if an attempt is made to send a bulk URB using the wrong endpoint > >> type. The current approach to perform endpoint checking does not > >> explicitly check if an endpoint in question has its type set to bulk. > >> > >> Fix this issue by using functions usb_endpoint_is_bulk_XXX() to > >> enable testing for correct ep types. > >> > >> This patch has not been tested on real hardware. > >> > >> [1] Syzkaller report: > >> usb 1-1: string descriptor 0 read error: -71 > >> smsusb:smsusb_probe: board id=2, interface number 0 > >> smsusb:siano_media_device_register: media controller created > >> ------------[ cut here ]------------ > >> usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > >> WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493 > >> ... > >> Call Trace: > >> smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195 > >> smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475 > >> smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566 > >> usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396 > >> really_probe+0x3cb/0x1020 drivers/base/dd.c:580 > >> driver_probe_device+0x178/0x350 drivers/base/dd.c:763 > >> ... > >> hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644 > >> process_one_work+0x833/0x10c0 kernel/workqueue.c:2276 > >> worker_thread+0xac1/0x1300 kernel/workqueue.c:2422 > >> kthread+0x39a/0x3c0 kernel/kthread.c:313 > >> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 > >> > >> Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com > >> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") > >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > >> --- > >> drivers/media/usb/siano/smsusb.c | 4 ++-- > >> 1 file changed, 2 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > >> index 723510520d09..daaac121c670 100644 > >> --- a/drivers/media/usb/siano/smsusb.c > >> +++ b/drivers/media/usb/siano/smsusb.c > >> @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > >> struct usb_endpoint_descriptor *desc = > >> &intf->cur_altsetting->endpoint[i].desc; > >> > >> - if (desc->bEndpointAddress & USB_DIR_IN) { > >> + if (usb_endpoint_is_bulk_in(desc)) { > >> dev->in_ep = desc->bEndpointAddress; > >> align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); > >> - } else { > >> + } else if (usb_endpoint_is_bulk_out(desc)) { > > > > Did you test it on what devices? I'm not sure if all siano devices > > are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out) > > instead of usb_endpoint_dir_(in|out)? > > I did not have the hardware required to test it properly, I'm afraid. > I made sure to point that out in the patch description. > > As for siano devices possibly having different endpoints type apart from > bulk, I was relying on the rest of the driver code to determine just > that (which is maybe somewhat flawed in this case). Most digital TV devices also have ISOC endpoints. I'm not sure about Siano. > Since > smsusb_submit_urb() (and some other functions like usb_rcvbulkpipe()) > works exclusively with bulk types, I did not feel the need to even > expect int types, for instance. The problem is not with int endpoints. Most media devices have isoc endpoints. There are even cases that different device models supported by the same driver have some bulk and some isoc endpoints. See em28xx driver for instance: most devices are isoc only; some have both isoc and bulk; a few have just bulk. In the specific case of siano, it supports 3 generations of devices (sms1000, sms11xx, sms22xx), each with several submodels. Those 3 generations have different specs, and sms1000 even have one "boot" USB ID, and one "warm" USB ID, each one with different endpoints. That's basically why I'm afraid of a patch like this could cause regressions if not properly tested, as it is changing the logic from just detecting the direction to also validate if the endpoint is not isoc. Regards, Mauro
Hi On 5/3/24 14:20, Mauro Carvalho Chehab wrote: > Em Fri, 3 May 2024 09:14:37 -0700 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > >>> Did you test it on what devices? I'm not sure if all siano devices >>> are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out) >>> instead of usb_endpoint_dir_(in|out)? >> >> I did not have the hardware required to test it properly, I'm afraid. >> I made sure to point that out in the patch description. >> >> As for siano devices possibly having different endpoints type apart from >> bulk, I was relying on the rest of the driver code to determine just >> that (which is maybe somewhat flawed in this case). > > Most digital TV devices also have ISOC endpoints. I'm not sure about > Siano. After a quick glance at Siano driver I am not sure about that being true specifically for Siano devices just yet, but I understand your point nonetheless. > >> Since >> smsusb_submit_urb() (and some other functions like usb_rcvbulkpipe()) >> works exclusively with bulk types, I did not feel the need to even >> expect int types, for instance. > > The problem is not with int endpoints. Most media devices have isoc > endpoints. There are even cases that different device models supported > by the same driver have some bulk and some isoc endpoints. See em28xx > driver for instance: most devices are isoc only; some have both isoc > and bulk; a few have just bulk. > > In the specific case of siano, it supports 3 generations of devices > (sms1000, sms11xx, sms22xx), each with several submodels. > Those 3 generations have different specs, and sms1000 even have one > "boot" USB ID, and one "warm" USB ID, each one with different endpoints. > > That's basically why I'm afraid of a patch like this could cause > regressions if not properly tested, as it is changing the logic > from just detecting the direction to also validate if the endpoint > is not isoc. Thank you for being so thorough with your answer. I think I overestimated how much I should trust current driver implementation. I should be (at least, in this case) looking at what hardware expects of the driver. Let me try to do a little more research into smsxxxx specs and maybe see about getting my hands on some actual devices for testing. > > Regards, > Mauro With regards, Nikita
Hi Mauro, On 5/3/24 14:20, Mauro Carvalho Chehab wrote: > Em Fri, 3 May 2024 09:14:37 -0700 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > >> Hi >> >> On 5/3/24 08:58, Mauro Carvalho Chehab wrote: >>> Em Tue, 9 Apr 2024 07:36:34 -0700 >>> Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: >>> >>>> Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs >>>> if an attempt is made to send a bulk URB using the wrong endpoint >>>> type. The current approach to perform endpoint checking does not >>>> explicitly check if an endpoint in question has its type set to bulk. >>>> >>>> Fix this issue by using functions usb_endpoint_is_bulk_XXX() to >>>> enable testing for correct ep types. >>>> >>>> This patch has not been tested on real hardware. >>>> >>>> [1] Syzkaller report: >>>> usb 1-1: string descriptor 0 read error: -71 >>>> smsusb:smsusb_probe: board id=2, interface number 0 >>>> smsusb:siano_media_device_register: media controller created >>>> ------------[ cut here ]------------ >>>> usb 1-1: BOGUS urb xfer, pipe 3 != type 1 >>>> WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493 >>>> ... >>>> Call Trace: >>>> smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195 >>>> smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475 >>>> smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566 >>>> usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396 >>>> really_probe+0x3cb/0x1020 drivers/base/dd.c:580 >>>> driver_probe_device+0x178/0x350 drivers/base/dd.c:763 >>>> ... >>>> hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644 >>>> process_one_work+0x833/0x10c0 kernel/workqueue.c:2276 >>>> worker_thread+0xac1/0x1300 kernel/workqueue.c:2422 >>>> kthread+0x39a/0x3c0 kernel/kthread.c:313 >>>> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294 >>>> >>>> Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@syzkaller.appspotmail.com >>>> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") >>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >>>> --- >>>> drivers/media/usb/siano/smsusb.c | 4 ++-- >>>> 1 file changed, 2 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c >>>> index 723510520d09..daaac121c670 100644 >>>> --- a/drivers/media/usb/siano/smsusb.c >>>> +++ b/drivers/media/usb/siano/smsusb.c >>>> @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) >>>> struct usb_endpoint_descriptor *desc = >>>> &intf->cur_altsetting->endpoint[i].desc; >>>> >>>> - if (desc->bEndpointAddress & USB_DIR_IN) { >>>> + if (usb_endpoint_is_bulk_in(desc)) { >>>> dev->in_ep = desc->bEndpointAddress; >>>> align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); >>>> - } else { >>>> + } else if (usb_endpoint_is_bulk_out(desc)) { >>> >>> Did you test it on what devices? I'm not sure if all siano devices >>> are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out) >>> instead of usb_endpoint_dir_(in|out)? >> >> I did not have the hardware required to test it properly, I'm afraid. >> I made sure to point that out in the patch description. >> >> As for siano devices possibly having different endpoints type apart from >> bulk, I was relying on the rest of the driver code to determine just >> that (which is maybe somewhat flawed in this case). > > Most digital TV devices also have ISOC endpoints. I'm not sure about > Siano. > >> Since >> smsusb_submit_urb() (and some other functions like usb_rcvbulkpipe()) >> works exclusively with bulk types, I did not feel the need to even >> expect int types, for instance. > > The problem is not with int endpoints. Most media devices have isoc > endpoints. There are even cases that different device models supported > by the same driver have some bulk and some isoc endpoints. See em28xx > driver for instance: most devices are isoc only; some have both isoc > and bulk; a few have just bulk. > > In the specific case of siano, it supports 3 generations of devices > (sms1000, sms11xx, sms22xx), each with several submodels. > Those 3 generations have different specs, and sms1000 even have one > "boot" USB ID, and one "warm" USB ID, each one with different endpoints. > > That's basically why I'm afraid of a patch like this could cause > regressions if not properly tested, as it is changing the logic > from just detecting the direction to also validate if the endpoint > is not isoc. > > Regards, > Mauro After doing a tiny bit of research I've had a couple more questions as regards to dealing with ISOC endpoints in siano devices. I am still not exactly versed in the subject, so forgive my ignorance. Firstly, judging from the current state of siano driver I think that sms1xxx devices do not actually deal with ISOC or some other "special" endpoints apart from bulk ones. em28xx and at least a couple of other drivers that may expect ISOC eps, check for their presence and prepare URBs differently, checking for pipe types and filling URBs with usb_rcvisocpipe() and usb_fill_int_urb() accordingly. Since siano essentially uses only bulk functions, it either treats isoc and bulk (and maybe other types) eps as bulk only - which seems wrong to me and implies problems with devices as a result of this; or siano driver doesn't actually require isoc-specific actions (or for other types). I want to point out that I myself tend to agree with latter option, once again judging by the way driver currently works with eps. Which brings me to another question - if you could share an example of a spec or datasheet for one of the smsxxxx devices that explicitly confirms presence of other ep types, that would be much appreciated. For some reason, I failed to find something meaningful on my own. Also, I could use some direction on how to properly test my change on real hardware. I am looking towards something along the lines of https://linuxtv.org/wiki/index.php/Smart_Plus as a test subject. If there are some examples/approaches you think are appropriate for this, I'll gladly adopt them. Thank you for your patience :) Regards, Nikita
diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c index 723510520d09..daaac121c670 100644 --- a/drivers/media/usb/siano/smsusb.c +++ b/drivers/media/usb/siano/smsusb.c @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) struct usb_endpoint_descriptor *desc = &intf->cur_altsetting->endpoint[i].desc; - if (desc->bEndpointAddress & USB_DIR_IN) { + if (usb_endpoint_is_bulk_in(desc)) { dev->in_ep = desc->bEndpointAddress; align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr); - } else { + } else if (usb_endpoint_is_bulk_out(desc)) { dev->out_ep = desc->bEndpointAddress; } }