On 8/10/20 3:43 PM, Mauro Matteo Cascella wrote:
> On Thu, Aug 6, 2020 at 3:21 PM Cédric Le Goater <c...@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.frede...@yahoo.fr>
>> Cc: Mauro Matteo Cascella <mcasc...@redhat.com>
>> Reported-by: Ziming Zhang <ezrak...@gmail.com>
>> Signed-off-by: Cédric Le Goater <c...@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);
>> + }
>> }
>>
>> ptr += len;
>> --
>> 2.25.4
>>
>
> I can confirm that I can't reproduce the issue with the above patch. Thank
> you.
>
Can I count that as a Tested-by ?
Thanks,
C.
