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

[Jason Wang <jasowang@redhat.com>]

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