Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

["Cédric Le Goater" <clg@kaod.org>]

On 7/10/20 1:33 PM, Peter Maydell wrote:
> On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcascell@redhat.com> wrote:
>>
>> An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
>> occurs while inserting the VLAN tag in packets whose length is less than
>> 12 bytes, as (len-12) is passed to memmove() without proper checking.
>> This patch is intended to fix this issue by checking the minimum
>> Ethernet frame size during packet transmission.
>>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
>> ---
>>  hw/net/ftgmac100.c | 14 ++++++++++++++
>>  1 file changed, 14 insertions(+)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 043ba61b86..bcf4d84aea 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -238,6 +238,11 @@ typedef struct {
>>   */
>>  #define FTGMAC100_MAX_FRAME_SIZE    9220
>>
>> +/*
>> + * Min frame size
>> + */
>> +#define FTGMAC100_MIN_FRAME_SIZE    64
>> +
>>  /* Limits depending on the type of the frame
>>   *
>>   *   9216 for Jumbo frames (+ 4 for VLAN)
>> @@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>          }
>>
>>          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
>> +
>> +        /* drop small packets */
>> +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
>> +            len < FTGMAC100_MIN_FRAME_SIZE) {
>> +            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d bytes\n",
>> +                          __func__, len);
>> +            break;
>> +        }
>> +
> 
> Andrew, Cedric: do you have the datasheet for this devic? Do you
> know if we should also be flagging the error back to the
> guest somehow?

zero is the only invalid size of a transmit buffer and the specs does
not have any special information on which bit to raise in that case.

I think FTGMAC100_INT_NO_NPTXBUF (transmit buffer unavailable) is our 
best option and we should add an extra 'len == 0' test in front of 
the dma_memory_read() call to raise it. A zero length is not considered 
bogus by dma_memory_read() it seems. Is address zero considered bogus ? 
If not, we need to check that also. 

The code doing the memmove() should be protected by a check on the 
length, 'sizeof(struct eth_header)' or ETH_HLEN. That will fix the 
integer overflow. 

For clarity, we could replace 12 and 16 by a L2 header size: 
'sizeof(struct eth_header)' and 
'sizeof(struct eth_header) + sizeof(struct vlan_header)'. It can be
done later.

Thanks,

C. 


> I think a 'break' here means we'll never update the
> descriptor flags to hand it back to the guest, which
> is probably not what the hardware does.
> 
> thanks
> -- PMM
>