[PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[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

Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[Jakub Kicinski]

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?

Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[Bjørn Mork]

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

Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

[Jakub Kicinski]

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!