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;
--
2.34.1On 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;
}
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
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>
© 2016 - 2024 Red Hat, Inc.