netdev - possible proble with skb_pull() in smsc75xx_rx

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-next>] [day] [month] [year] [list]

Date: Wed, 15 Nov 2023 12:45:53 +0100
From: Oliver Neukum <oneukum@...e.com>
To: Szymon Heidrich <szymon.heidrich@...il.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 USB list <linux-usb@...r.kernel.org>
Subject: possible proble with skb_pull() in smsc75xx_rx_fixup()

Hi,

looking at your security fixes, it seems to me that there
is a further issue they do not cover.

If we look at this:

         while (skb->len > 0) {

len is positive ...

                 u32 rx_cmd_a, rx_cmd_b, align_count, size;
                 struct sk_buff *ax_skb;
                 unsigned char *packet;

                 rx_cmd_a = get_unaligned_le32(skb->data);
                 skb_pull(skb, 4);

... but it may be smaller than 4
If that happens skb_pull() is a nop.

                 rx_cmd_b = get_unaligned_le32(skb->data);
                 skb_pull(skb, 4 + RXW_PADDING);

Then this is a nop, too.
That means that rx_cmd_a and rx_cmd_b are identical and garbage.

                 packet = skb->data;

                 /* get the packet length */
                 size = (rx_cmd_a & RX_CMD_A_LEN) - RXW_PADDING;

In that case size is unpredictable.

                 align_count = (4 - ((size + RXW_PADDING) % 4)) % 4;

                 if (unlikely(size > skb->len)) {

That means that this check may or may not work.

                         netif_dbg(dev, rx_err, dev->net,
                                   "size err rx_cmd_a=0x%08x\n",
                                   rx_cmd_a);
                         return 0;
                 }

There is also an error case where 4 <= skb->len < 4 + RXW_PADDING

I think we really need to check for the amount of buffer that is pulled.
Something like this:

  static int smsc75xx_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
  {
+       u32 rx_cmd_a, rx_cmd_b, align_count, size;
+
         /* This check is no longer done by usbnet */
         if (skb->len < dev->net->hard_header_len)
                 return 0;
  
-       while (skb->len > 0) {
-               u32 rx_cmd_a, rx_cmd_b, align_count, size;
+       while (skb->len > (sizeof(rx_cmd_a) + sizeof(rx_cmd_b) + RXW_PADDING)) {

	Regards
		Oliver