Re: [PATCH 1/4] hw/net/fsl_etsec/rings.c: Avoid variable length array

[Peter Maydell]

On Thu, 24 Aug 2023 at 16:47, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
>
> On 24/8/23 17:32, Peter Maydell wrote:
> > In fill_rx_bd() we create a variable length array of size
> > etsec->rx_padding. In fact we know that this will never be
> > larger than 64 bytes, because rx_padding is set in rx_init_frame()
> > in a way that ensures it is only that large. Use a fixed sized
> > array and assert that it is big enough.
> >
> > Since padd[] is now potentially rather larger than the actual
> > padding required, adjust the memset() we do on it to match the
> > size that we write with cpu_physical_memory_write(), rather than
> > clearing the entire array.
> >
> > The codebase has very few VLAs, and if we can get rid of them all we
> > can make the compiler error on new additions.  This is a defensive
> > measure against security bugs where an on-stack dynamic allocation
> > isn't correctly size-checked (e.g.  CVE-2021-3527).
> >
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> >   hw/net/fsl_etsec/rings.c | 12 ++++++++++--
> >   1 file changed, 10 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/net/fsl_etsec/rings.c b/hw/net/fsl_etsec/rings.c
> > index 788463f1b62..2f2f359f7a5 100644
> > --- a/hw/net/fsl_etsec/rings.c
> > +++ b/hw/net/fsl_etsec/rings.c
> > @@ -372,6 +372,12 @@ void etsec_walk_tx_ring(eTSEC *etsec, int ring_nbr)
> >       etsec->regs[TSTAT].value |= 1 << (31 - ring_nbr);
> >   }
> >
> > +/*
> > + * rx_init_frame() ensures we never do more padding than this
> > + * (checksum plus minimum data packet size)
> > + */
> > +#define MAX_RX_PADDING 64
> > +
> >   static void fill_rx_bd(eTSEC          *etsec,
> >                          eTSEC_rxtx_bd  *bd,
> >                          const uint8_t **buf,
> > @@ -380,9 +386,11 @@ static void fill_rx_bd(eTSEC          *etsec,
> >       uint16_t to_write;
> >       hwaddr   bufptr = bd->bufptr +
> >           ((hwaddr)(etsec->regs[TBDBPH].value & 0xF) << 32);
> > -    uint8_t  padd[etsec->rx_padding];
> > +    uint8_t  padd[MAX_RX_PADDING];
> >       uint8_t  rem;
> >
> > +    assert(etsec->rx_padding <= MAX_RX_PADDING);
> > +
> >       RING_DEBUG("eTSEC fill Rx buffer @ 0x%016" HWADDR_PRIx
> >                  " size:%zu(padding + crc:%u) + fcb:%u\n",
> >                  bufptr, *size, etsec->rx_padding, etsec->rx_fcb_size);
> > @@ -426,7 +434,7 @@ static void fill_rx_bd(eTSEC          *etsec,
> >           rem = MIN(etsec->regs[MRBLR].value - bd->length, 
> > etsec->rx_padding);
> >
> >           if (rem > 0) {
> > -            memset(padd, 0x0, sizeof(padd));
> > +            memset(padd, 0x0, rem);
> >               etsec->rx_padding -= rem;
> >               *size             -= rem;
> >               bd->length        += rem;
>
> Maybe we can add this for clarity:
>
> @@ -468,6 +468,6 @@ static void rx_init_frame(eTSEC *etsec, const
> uint8_t *buf, size_t size)
>        * minimum MTU size bytes long (64)
>        */
> -    if (etsec->rx_buffer_len < 60) {
> -        etsec->rx_padding += 60 - etsec->rx_buffer_len;
> +    if (etsec->rx_padding + etsec->rx_buffer_len < MAX_RX_PADDING) {
> +        etsec->rx_padding = MAX_RX_PADDING - etsec->rx_buffer_len;
>       }

I think that's a more confusing way of putting it. What the
code is doing is "if the packet is too short, pad it to
the minimum-packet-length", and the clear way to express
that is "if (packet_len < max) add_more_padding;".

There is potential to use the constants ETH_ZLEN (60) and
ETH_FCS_LEN (4) instead of the hard-coded 60 and 4 currently
in the code, but I felt that was starting to wander a bit
out of scope of just getting rid of the VLA.

thanks
-- PMM