On 7/10/20 1:33 PM, Peter Maydell wrote:
> On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcasc...@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 <ezrak...@gmail.com>
>> Signed-off-by: Mauro Matteo Cascella <mcasc...@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
>
