[PATCH net] net: usb: rtl8150: enable basic endpoint checking

[PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Nikita Zhandarovich]

Syzkaller reports [1] encountering a common issue of utilizing a wrong
usb endpoint type during URB submitting stage. This, in turn, triggers
a warning shown below.

For now, enable simple endpoint checking (specifically, bulk and
interrupt eps, testing control one is not essential) to mitigate
the issue with a view to do other related cosmetic changes later,
if they are necessary.

[1] Syzkaller report:
usb 1-1: BOGUS urb xfer, pipe 3 != type 1
WARNING: CPU: 1 PID: 2586 at drivers/usb/core/urb.c:503 usb_submit_urb+0xe4b/0x1730 driv>
Modules linked in:
CPU: 1 UID: 0 PID: 2586 Comm: dhcpcd Not tainted 6.11.0-rc4-syzkaller-00069-gfc88bb11617>
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
RIP: 0010:usb_submit_urb+0xe4b/0x1730 drivers/usb/core/urb.c:503
Code: 84 3c 02 00 00 e8 05 e4 fc fc 4c 89 ef e8 fd 25 d7 fe 45 89 e0 89 e9 4c 89 f2 48 8>
RSP: 0018:ffffc9000441f740 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffff888112487a00 RCX: ffffffff811a99a9
RDX: ffff88810df6ba80 RSI: ffffffff811a99b6 RDI: 0000000000000001
RBP: 0000000000000003 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
R13: ffff8881023bf0a8 R14: ffff888112452a20 R15: ffff888112487a7c
FS:  00007fc04eea5740(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007f0a1de9f870 CR3: 000000010dbd0000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
 <TASK>
 rtl8150_open+0x300/0xe30 drivers/net/usb/rtl8150.c:733
 __dev_open+0x2d4/0x4e0 net/core/dev.c:1474
 __dev_change_flags+0x561/0x720 net/core/dev.c:8838
 dev_change_flags+0x8f/0x160 net/core/dev.c:8910
 devinet_ioctl+0x127a/0x1f10 net/ipv4/devinet.c:1177
 inet_ioctl+0x3aa/0x3f0 net/ipv4/af_inet.c:1003
 sock_do_ioctl+0x116/0x280 net/socket.c:1222
 sock_ioctl+0x22e/0x6c0 net/socket.c:1341
 vfs_ioctl fs/ioctl.c:51 [inline]
 __do_sys_ioctl fs/ioctl.c:907 [inline]
 __se_sys_ioctl fs/ioctl.c:893 [inline]
 __x64_sys_ioctl+0x193/0x220 fs/ioctl.c:893
 do_syscall_x64 arch/x86/entry/common.c:52 [inline]
 do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fc04ef73d49
...

This change has not been tested on real hardware.

Reported-and-tested-by: syzbot+d7e968426f644b567e31@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d7e968426f644b567e31
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
 drivers/net/usb/rtl8150.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
index 01a3b2417a54..f77af8cf543c 100644
--- a/drivers/net/usb/rtl8150.c
+++ b/drivers/net/usb/rtl8150.c
@@ -71,6 +71,14 @@
 #define MSR_SPEED		(1<<3)
 #define MSR_LINK		(1<<2)
 
+/* USB endpoints */
+enum rtl8150_usb_ep {
+	RTL8150_USB_EP_CONTROL = 0,
+	RTL8150_USB_EP_BULK_IN = 1,
+	RTL8150_USB_EP_BULK_OUT = 2,
+	RTL8150_USB_EP_INT_IN = 3,
+};
+
 /* Interrupt pipe data */
 #define INT_TSR			0x00
 #define INT_RSR			0x01
@@ -880,6 +888,20 @@ static int rtl8150_probe(struct usb_interface *intf,
 		return -ENOMEM;
 	}
 
+	/* Verify that all required endpoints are present */
+	static const u8 bulk_ep_addr[] = {
+		RTL8150_USB_EP_BULK_IN | USB_DIR_IN,
+		RTL8150_USB_EP_BULK_OUT | USB_DIR_OUT,
+		0};
+	static const u8 int_ep_addr[] = {
+		RTL8150_USB_EP_INT_IN | USB_DIR_IN,
+		0};
+	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
+	    !usb_check_int_endpoints(intf, int_ep_addr)) {
+		dev_err(&intf->dev, "couldn't find required endpoints\n");
+		goto out;
+	}
+
 	tasklet_setup(&dev->tl, rx_fixup);
 	spin_lock_init(&dev->rx_pool_lock);
 

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Petko Manolov]

