[Qemu-devel] [PATCH] cadence_gem: fix buffer overflow

[Qemu-devel] [PATCH] cadence_gem: fix buffer overflow

[Michael S. Tsirkin]

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: 刘令 <liuling-it@360.cn>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 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] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Peter Maydell]

On 14 January 2016 at 09:43, Michael S. Tsirkin <mst@redhat.com> 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: 刘令 <liuling-it@360.cn>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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;
> +        }

Is this what the real hardware does in this situation?
Should we log this as a guest error?

> +
>          /* Gather this fragment of the packet from "dma memory" to our contig.
>           * buffer.
>           */
> --
> MST
>

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Michael S. Tsirkin]

On Thu, Jan 14, 2016 at 10:03:15AM +0000, Peter Maydell wrote:
> On 14 January 2016 at 09:43, Michael S. Tsirkin <mst@redhat.com> 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: 刘令 <liuling-it@360.cn>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >  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;
> > +        }
> 
> Is this what the real hardware does in this situation?
> Should we log this as a guest error?

I don't really know.  This is just consistent with what this device does
for other descriptor errors.

> > +
> >          /* Gather this fragment of the packet from "dma memory" to our contig.
> >           * buffer.
> >           */
> > --
> > MST
> >
> 
> thanks
> -- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Peter Crosthwaite]

On Thu, Jan 14, 2016 at 2:03 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 14 January 2016 at 09:43, Michael S. Tsirkin <mst@redhat.com> 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: 刘令 <liuling-it@360.cn>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  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;
>> +        }
>
> 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
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[P J P]

+-- On Thu, 14 Jan 2016, Peter Crosthwaite wrote --+
| 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. The easiest 
| solution is to realloc the buffer as it goes with the increasing sizes.

  Yes, I was considering increasing buffer size with view of the jumbo 
packets. In gem_transmit(), 'tx_desc_get_length' returns length masked by 
DESC_1_LENGTH(=0x1FFF=8191) bytes. But thought dynamic allocation might lead 
to excessive buffer allocation, not sure how constrained the Xilinx platform 
is for memory.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Alistair Francis]

On Fri, Jan 15, 2016 at 12:06 AM, P J P <ppandit@redhat.com> wrote:
> +-- On Thu, 14 Jan 2016, Peter Crosthwaite wrote --+
> | 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. The easiest
> | solution is to realloc the buffer as it goes with the increasing sizes.
>
>   Yes, I was considering increasing buffer size with view of the jumbo
> packets. In gem_transmit(), 'tx_desc_get_length' returns length masked by
> DESC_1_LENGTH(=0x1FFF=8191) bytes. But thought dynamic allocation might lead
> to excessive buffer allocation, not sure how constrained the Xilinx platform
> is for memory.

Won't the allocation/reallocation happen on the host?

Thanks,

Alistair

>
> --
>  - P J P
> 47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[P J P]

+-- On Fri, 15 Jan 2016, Alistair Francis wrote --+
| Won't the allocation/reallocation happen on the host?

  Ah yes, don't know what I was thinking.

--
 - P J P
47AF CE69 3A90 54AA 9045 1053 DD13 3D32 FE5B 041F

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Jason Wang]

On 01/15/2016 02:19 PM, Peter Crosthwaite wrote:
> On Thu, Jan 14, 2016 at 2:03 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 14 January 2016 at 09:43, Michael S. Tsirkin <mst@redhat.com> 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: 刘令 <liuling-it@360.cn>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  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;
>>> +        }
>> 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.

If we are not sure this is what real hardware did, dropping looks more
safe than sending the truncated packets on the wire.

>  The easiest solution is to realloc the buffer
> as it goes with the increasing sizes.

This could allow possible DOS from guest (see
cde31a0e3dc0e4ac83e454d6096350cec584adf1).

> 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

Let's avoid putting guest trigger-able assert() here. The patch looks
good for fixing the issue. Refactoring could be done on top.

Thanks

