Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()

From: "Cédric Le Goater" <clg@kaod.org>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: Mauro Matteo Cascella <mcascell@redhat.com>,
	Frederic Konrad <konrad.frederic@yahoo.fr>,
	Andrew Jeffery <andrew@aj.id.au>,
	QEMU Developers <qemu-devel@nongnu.org>,
	qemu-arm <qemu-arm@nongnu.org>, Joel Stanley <joel@jms.id.au>,
	Ziming Zhang <ezrakiez@gmail.com>
Subject: Re: [PATCH for-5.2 14/19] ftgmac100: Fix integer overflow in ftgmac100_do_tx()
Date: Tue, 18 Aug 2020 16:54:02 +0200	[thread overview]
Message-ID: <7a6f7515-9a5f-3160-6187-675032ce8c75@kaod.org> (raw)
In-Reply-To: <CAFEAcA_rAre03ATda5rxgear4wsoef2-qD5SyAjunH8QW65W2A@mail.gmail.com>

On 8/11/20 2:39 PM, Peter Maydell wrote:
> On Thu, 6 Aug 2020 at 14:21, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> When inserting the VLAN tag in packets, memmove() can generate an
>> integer overflow for packets whose length is less than 12 bytes.
>>
>> Check length against the size of the ethernet header (14 bytes) to
>> avoid the crash and return FTGMAC100_INT_XPKT_LOST status. This seems
>> like a good modeling choice even if Aspeed does not specify anything
>> in that case.
>>
>> Cc: Frederic Konrad <konrad.frederic@yahoo.fr>
>> Cc: Mauro Matteo Cascella <mcascell@redhat.com>
>> Reported-by: Ziming Zhang <ezrakiez@gmail.com>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>  hw/net/ftgmac100.c | 19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
>> index 280aa3d3a1e2..987b843fabc4 100644
>> --- a/hw/net/ftgmac100.c
>> +++ b/hw/net/ftgmac100.c
>> @@ -540,10 +540,21 @@ static void ftgmac100_do_tx(FTGMAC100State *s, uint32_t tx_ring,
>>                  s->isr |= FTGMAC100_INT_XPKT_LOST;
>>                  len =  sizeof(s->frame) - frame_size - 4;
>>              }
>> -            memmove(ptr + 16, ptr + 12, len - 12);
>> -            stw_be_p(ptr + 12, ETH_P_VLAN);
>> -            stw_be_p(ptr + 14, bd.des1);
>> -            len += 4;
>> +
>> +            if (len < sizeof(struct eth_header)) {
>> +                qemu_log_mask(LOG_GUEST_ERROR,
>> +                         "%s: frame too small for VLAN insertion : %d bytes\n",
>> +                         __func__, len);
>> +                s->isr |= FTGMAC100_INT_XPKT_LOST;
>> +            } else {
>> +                uint8_t *vlan_hdr = ptr + (ETH_ALEN * 2);
>> +                uint8_t *payload = vlan_hdr + sizeof(struct vlan_header);
>> +
>> +                memmove(payload, vlan_hdr, len - (ETH_ALEN * 2));
>> +                stw_be_p(vlan_hdr, ETH_P_VLAN);
>> +                stw_be_p(vlan_hdr + 2, FTGMAC100_TXDES1_VLANTAG_CI(bd.des1));
>> +                len += sizeof(struct vlan_header);
>> +            }
>>          }
> 
> If you want to be picky, this will unnecessarily fail for the case of
> a packet that is big enough for the vlan header but which has been
> split up into multiple tx descriptors such that the first one is
> smaller than the size of the eth_header. You could fix that by
> doing the insertion of the vlan tag when you process the TXDES0_LTS
> descriptor rather than when you process the TXDES0_FTS one. (We
> already save the des1 info where the INS_VLANTAG flag is in the
> 'flags' variable, so we have that available for the LTS descriptor code.)

yes. Good idea. The code is cleaner and the driver can even survive 
a bogus frame.

I will send a new version, without the Tested and Reviewed tags.

To reproduce, I have created a little kernel module tester based 
on the POC proposed by Ziming, which was for another MAC.

	https://github.com/legoater/ftgmac100-test

Thanks,

C. 