From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYPkA-00029S-HE for qemu-devel@nongnu.org; Tue, 23 Feb 2016 22:04:03 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aYPk7-0003mP-9M for qemu-devel@nongnu.org; Tue, 23 Feb 2016 22:04:02 -0500 Received: from mx1.redhat.com ([209.132.183.28]:51190) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aYPk7-0003mL-3s for qemu-devel@nongnu.org; Tue, 23 Feb 2016 22:03:59 -0500 References: <1455710904-13368-1-git-send-email-ppandit@redhat.com> <878u2jh4uz.fsf@blackfin.pond.sub.org> <87povujrf8.fsf@blackfin.pond.sub.org> <56CC10E5.6010902@redhat.com> From: Jason Wang Message-ID: <56CD1D93.2010708@redhat.com> Date: Wed, 24 Feb 2016 11:03:47 +0800 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] net: check packet payload length List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: P J P Cc: Qemu Developers , Markus Armbruster , Liu Ling 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 >