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@xxxxxxxx> 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
>
