On Thu, 24 Aug 2023 at 16:47, Philippe Mathieu-Daudé <phi...@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.mayd...@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