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

From: Mauro Matteo Cascella <mcascell@redhat.com>
To: "Cédric Le Goater" <clg@kaod.org>
Cc: Peter Maydell <peter.maydell@linaro.org>,
	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: Mon, 10 Aug 2020 15:43:00 +0200	[thread overview]
Message-ID: <CAA8xKjVfG8cp_g6FDNxUCQ6dqiMHnf17eMNCE2Wz+Vr+grAprw@mail.gmail.com> (raw)
In-Reply-To: <20200806132106.747414-15-clg@kaod.org>

On Thu, Aug 6, 2020 at 3:21 PM 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);
> +            }
>          }
>
>          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