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

[Peter Maydell]

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.)

thanks
-- PMM