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

[Jason Wang <jasowang@redhat.com>]

On 02/18/2016 06:33 PM, P J P wrote:
>   Hello Markus,
>
> +-- On Thu, 18 Feb 2016, Markus Armbruster wrote --+
> | 
> |           if ((data[14] & 0xf0) != 0x40)
> | Buffer overrun when length <= 14.
> | 
> |           proto = data[23];
> | Buffer overrun when length <= 23.
> | 
> | I think we should check that we got at least an IPv4 header without
> | options (length >= 14 + 20) before accessing said header.
>
>   Right. Some callers do have checks in place to ensure length is >= minium 
> packet length. Still it'll help to add an assert(length >= 14+20);

Let's avoid adding assert() here since it could be triggered by guest.

>
> |     /*
> |      * Compute and store checksum of IPv4 TCP or UDP packet.
> |      * @data holds @length bytes.  It must be a complete packet.
> |      * If this is an IPv4 TCP packet, compute its checksum and store it
> |      * in the TCP header.
> |      * Else if this is a complete IPv4 UDP packet, compute its checksum
> |      * and store it in the UDP header.
> |      * Else do nothing.
> |      */
> |     void net_checksum_calculate(uint8_t *data, int length)
> | 
> | I find it simpler.
> | 
> | If the other buffer overrun I mentioned above needs fixing: yes.
> | Else: depends on the maintainer.
>
>   Okay, I'll wait for Jason's inputs and will send a revised patch including 
> your suggestions above.
>
> Thank you.
> --
> Prasad J Pandit / Red Hat Product Security Team
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

I think you need audit all the callers to see if the issue mentioned by
Markus existed first.