On 25-01-22 02:42:46, Nikita Zhandarovich wrote:
> Syzkaller reports [1] encountering a common issue of utilizing a wrong usb
> endpoint type during URB submitting stage. This, in turn, triggers a warning
> shown below.

If these endpoints were of the wrong type the driver simply wouldn't work.

The proposed change in the patch doesn't do much in terms of fixing the issue
(pipe 3 != type 1) and if usb_check_bulk_endpoints() fails, the driver will just
not probe successfully.  I don't see how this is an improvement to the current
situation.

We should either spend some time fixing the "BOGUS urb xfer, pipe 3 != type 1"
for real or not touch anything.


		Petko


> For now, enable simple endpoint checking (specifically, bulk and
> interrupt eps, testing control one is not essential) to mitigate
> the issue with a view to do other related cosmetic changes later,
> if they are necessary.
> 
> [1] Syzkaller report:
> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> WARNING: CPU: 1 PID: 2586 at drivers/usb/core/urb.c:503 usb_submit_urb+0xe4b/0x1730 driv>
> Modules linked in:
> CPU: 1 UID: 0 PID: 2586 Comm: dhcpcd Not tainted 6.11.0-rc4-syzkaller-00069-gfc88bb11617>
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
> RIP: 0010:usb_submit_urb+0xe4b/0x1730 drivers/usb/core/urb.c:503
> Code: 84 3c 02 00 00 e8 05 e4 fc fc 4c 89 ef e8 fd 25 d7 fe 45 89 e0 89 e9 4c 89 f2 48 8>
> RSP: 0018:ffffc9000441f740 EFLAGS: 00010282
> RAX: 0000000000000000 RBX: ffff888112487a00 RCX: ffffffff811a99a9
> RDX: ffff88810df6ba80 RSI: ffffffff811a99b6 RDI: 0000000000000001
> RBP: 0000000000000003 R08: 0000000000000001 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
> R13: ffff8881023bf0a8 R14: ffff888112452a20 R15: ffff888112487a7c
> FS:  00007fc04eea5740(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007f0a1de9f870 CR3: 000000010dbd0000 CR4: 00000000003506f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
>  <TASK>
>  rtl8150_open+0x300/0xe30 drivers/net/usb/rtl8150.c:733
>  __dev_open+0x2d4/0x4e0 net/core/dev.c:1474
>  __dev_change_flags+0x561/0x720 net/core/dev.c:8838
>  dev_change_flags+0x8f/0x160 net/core/dev.c:8910
>  devinet_ioctl+0x127a/0x1f10 net/ipv4/devinet.c:1177
>  inet_ioctl+0x3aa/0x3f0 net/ipv4/af_inet.c:1003
>  sock_do_ioctl+0x116/0x280 net/socket.c:1222
>  sock_ioctl+0x22e/0x6c0 net/socket.c:1341
>  vfs_ioctl fs/ioctl.c:51 [inline]
>  __do_sys_ioctl fs/ioctl.c:907 [inline]
>  __se_sys_ioctl fs/ioctl.c:893 [inline]
>  __x64_sys_ioctl+0x193/0x220 fs/ioctl.c:893
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> RIP: 0033:0x7fc04ef73d49
> ...
> 
> This change has not been tested on real hardware.
> 
> Reported-and-tested-by: syzbot+d7e968426f644b567e31@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d7e968426f644b567e31
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
>  drivers/net/usb/rtl8150.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
> index 01a3b2417a54..f77af8cf543c 100644
> --- a/drivers/net/usb/rtl8150.c
> +++ b/drivers/net/usb/rtl8150.c
> @@ -71,6 +71,14 @@
>  #define MSR_SPEED		(1<<3)
>  #define MSR_LINK		(1<<2)
>  
> +/* USB endpoints */
> +enum rtl8150_usb_ep {
> +	RTL8150_USB_EP_CONTROL = 0,
> +	RTL8150_USB_EP_BULK_IN = 1,
> +	RTL8150_USB_EP_BULK_OUT = 2,
> +	RTL8150_USB_EP_INT_IN = 3,
> +};
> +
>  /* Interrupt pipe data */
>  #define INT_TSR			0x00
>  #define INT_RSR			0x01
> @@ -880,6 +888,20 @@ static int rtl8150_probe(struct usb_interface *intf,
>  		return -ENOMEM;
>  	}
>  
> +	/* Verify that all required endpoints are present */
> +	static const u8 bulk_ep_addr[] = {
> +		RTL8150_USB_EP_BULK_IN | USB_DIR_IN,
> +		RTL8150_USB_EP_BULK_OUT | USB_DIR_OUT,
> +		0};
> +	static const u8 int_ep_addr[] = {
> +		RTL8150_USB_EP_INT_IN | USB_DIR_IN,
> +		0};
> +	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
> +	    !usb_check_int_endpoints(intf, int_ep_addr)) {
> +		dev_err(&intf->dev, "couldn't find required endpoints\n");
> +		goto out;
> +	}
> +
>  	tasklet_setup(&dev->tl, rx_fixup);
>  	spin_lock_init(&dev->rx_pool_lock);
>  
> 

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Nikita Zhandarovich]

