From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42191) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aJxiy-0001dB-2W for qemu-devel@nongnu.org; Fri, 15 Jan 2016 01:19:04 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aJxiw-00087i-23 for qemu-devel@nongnu.org; Fri, 15 Jan 2016 01:19:04 -0500 MIME-Version: 1.0 In-Reply-To: References: <1452764448-17953-1-git-send-email-mst@redhat.com> Date: Thu, 14 Jan 2016 22:19:01 -0800 Message-ID: From: Peter Crosthwaite Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: "Michael S. Tsirkin" , Jason Wang , QEMU Developers , Prasad Pandit , qemu-arm , =?UTF-8?B?5YiY5Luk?= , Alistair Francis On Thu, Jan 14, 2016 at 2:03 AM, Peter Maydell w= rote: > On 14 January 2016 at 09:43, Michael S. Tsirkin wrote: >> gem_receive copies a packet received from network into an rxbuf[2048] >> array on stack, with size limited by descriptor length set by guest. If >> guest is malicious and specifies a descriptor length that is too large, >> and should packet size exceed array size, this results in a buffer >> overflow. >> >> Reported-by: =E5=88=98=E4=BB=A4 >> Signed-off-by: Michael S. Tsirkin >> --- >> hw/net/cadence_gem.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/hw/net/cadence_gem.c b/hw/net/cadence_gem.c >> index 3639fc1..15a0786 100644 >> --- a/hw/net/cadence_gem.c >> +++ b/hw/net/cadence_gem.c >> @@ -862,6 +862,14 @@ static void gem_transmit(CadenceGEMState *s) >> break; >> } >> >> + if (tx_desc_get_length(desc) > sizeof(tx_packet) - (p - tx_pack= et)) { >> + DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0= x%x\n", >> + (unsigned)packet_desc_addr, >> + (unsigned)tx_desc_get_length(desc), >> + sizeof(tx_packet) - (p - tx_packet)); >> + break; >> + } > > Is this what the real hardware does in this situation? > Should we log this as a guest error? > I'm not sure it is a guest error. I think its just a shortcut in the original implementation. I guess QEMU needs the whole packet before handing off to the net layer and the assumption is that the packet is always within 2048. I think the hardware is just going to put the data on the wire as it goes. The easiest solution is to realloc the buffer as it goes with the increasing sizes. Otherwise you could refactor the code to be two pass over the descriptor ring section (containing the packet). If we want to fix the buffer overflow more urgently, the correct error would be an assert(). Regards, Peter >> + >> /* Gather this fragment of the packet from "dma memory" to our = contig. >> * buffer. >> */ >> -- >> MST >> > > thanks > -- PMM >