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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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;
      }