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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250122124359.GA9183@bender.k.g>
Date: Wed, 22 Jan 2025 14:43:59 +0200
From: Petko Manolov <petkan@...leusys.com>
To: Nikita Zhandarovich <n.zhandarovich@...tech.ru>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	linux-usb@...r.kernel.org, netdev@...r.kernel.org,
	syzbot+d7e968426f644b567e31@...kaller.appspotmail.com,
	syzkaller-bugs@...glegroups.com, linux-kernel@...r.kernel.org,
	lvc-project@...uxtesting.org
Subject: Re: [PATCH net] net: usb: rtl8150: enable basic endpoint checking

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@...kaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=d7e968426f644b567e31
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Cc: stable@...r.kernel.org
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@...tech.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);
>  
>