Jakub Kicinski <jakub.kicinski@xxxxxxxxxxxxx> 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@xxxxxxx>
>
> 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
