Re: [Qemu-devel] [PATCH v2 1/2] net: check packet payload length

[P J P]

  Hello Jason,

+-- On Fri, 26 Feb 2016, Jason Wang wrote --+
| Should we count mac header here? Did "plen + hlen >= length" imply "14 +
| hlen + csum_offset + 1" < length?
| 
| Looks not. Consider a TCP packet can report evil plen (e.g 20) but just
| have 10 bytes payload in fact. In this case:
| 
| hlen = 20, plen = 20, csum_offset = 16, length = 44 which can pass all
| the above tests, but 14 + hlen + csum_offset = 50 which is greater than
| length.

  Ummn, not sure. hlen + plen = total length(IP + TCP/UDP), 20+20 != 44. 
'length' is also used to read and allocate requisite buffers in other parts 
from where net_checksum_calculate() is called,

===
   etsec->tx_buffer = g_realloc(etsec->tx_buffer,
                                 etsec->tx_buffer_len + bd->length);
   ...
   /* Update buffer length */
   etsec->tx_buffer_len += bd->length;

   process_tx_fcb(etsec);
     -> net_checksum_calculate(etsec->tx_buffer + 8,
                                etsec->tx_buffer_len - 8);

   OR

   memcpy(tmpbuf, page + txreq.offset, txreq.size);
   net_checksum_calculate(tmpbuf, txreq.size);
===

  - With 'length < 14 + 20', we ensure that L2 & L3 headers are complete.
    44 - 34 = 10, TCP/UDP header is incomplete.

I think 'plen=20 != 10' hints at an error in other parts of the code? Also if 
data buffer has more space than actual data, OOB access may not occur.

--
Prasad J Pandit / Red Hat Product Security Team
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F