On 01/15/2016 02:19 PM, Peter Crosthwaite wrote:
> On Thu, Jan 14, 2016 at 2:03 AM, Peter Maydell <peter.mayd...@linaro.org>
> wrote:
>> On 14 January 2016 at 09:43, Michael S. Tsirkin <m...@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...@360.cn>
>>> Signed-off-by: Michael S. Tsirkin <m...@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
>>