>
>>> +
>>>          /* Gather this fragment of the packet from "dma memory" to our contig.
>>>           * buffer.
>>>           */
>>> --
>>> MST
>>>
>> thanks
>> -- PMM
>>

Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow

[P J P]

+-- 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

Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow

[Jason Wang]

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: 刘令 <liuling-it@360.cn>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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

[P J P]

+-- 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

Re: [Qemu-devel] [PATCH] cadence_gem: fix buffer overflow

[Jason Wang]

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: 刘令 <liuling-it@360.cn>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  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

[Peter Crosthwaite]

On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang <jasowang@redhat.com> 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: 刘令 <liuling-it@360.cn>
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>> ---
>>  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

[Jason Wang]

On 01/18/2016 03:04 PM, Peter Crosthwaite wrote:
> On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang <jasowang@redhat.com> 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: 刘令 <liuling-it@360.cn>
>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>> ---
>>>  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

[Peter Crosthwaite]

On Mon, Jan 18, 2016 at 12:12 AM, Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 01/18/2016 03:04 PM, Peter Crosthwaite wrote:
>> On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang <jasowang@redhat.com> 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: 刘令 <liuling-it@360.cn>
>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>> ---
>>>>  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

[Jason Wang]

On 01/18/2016 05:08 PM, Peter Crosthwaite wrote:
> On Mon, Jan 18, 2016 at 12:12 AM, Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 01/18/2016 03:04 PM, Peter Crosthwaite wrote:
>>> On Sun, Jan 17, 2016 at 10:50 PM, Jason Wang <jasowang@redhat.com> 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: 刘令 <liuling-it@360.cn>
>>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>>>>> ---
>>>>>  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] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Peter Maydell]

On 18 January 2016 at 09:57, Jason Wang <jasowang@redhat.com> wrote:
> 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?

I have a vague feeling from the last time I looked at something like
this that what often happens is the hardware happily goes on dumping
data out on the wire but this is a violation of the ethernet protocol,
ie a guest error. But I'm no ethernet expert...

thanks
-- PMM

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Alistair Francis]

On Mon, Jan 18, 2016 at 2:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 18 January 2016 at 09:57, Jason Wang <jasowang@redhat.com> wrote:
>> 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?

I'd say this should be a guest error then. Unless anyone who knows
Ethernet well thinks otherwise?

Thanks,

Alistair

>
> I have a vague feeling from the last time I looked at something like
> this that what often happens is the hardware happily goes on dumping
> data out on the wire but this is a violation of the ethernet protocol,
> ie a guest error. But I'm no ethernet expert...
>
> thanks
> -- PMM
>

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Jason Wang]

On 01/19/2016 12:54 AM, Alistair Francis wrote:
> On Mon, Jan 18, 2016 at 2:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> On 18 January 2016 at 09:57, Jason Wang <jasowang@redhat.com> wrote:
>>> 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?
> I'd say this should be a guest error then. Unless anyone who knows
> Ethernet well thinks otherwise?
>
> Thanks,
>
> Alistair

Yes, agree.

>
>> I have a vague feeling from the last time I looked at something like
>> this that what often happens is the hardware happily goes on dumping
>> data out on the wire but this is a violation of the ethernet protocol,
>> ie a guest error. But I'm no ethernet expert...
>>
>> thanks
>> -- PMM
>>

Re: [Qemu-devel] [Qemu-arm] [PATCH] cadence_gem: fix buffer overflow

[Jason Wang]

On 01/18/2016 06:06 PM, Peter Maydell wrote:
> On 18 January 2016 at 09:57, Jason Wang <jasowang@redhat.com> wrote:
>> 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?
> I have a vague feeling from the last time I looked at something like
> this that what often happens is the hardware happily goes on dumping
> data out on the wire but this is a violation of the ethernet protocol,
> ie a guest error. But I'm no ethernet expert...
>
> thanks
> -- PMM
>

I think it's a guest error (unless we find a guest/driver that depends
on this). Anyway, fixing the buffer overflow should be fine.