Re: [Qemu-devel] [PATCH] net: check packet payload length

[Jason Wang <jasowang@redhat.com>]

On 02/23/2016 07:11 PM, P J P wrote:
>   Hello Jason,
>
> +-- On Tue, 23 Feb 2016, Jason Wang wrote --+
> | Let's avoid adding assert() here since it could be triggered by guest.
>
>   Okay.
>
> | I think you need audit all the callers to see if the issue mentioned by
> | Markus existed first.
>
>   Yes, I did. As mentioned earlier, some have check for minimum packet size, 
> others check for length > 0.
>
> net_checksum_calculate is called as:
>
>     -  hw/net/cadence_gem.c
>          if (tx_desc_get_length(desc) == 0))
>             DB_PRINT("Invalid TX descriptor @ 0x%x\n",
>          }
>          total_bytes += tx_desc_get_length(desc);
>            net_checksum_calculate(tx_packet, total_bytes);
>
>     -  hw/net/fsl_etsec/rings.c
>          if (etsec->tx_buffer_len != 0
>              net_checksum_calculate(etsec->tx_buffer + 8,
>                                       etsec->tx_buffer_len - 8);
>
>     -  hw/net/virtio-net.c
>          if (size > 27 && size < 1500) &&
>          (buf[34] == 0 && buf[35] == 67)) {

Are we sure size of buf is >= 35 here?

>              net_checksum_calculate(buf, size);
>
>     -  hw/net/xen_nic.c
>          if (txreq.size < 14) {                                              
>              xen_be_printf(&netdev->xendev, 0, "bad packet size:
>          }
>          net_checksum_calculate(tmpbuf, txreq.size);
>   
> It might be possible to send a packet with (length < 14+20), and cause an OOB 
> read in net_checksum_calculate,
>
>     if ((data[14] & 0xf0) != 0x40)
>         return; /* not IPv4 */
>     hlen  = (data[14] & 0x0f) * 4;
>     plen  = (data[16] << 8 | data[17]) - hlen;
>     proto = data[23];
>
> I'll send a revised patch with a check for minimum 'data' length to include 
> complete layer-2(14) + layer-3(20) headers.
>
> +    if (len < 14+20)
> +        return;
>
>
> Thank you.

Ok.

> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>