Hi,

On 1/22/25 04:43, Petko Manolov wrote:
> On 25-01-22 02:42:46, Nikita Zhandarovich wrote:
>> Syzkaller reports [1] encountering a common issue of utilizing a wrong usb
>> endpoint type during URB submitting stage. This, in turn, triggers a warning
>> shown below.
> 
> If these endpoints were of the wrong type the driver simply wouldn't work.
> 
> The proposed change in the patch doesn't do much in terms of fixing the issue
> (pipe 3 != type 1) and if usb_check_bulk_endpoints() fails, the driver will just
> not probe successfully.  I don't see how this is an improvement to the current
> situation.
> 
> We should either spend some time fixing the "BOGUS urb xfer, pipe 3 != type 1"
> for real or not touch anything.
> 
> 
> 		Petko
> 
> 

Thank you for your answer, I had a couple thoughts though.

If I understand correctly (which may not be the case, of course), since
the driver currently does not have any sanity checks for endpoints and
URBs' pipes are initialized essentially by fixed constants (as is often
the case), once again without any testing, then a virtual, weirdly
constructed device, like the one made up by Syzkaller, could provide
endpoints with contents that may cause that exact warning.

Real-life devices (with appropriate eps) would still work well and are
in no danger, with or without the patch. And even if that warning is
triggered, I am not certain the consequences are that severe, maybe on
kernels with 'panic_on_warn' set, and that's another conversation.
However, it seems that the change won't hurt either. Failing probe() in
such situations looks to be the standard.

If my approach is flawed, I'd really appreciate some hints on how you
would address that issue and I'd like to tackle it. I'd also ask if
other recipients could provide some of their views on the issue, even if
just to prove me wrong.

Regards,
Nikita

