Re: [PATCH] hw/net/ftgmac100: Fix integer overflow in ftgmac100_do_tx()

[Mauro Matteo Cascella]

On Fri, Jul 10, 2020 at 1:33 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 10 Jul 2020 at 09:56, Mauro Matteo Cascella <mcascell@redhat.com> 
> wrote:
> >
> > An integer overflow issue was reported by Mr. Ziming Zhang, CC'd here. It
> > occurs while inserting the VLAN tag in packets whose length is less than
> > 12 bytes, as (len-12) is passed to memmove() without proper checking.
> > This patch is intended to fix this issue by checking the minimum
> > Ethernet frame size during packet transmission.
> >
> > Reported-by: Ziming Zhang <ezrakiez@gmail.com>
> > Signed-off-by: Mauro Matteo Cascella <mcascell@redhat.com>
> > ---
> >  hw/net/ftgmac100.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/hw/net/ftgmac100.c b/hw/net/ftgmac100.c
> > index 043ba61b86..bcf4d84aea 100644
> > --- a/hw/net/ftgmac100.c
> > +++ b/hw/net/ftgmac100.c
> > @@ -238,6 +238,11 @@ typedef struct {
> >   */
> >  #define FTGMAC100_MAX_FRAME_SIZE    9220
> >
> > +/*
> > + * Min frame size
> > + */
> > +#define FTGMAC100_MIN_FRAME_SIZE    64
> > +
> >  /* Limits depending on the type of the frame
> >   *
> >   *   9216 for Jumbo frames (+ 4 for VLAN)
> > @@ -507,6 +512,15 @@ static void ftgmac100_do_tx(FTGMAC100State *s, 
> > uint32_t tx_ring,
> >          }
> >
> >          len = FTGMAC100_TXDES0_TXBUF_SIZE(bd.des0);
> > +
> > +        /* drop small packets */
> > +        if (bd.des0 & FTGMAC100_TXDES0_FTS &&
> > +            len < FTGMAC100_MIN_FRAME_SIZE) {
> > +            qemu_log_mask(LOG_GUEST_ERROR, "%s: frame too small: %d 
> > bytes\n",
> > +                          __func__, len);
> > +            break;
> > +        }
> > +
>
> Andrew, Cedric: do you have the datasheet for this devic? Do you
> know if we should also be flagging the error back to the
> guest somehow?
>
> I think a 'break' here means we'll never update the
> descriptor flags to hand it back to the guest, which
> is probably not what the hardware does.
>
> thanks
> -- PMM
>

I thought of setting FTGMAC100_INT_XPKT_LOST, but not sure if this is
the most appropriate flag here.

Regards,
Mauro