possible proble with skb_pull() in smsc75xx_rx_fixup()

[Oliver Neukum <oneukum@xxxxxxxx>]

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