netdev - Re: 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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <EB9ACA9B-78ED-48C3-99D6-86E886557FBC@gmail.com>
Date: Thu, 16 Nov 2023 21:09:35 +0100
From: Szymon Heidrich <szymon.heidrich@...il.com>
To: Oliver Neukum <oneukum@...e.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
 USB list <linux-usb@...r.kernel.org>
Subject: Re: possible proble with skb_pull() in smsc75xx_rx_fixup()

Hello Oliver,

Could you please give me some hints how this could be practically exploited to cause mischief?

Best regards,
Szymon


> Wiadomość napisana przez Oliver Neukum <oneukum@...e.com> w dniu 15.11.2023, o godz. 12:45:
> 
> 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
>