>> For now, enable simple endpoint checking (specifically, bulk and
>> interrupt eps, testing control one is not essential) to mitigate
>> the issue with a view to do other related cosmetic changes later,
>> if they are necessary.
>>
>> [1] Syzkaller report:
>> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
>> WARNING: CPU: 1 PID: 2586 at drivers/usb/core/urb.c:503 usb_submit_urb+0xe4b/0x1730 driv>
>> Modules linked in:
>> CPU: 1 UID: 0 PID: 2586 Comm: dhcpcd Not tainted 6.11.0-rc4-syzkaller-00069-gfc88bb11617>
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 08/06/2024
>> RIP: 0010:usb_submit_urb+0xe4b/0x1730 drivers/usb/core/urb.c:503
>> Code: 84 3c 02 00 00 e8 05 e4 fc fc 4c 89 ef e8 fd 25 d7 fe 45 89 e0 89 e9 4c 89 f2 48 8>
>> RSP: 0018:ffffc9000441f740 EFLAGS: 00010282
>> RAX: 0000000000000000 RBX: ffff888112487a00 RCX: ffffffff811a99a9
>> RDX: ffff88810df6ba80 RSI: ffffffff811a99b6 RDI: 0000000000000001
>> RBP: 0000000000000003 R08: 0000000000000001 R09: 0000000000000000
>> R10: 0000000000000000 R11: 0000000000000001 R12: 0000000000000001
>> R13: ffff8881023bf0a8 R14: ffff888112452a20 R15: ffff888112487a7c
>> FS:  00007fc04eea5740(0000) GS:ffff8881f6300000(0000) knlGS:0000000000000000
>> CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> CR2: 00007f0a1de9f870 CR3: 000000010dbd0000 CR4: 00000000003506f0
>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> Call Trace:
>>  <TASK>
>>  rtl8150_open+0x300/0xe30 drivers/net/usb/rtl8150.c:733
>>  __dev_open+0x2d4/0x4e0 net/core/dev.c:1474
>>  __dev_change_flags+0x561/0x720 net/core/dev.c:8838
>>  dev_change_flags+0x8f/0x160 net/core/dev.c:8910
>>  devinet_ioctl+0x127a/0x1f10 net/ipv4/devinet.c:1177
>>  inet_ioctl+0x3aa/0x3f0 net/ipv4/af_inet.c:1003
>>  sock_do_ioctl+0x116/0x280 net/socket.c:1222
>>  sock_ioctl+0x22e/0x6c0 net/socket.c:1341
>>  vfs_ioctl fs/ioctl.c:51 [inline]
>>  __do_sys_ioctl fs/ioctl.c:907 [inline]
>>  __se_sys_ioctl fs/ioctl.c:893 [inline]
>>  __x64_sys_ioctl+0x193/0x220 fs/ioctl.c:893
>>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>>  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
>> RIP: 0033:0x7fc04ef73d49
>> ...
>>
>> This change has not been tested on real hardware.
>>
>> Reported-and-tested-by: syzbot+d7e968426f644b567e31@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=d7e968426f644b567e31
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
>> ---
>>  drivers/net/usb/rtl8150.c | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>
>> diff --git a/drivers/net/usb/rtl8150.c b/drivers/net/usb/rtl8150.c
>> index 01a3b2417a54..f77af8cf543c 100644
>> --- a/drivers/net/usb/rtl8150.c
>> +++ b/drivers/net/usb/rtl8150.c
>> @@ -71,6 +71,14 @@
>>  #define MSR_SPEED		(1<<3)
>>  #define MSR_LINK		(1<<2)
>>  
>> +/* USB endpoints */
>> +enum rtl8150_usb_ep {
>> +	RTL8150_USB_EP_CONTROL = 0,
>> +	RTL8150_USB_EP_BULK_IN = 1,
>> +	RTL8150_USB_EP_BULK_OUT = 2,
>> +	RTL8150_USB_EP_INT_IN = 3,
>> +};
>> +
>>  /* Interrupt pipe data */
>>  #define INT_TSR			0x00
>>  #define INT_RSR			0x01
>> @@ -880,6 +888,20 @@ static int rtl8150_probe(struct usb_interface *intf,
>>  		return -ENOMEM;
>>  	}
>>  
>> +	/* Verify that all required endpoints are present */
>> +	static const u8 bulk_ep_addr[] = {
>> +		RTL8150_USB_EP_BULK_IN | USB_DIR_IN,
>> +		RTL8150_USB_EP_BULK_OUT | USB_DIR_OUT,
>> +		0};
>> +	static const u8 int_ep_addr[] = {
>> +		RTL8150_USB_EP_INT_IN | USB_DIR_IN,
>> +		0};
>> +	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
>> +	    !usb_check_int_endpoints(intf, int_ep_addr)) {
>> +		dev_err(&intf->dev, "couldn't find required endpoints\n");
>> +		goto out;
>> +	}
>> +
>>  	tasklet_setup(&dev->tl, rx_fixup);
>>  	spin_lock_init(&dev->rx_pool_lock);
>>  
>>

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Alan Stern]

