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

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

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>