Re: [PATCH] hw/net/smc91c111: Don't allow data register access to overrun buffer

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

On 28/2/25 20:16, Peter Maydell wrote:
> For accesses to the 91c111 data register, the address within the
> packet's data frame is determined by a combination of the pointer
> register and the offset used to access the data register, so that you
> can access data at effectively wider than byte width.  The pointer
> register's pointer field is 11 bits wide, which is exactly the size
> to index a 2048-byte data frame.
> 
> We weren't quite getting the logic right for ensuring that we end up
> with a pointer value to use in the s->data[][] array that isn't out
> of bounds:
> 
>   * we correctly mask when getting the initial pointer value
>   * for the "autoincrement the pointer register" case, we
>     correctly mask after adding 1 so that the pointer register
>     wraps back around at the 2048 byte mark
>   * but for the non-autoincrement case where we have to add the
>     low 2 bits of the data register offset, we don't account
>     for the possibility that the pointer register is 0x7ff
>     and the addition should wrap
> 
> Fix this bug by factoring out the "get the p value to use as an array
> index" into a function, making it use FIELD macro names rather than
> hard-coded constants, and having a utility function that does "add a
> value and wrap it" that we can use both for the "autoincrement" and
> "add the offset bits" codepaths.
> 
> Cc: qemu-stable@nongnu.org
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2758
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Based-on: 20250228174802.1945417-1-peter.maydell@linaro.org
> ("hw/net/smc91c111: Fix potential array overflows")
> 
> I'd only just sent the other smc91c111 patchset when I found
> another fuzzer bug for it in the bug tracker :-)
> 
> The "modernisation" of the code seemed clearer to me than
> doing a minimal in-place fix (which would look something
> like "p = (p + (offset & 3) & 0x7ff;" in both read and write
> functions). But it does make the actual bugfix a little less
> obvious.
> 
> ---
>   hw/net/smc91c111.c | 65 +++++++++++++++++++++++++++++++++++++---------
>   1 file changed, 53 insertions(+), 12 deletions(-)
Well clean, thanks.

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