* [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize @ 2019-09-18 12:17 Bjørn Mork 2019-09-21 2:03 ` Jakub Kicinski 0 siblings, 1 reply; 4+ messages in thread From: Bjørn Mork @ 2019-09-18 12:17 UTC (permalink / raw) To: netdev; +Cc: linux-usb, Oliver Neukum, Bjørn Mork Endpoints with zero wMaxPacketSize are not usable for transferring data. Ignore such endpoints when looking for valid in, out and status pipes, to make the drivers more robust against invalid and meaningless descriptors. The wMaxPacketSize of these endpoints are used for memory allocations and as divisors in many usbnet minidrivers. Avoiding zero is therefore critical. Signed-off-by: Bjørn Mork <bjorn@mork.no> --- drivers/net/usb/usbnet.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 58952a79b05f..dbea2136d901 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -100,6 +100,11 @@ int usbnet_get_endpoints(struct usbnet *dev, struct usb_interface *intf) int intr = 0; e = alt->endpoint + ep; + + /* ignore endpoints which cannot transfer data */ + if (!usb_endpoint_maxp(&e->desc)) + continue; + switch (e->desc.bmAttributes) { case USB_ENDPOINT_XFER_INT: if (!usb_endpoint_dir_in(&e->desc)) -- 2.20.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize 2019-09-18 12:17 [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize Bjørn Mork @ 2019-09-21 2:03 ` Jakub Kicinski 2019-09-21 12:54 ` Bjørn Mork 0 siblings, 1 reply; 4+ messages in thread From: Jakub Kicinski @ 2019-09-21 2:03 UTC (permalink / raw) To: Bjørn Mork; +Cc: netdev, linux-usb, Oliver Neukum On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote: > Endpoints with zero wMaxPacketSize are not usable for transferring > data. Ignore such endpoints when looking for valid in, out and > status pipes, to make the drivers more robust against invalid and > meaningless descriptors. > > The wMaxPacketSize of these endpoints are used for memory allocations > and as divisors in many usbnet minidrivers. Avoiding zero is therefore > critical. > > Signed-off-by: Bjørn Mork <bjorn@mork.no> Fixes tag would be useful. I'm not sure how far into stable we should backport this. Is this something that occurs on real devices or protection from malicious ones? ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize 2019-09-21 2:03 ` Jakub Kicinski @ 2019-09-21 12:54 ` Bjørn Mork 2019-09-21 19:30 ` Jakub Kicinski 0 siblings, 1 reply; 4+ messages in thread From: Bjørn Mork @ 2019-09-21 12:54 UTC (permalink / raw) To: Jakub Kicinski; +Cc: netdev, linux-usb, Oliver Neukum Jakub Kicinski <jakub.kicinski@netronome.com> writes: > On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote: >> Endpoints with zero wMaxPacketSize are not usable for transferring >> data. Ignore such endpoints when looking for valid in, out and >> status pipes, to make the drivers more robust against invalid and >> meaningless descriptors. >> >> The wMaxPacketSize of these endpoints are used for memory allocations >> and as divisors in many usbnet minidrivers. Avoiding zero is therefore >> critical. >> >> Signed-off-by: Bjørn Mork <bjorn@mork.no> > > Fixes tag would be useful. I'm not sure how far into stable we should > backport this. That would be commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so I don't think a Fixes tag is very useful... I haven't verified how deep into the code you have been able to get with wMaxPacketSize being zero. But I don't think there ever has been much protection since it's so obviously "insane". There was no point in protecting against this as long as we considered the USB port a security barrier. I see that the v2.6.12-rc2 version of drivers/usb/net/usbnet.c (sic) already had this in it's genelink_tx_fixup(): ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1984) // add padding byte ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1985) if ((skb->len % dev->maxpacket) == 0) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1986) skb_put (skb, 1); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1987) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1988) return skb; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1989) } And this in usbnet_start_xmit(): ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3564) /* don't assume the hardware handles USB_ZERO_PACKET ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3565) * NOTE: strictly conforming cdc-ether devices should expect ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3566) * the ZLP here, but ignore the one-byte packet. ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3567) * ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3568) * FIXME zero that byte, if it doesn't require a new skb. ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3569) */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3570) if ((length % dev->maxpacket) == 0) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3571) urb->transfer_buffer_length++; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3572) usbnet_probe() calculated dev->maxpacket as ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 3826) dev->maxpacket = usb_maxpacket (dev->udev, dev->out, 1); without any sanity checking. And usb_maxpacket() hasn't changed much. It was pretty much the same then as now: ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1123) usb_maxpacket(struct usb_device *udev, int pipe, int is_out) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1124) { ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1125) struct usb_host_endpoint *ep; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1126) unsigned epnum = usb_pipeendpoint(pipe); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1127) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1128) if (is_out) { ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1129) WARN_ON(usb_pipein(pipe)); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1130) ep = udev->ep_out[epnum]; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1131) } else { ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1132) WARN_ON(usb_pipeout(pipe)); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1133) ep = udev->ep_in[epnum]; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1134) } ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1135) if (!ep) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1136) return 0; ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1137) ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1138) /* NOTE: only 0x07ff bits are for packet size... */ ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1139) return le16_to_cpu(ep->desc.wMaxPacketSize); ^1da177e4c3f4 (Linus Torvalds 2005-04-16 15:20:36 -0700 1140) } So, to summarize: I believe the fix is valid for all stable versions. I'll leave it up to the more competent stable maintainers to decide how many, if any, it should be backported to. I will not cry if the answer is none. > Is this something that occurs on real devices or protection from > malicious ones? Only malicious ones AFAICS. I don't necessarily agree, but I believe the current policy makes this a "security" issue. CVEs have previously been allocated for similar crashes triggered by buggy USB descriptors. For some reason we are supposed to protect the system against *some* types of malicious hardware. I am looking forward to the fixes coming up next to protect against malicious CPUs and microcode ;-) Bjørn ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize 2019-09-21 12:54 ` Bjørn Mork @ 2019-09-21 19:30 ` Jakub Kicinski 0 siblings, 0 replies; 4+ messages in thread From: Jakub Kicinski @ 2019-09-21 19:30 UTC (permalink / raw) To: Bjørn Mork; +Cc: netdev, linux-usb, Oliver Neukum On Sat, 21 Sep 2019 14:54:03 +0200, Bjørn Mork wrote: > Jakub Kicinski <jakub.kicinski@netronome.com> writes: > > On Wed, 18 Sep 2019 14:17:38 +0200, Bjørn Mork wrote: > >> Endpoints with zero wMaxPacketSize are not usable for transferring > >> data. Ignore such endpoints when looking for valid in, out and > >> status pipes, to make the drivers more robust against invalid and > >> meaningless descriptors. > >> > >> The wMaxPacketSize of these endpoints are used for memory allocations > >> and as divisors in many usbnet minidrivers. Avoiding zero is therefore > >> critical. > >> > >> Signed-off-by: Bjørn Mork <bjorn@mork.no> > > > > Fixes tag would be useful. I'm not sure how far into stable we should > > backport this. > > That would be commit 1da177e4c3f4 ("Linux-2.6.12-rc2"), so I don't think > a Fixes tag is very useful... It's slightly useful to add it anyway, IMHO, even if it's 2.6.12, because it may save people processing the patch checking how far it applies. You already did the research, anyway. Granted, that's a little `process-centric`, rather than `merit-centric` view. > I haven't verified how deep into the code you have been able to get with > wMaxPacketSize being zero. But I don't think there ever has been much > protection since it's so obviously "insane". There was no point in > protecting against this as long as we considered the USB port a security > barrier. > > I see that the v2.6.12-rc2 version of drivers/usb/net/usbnet.c (sic) > already had this in it's genelink_tx_fixup(): <snip> Thanks for the detailed analysis! > So, to summarize: I believe the fix is valid for all stable versions. > > I'll leave it up to the more competent stable maintainers to decide how > many, if any, it should be backported to. I will not cry if the answer > is none. Right, I'll put it in the stable queue, we'll see if it passes Dave's and Greg's filters :) > > Is this something that occurs on real devices or protection from > > malicious ones? > > Only malicious ones AFAICS. > > I don't necessarily agree, but I believe the current policy makes this a > "security" issue. CVEs have previously been allocated for similar > crashes triggered by buggy USB descriptors. For some reason we are > supposed to protect the system against *some* types of malicious > hardware. I see, the patch is fairly intrusive and very unlikely to cause to lead to regressions on real devices, so regardless of the practical impact shouldn't hurt. > I am looking forward to the fixes coming up next to protect against > malicious CPUs and microcode ;-) I hope not before we retire.. Applied, queued for stable, thank you! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-09-21 19:30 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-09-18 12:17 [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize Bjørn Mork 2019-09-21 2:03 ` Jakub Kicinski 2019-09-21 12:54 ` Bjørn Mork 2019-09-21 19:30 ` Jakub Kicinski
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).