On Wed, Jan 22, 2025 at 05:20:12AM -0800, Nikita Zhandarovich wrote:
> Hi,
> 
> On 1/22/25 04:43, Petko Manolov wrote:
> > On 25-01-22 02:42:46, Nikita Zhandarovich wrote:
> >> Syzkaller reports [1] encountering a common issue of utilizing a wrong usb
> >> endpoint type during URB submitting stage. This, in turn, triggers a warning
> >> shown below.
> > 
> > If these endpoints were of the wrong type the driver simply wouldn't work.

Better not to bind at all than to bind in a non-working way.  Especially 
when we can tell by a simple check that the device isn't what the driver 
expects.

> > The proposed change in the patch doesn't do much in terms of fixing the issue
> > (pipe 3 != type 1) and if usb_check_bulk_endpoints() fails, the driver will just
> > not probe successfully.  I don't see how this is an improvement to the current
> > situation.

It fixes the issue by preventing the driver from submitting an interrupt 
URB to a bulk endpoint or vice versa.

> > We should either spend some time fixing the "BOGUS urb xfer, pipe 3 != type 1"
> > for real or not touch anything.
> > 
> > 
> > 		Petko
> > 
> > 
> 
> Thank you for your answer, I had a couple thoughts though.
> 
> If I understand correctly (which may not be the case, of course), since
> the driver currently does not have any sanity checks for endpoints and
> URBs' pipes are initialized essentially by fixed constants (as is often
> the case), once again without any testing, then a virtual, weirdly
> constructed device, like the one made up by Syzkaller, could provide
> endpoints with contents that may cause that exact warning.
> 
> Real-life devices (with appropriate eps) would still work well and are
> in no danger, with or without the patch. And even if that warning is
> triggered, I am not certain the consequences are that severe, maybe on
> kernels with 'panic_on_warn' set, and that's another conversation.
> However, it seems that the change won't hurt either. Failing probe() in
> such situations looks to be the standard.
> 
> If my approach is flawed, I'd really appreciate some hints on how you
> would address that issue and I'd like to tackle it. I'd also ask if
> other recipients could provide some of their views on the issue, even if
> just to prove me wrong.

I agree with this approach; it seems like the best way to address this 
issue.

Alan Stern

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Petko Manolov]

On 25-01-22 10:59:33, Alan Stern wrote:
> On Wed, Jan 22, 2025 at 05:20:12AM -0800, Nikita Zhandarovich wrote:
> > Hi,
> > 
> > On 1/22/25 04:43, Petko Manolov wrote:
> > > On 25-01-22 02:42:46, Nikita Zhandarovich wrote:
> > >> Syzkaller reports [1] encountering a common issue of utilizing a wrong usb
> > >> endpoint type during URB submitting stage. This, in turn, triggers a warning
> > >> shown below.
> > > 
> > > If these endpoints were of the wrong type the driver simply wouldn't work.
> 
> Better not to bind at all than to bind in a non-working way.  Especially when
> we can tell by a simple check that the device isn't what the driver expects.
> 
> > > The proposed change in the patch doesn't do much in terms of fixing the
> > > issue (pipe 3 != type 1) and if usb_check_bulk_endpoints() fails, the
> > > driver will just not probe successfully.  I don't see how this is an
> > > improvement to the current situation.
> 
> It fixes the issue by preventing the driver from submitting an interrupt URB
> to a bulk endpoint or vice versa.

I always thought that once DID/VID is verified, there's no much room for that to
happen.

