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

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: <87h855g68k.fsf@miraculix.mork.no>
Date:   Sat, 21 Sep 2019 14:54:03 +0200
From:   Bjørn Mork <bjorn@...k.no>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, linux-usb@...r.kernel.org,
        Oliver Neukum <oliver@...kum.org>
Subject: Re: [PATCH net,stable] usbnet: ignore endpoints with invalid wMaxPacketSize

Jakub Kicinski <jakub.kicinski@...ronome.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@...k.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