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

[Philippe Mathieu-Daudé]

On 24/8/23 18:01, Peter Maydell wrote:
> 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


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

Right. So possibly:

#define MAX_RX_PADDING (ETH_ZLEN + ETH_FCS_LEN)

but 64 is clear enough.

Thanks for the feedback.

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>