> > > We should either spend some time fixing the "BOGUS urb xfer, pipe 3 !=
> > > type 1" for real or not touch anything.
> > > 
> > > 
> > > 		Petko
> > > 
> > > 
> > 
> > Thank you for your answer, I had a couple thoughts though.
> > 
> > If I understand correctly (which may not be the case, of course), since the
> > driver currently does not have any sanity checks for endpoints and URBs'
> > pipes are initialized essentially by fixed constants (as is often the case),
> > once again without any testing, then a virtual, weirdly constructed device,
> > like the one made up by Syzkaller, could provide endpoints with contents
> > that may cause that exact warning.
> > 
> > Real-life devices (with appropriate eps) would still work well and are in no
> > danger, with or without the patch. And even if that warning is triggered, I
> > am not certain the consequences are that severe, maybe on kernels with
> > 'panic_on_warn' set, and that's another conversation. However, it seems that
> > the change won't hurt either. Failing probe() in such situations looks to be
> > the standard.
> > 
> > If my approach is flawed, I'd really appreciate some hints on how you would
> > address that issue and I'd like to tackle it. I'd also ask if other
> > recipients could provide some of their views on the issue, even if just to
> > prove me wrong.
> 
> I agree with this approach; it seems like the best way to address this issue.

Alright then.  I'd recommend following Fedor Pchelkin's advise about moving
those declarations to the beginning of probe(), though.


		Petko

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Alan Stern]

On Thu, Jan 23, 2025 at 11:49:30AM +0200, Petko Manolov wrote:
> On 25-01-22 10:59:33, Alan Stern wrote:
> > On Wed, Jan 22, 2025 at 05:20:12AM -0800, Nikita Zhandarovich wrote:
> > > Hi,
> > > 
> > > On 1/22/25 04:43, Petko Manolov wrote:
> > > > On 25-01-22 02:42:46, Nikita Zhandarovich wrote:
> > > >> Syzkaller reports [1] encountering a common issue of utilizing a wrong usb
> > > >> endpoint type during URB submitting stage. This, in turn, triggers a warning
> > > >> shown below.
> > > > 
> > > > If these endpoints were of the wrong type the driver simply wouldn't work.
> > 
> > Better not to bind at all than to bind in a non-working way.  Especially when
> > we can tell by a simple check that the device isn't what the driver expects.
> > 
> > > > The proposed change in the patch doesn't do much in terms of fixing the
> > > > issue (pipe 3 != type 1) and if usb_check_bulk_endpoints() fails, the
> > > > driver will just not probe successfully.  I don't see how this is an
> > > > improvement to the current situation.
> > 
> > It fixes the issue by preventing the driver from submitting an interrupt URB
> > to a bulk endpoint or vice versa.
> 
> I always thought that once DID/VID is verified, there's no much room for that to
> happen.

Unfortunately that's not so, for two reasons.  First, the vendor may 
change the device's design without updating the Product or Device ID, 
and second, a malicious device may spoof the VID, PID, and DID values.  
(Or, as in this case, a fuzzer may try to fool the driver.)

> Alright then.  I'd recommend following Fedor Pchelkin's advise about moving
> those declarations to the beginning of probe(), though.

Agreed.

Alan Stern

Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

[Fedor Pchelkin]

On Wed, 22. Jan 02:42, Nikita Zhandarovich wrote:
> @@ -880,6 +888,20 @@ static int rtl8150_probe(struct usb_interface *intf,
>  		return -ENOMEM;
>  	}
>  
> +	/* Verify that all required endpoints are present */
> +	static const u8 bulk_ep_addr[] = {
> +		RTL8150_USB_EP_BULK_IN | USB_DIR_IN,
> +		RTL8150_USB_EP_BULK_OUT | USB_DIR_OUT,
> +		0};
> +	static const u8 int_ep_addr[] = {
> +		RTL8150_USB_EP_INT_IN | USB_DIR_IN,
> +		0};

nit: It's better to avoid using mixed declarations and code, especially
if the patch is considered to be a material for stable branches.

When building old kernels which lack b5ec6fd286df ("kbuild: Drop
-Wdeclaration-after-statement"), the following unnecessary warning
occurs:

  drivers/net/usb/rtl8150.c: In function ‘rtl8150_probe’:
  drivers/net/usb/rtl8150.c:892:9: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    892 |         static const u8 bulk_ep_addr[] = {
        |         ^~~~~~


> +	if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
> +	    !usb_check_int_endpoints(intf, int_ep_addr)) {
> +		dev_err(&intf->dev, "couldn't find required endpoints\n");
> +		goto out;
> +	}
> +
>  	tasklet_setup(&dev->tl, rx_fixup);
>  	spin_lock_init(&dev->rx_pool_lock);
>