Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On 01/18/2016 03:04 PM, Peter Crosthwaite wrote: > On Sun, Jan 17, 2016 at 10:50 PM, Jason Wangwrote: >> >> On 01/14/2016 05:43 PM, 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: 刘令 >>> Signed-off-by: Michael S. Tsirkin >>> --- >>> hw/net/cadence_gem.c | 8 >>> 1 file changed, 8 insertions(+) >> Apply to my -net with tweak on commit log (changing receive to transmit >> as noticed). >> > As this is actually an unimplemented feature you should change the > message to a LOG_UNIMP rather than a debug printf. > > Regards, > Peter Thanks for the reminding. But we need know the whether real device could send packet whose length is greater than 2048. Do you know the link to the manual? (Haven't fond it in cadence page.) A hint is the linux driver (drivers/net/ethernet/cadence/macb.c), use 1500 as its MTU. > >> Thanks >> >>> 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_packet)) { >>> +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space >>> 0x%x\n", >>> + (unsigned)packet_desc_addr, >>> + (unsigned)tx_desc_get_length(desc), >>> + sizeof(tx_packet) - (p - tx_packet)); >>> +break; >>> +} >>> + >>> /* Gather this fragment of the packet from "dma memory" to our >>> contig. >>> * buffer. >>> */
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On Mon, Jan 18, 2016 at 12:12 AM, Jason Wangwrote: > > > On 01/18/2016 03:04 PM, Peter Crosthwaite wrote: >> On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang wrote: >>> >>> On 01/14/2016 05:43 PM, 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: 刘令 Signed-off-by: Michael S. Tsirkin --- hw/net/cadence_gem.c | 8 1 file changed, 8 insertions(+) >>> Apply to my -net with tweak on commit log (changing receive to transmit >>> as noticed). >>> >> As this is actually an unimplemented feature you should change the >> message to a LOG_UNIMP rather than a debug printf. >> >> Regards, >> Peter > > Thanks for the reminding. But we need know the whether real device could > send packet whose length is greater than 2048. Do you know the link to > the manual? (Haven't fond it in cadence page.) A hint is the linux Xilinx UG585 has details: http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf Regards, Peter > driver (drivers/net/ethernet/cadence/macb.c), use 1500 as its MTU. > >> >>> Thanks >>> 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_packet)) { +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0x%x\n", + (unsigned)packet_desc_addr, + (unsigned)tx_desc_get_length(desc), + sizeof(tx_packet) - (p - tx_packet)); +break; +} + /* Gather this fragment of the packet from "dma memory" to our contig. * buffer. */ >
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On 01/18/2016 05:08 PM, Peter Crosthwaite wrote: > On Mon, Jan 18, 2016 at 12:12 AM, Jason Wangwrote: >> >> On 01/18/2016 03:04 PM, Peter Crosthwaite wrote: >>> On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang wrote: On 01/14/2016 05:43 PM, 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: 刘令 > Signed-off-by: Michael S. Tsirkin > --- > hw/net/cadence_gem.c | 8 > 1 file changed, 8 insertions(+) Apply to my -net with tweak on commit log (changing receive to transmit as noticed). >>> As this is actually an unimplemented feature you should change the >>> message to a LOG_UNIMP rather than a debug printf. >>> >>> Regards, >>> Peter >> Thanks for the reminding. But we need know the whether real device could >> send packet whose length is greater than 2048. Do you know the link to >> the manual? (Haven't fond it in cadence page.) A hint is the linux > Xilinx UG585 has details: > > http://www.xilinx.com/support/documentation/user_guides/ug585-Zynq-7000-TRM.pdf > > Regards, > Peter > > Thanks for the pointer. In section 16.1.5, it said "Jumbo frames are not supported." So it was in fact not an unimplemented feature?
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On Sun, Jan 17, 2016 at 10:50 PM, Jason Wangwrote: > > > On 01/14/2016 05:43 PM, 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: 刘令 >> Signed-off-by: Michael S. Tsirkin >> --- >> hw/net/cadence_gem.c | 8 >> 1 file changed, 8 insertions(+) > > Apply to my -net with tweak on commit log (changing receive to transmit > as noticed). > As this is actually an unimplemented feature you should change the message to a LOG_UNIMP rather than a debug printf. Regards, Peter > Thanks > >> >> 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_packet)) >> { >> +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space >> 0x%x\n", >> + (unsigned)packet_desc_addr, >> + (unsigned)tx_desc_get_length(desc), >> + sizeof(tx_packet) - (p - tx_packet)); >> +break; >> +} >> + >> /* Gather this fragment of the packet from "dma memory" to our >> contig. >> * buffer. >> */ >
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On 01/14/2016 05:43 PM, 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: 刘令> Signed-off-by: Michael S. Tsirkin > --- > hw/net/cadence_gem.c | 8 > 1 file changed, 8 insertions(+) Apply to my -net with tweak on commit log (changing receive to transmit as noticed). Thanks > > 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_packet)) { > +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space > 0x%x\n", > + (unsigned)packet_desc_addr, > + (unsigned)tx_desc_get_length(desc), > + sizeof(tx_packet) - (p - tx_packet)); > +break; > +} > + > /* Gather this fragment of the packet from "dma memory" to our > contig. > * buffer. > */
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
+-- On Thu, 14 Jan 2016, 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. | | 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_packet)) { | +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0x%x\n", | + (unsigned)packet_desc_addr, | + (unsigned)tx_desc_get_length(desc), | + sizeof(tx_packet) - (p - tx_packet)); | +break; | +} | + Commit message says gem_receive, but the patch fixes gem_transmit() routine. -- - P J P 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
[Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
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: 刘令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_packet)) { +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space 0x%x\n", + (unsigned)packet_desc_addr, + (unsigned)tx_desc_get_length(desc), + sizeof(tx_packet) - (p - tx_packet)); +break; +} + /* Gather this fragment of the packet from "dma memory" to our contig. * buffer. */ -- MST
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
On 01/14/2016 05:43 PM, 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: 刘令> 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_packet)) { > +DB_PRINT("TX descriptor @ 0x%x too large: size 0x%x space > 0x%x\n", > + (unsigned)packet_desc_addr, > + (unsigned)tx_desc_get_length(desc), > + sizeof(tx_packet) - (p - tx_packet)); > +break; > +} > + > /* Gather this fragment of the packet from "dma memory" to our > contig. > * buffer. > */ Looks like we need similar issue in gen_receive(), need to fix that?
Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow
+-- On Fri, 15 Jan 2016, Jason Wang wrote --+ | Looks like we need similar issue in gen_receive(), need to fix that? Yes, I'm preparing a patch. -- Prasad J Pandit / Red Hat Product Security Team 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F