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.
--
Mauro Matteo Cascella, Red Hat Product Security
6F78 E20B 5935 928C F0A8 1A9D 4E55 23B8 BB34 10B0
