Re: [PATCH] net: eepro100: validate various address values

[Peter Maydell]

On Thu, 18 Feb 2021 at 14:13, P J P <ppandit@redhat.com> wrote:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> While processing controller commands, eepro100 emulator gets
> command unit(CU) base address OR receive unit (RU) base address
> OR command block (CB) address from guest. If these values are not
> checked, it may lead to an infinite loop kind of issues. Add checks
> to avoid it.
>
> Reported-by: Ruhr-University Bochum <bugs-syssec@rub.de>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
>  hw/net/eepro100.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/hw/net/eepro100.c b/hw/net/eepro100.c
> index 16e95ef9cc..afa1c9b2aa 100644
> --- a/hw/net/eepro100.c
> +++ b/hw/net/eepro100.c
> @@ -843,7 +843,8 @@ static void action_command(EEPRO100State *s)
>          bool bit_i;
>          bool bit_nc;
>          uint16_t ok_status = STATUS_OK;
> -        s->cb_address = s->cu_base + s->cu_offset;
> +        s->cb_address = s->cu_base + s->cu_offset;  /* uint32_t overflow */
> +        assert (s->cb_address >= s->cu_base);

We get these values from the guest; you can't just assert() on them.
You need to do something else.

My reading of the 8255x data sheet is that there is nothing
in the hardware that forbids the guest from programming the
device such that the cu_base + cu_offset wraps around:
http://www.intel.com/content/dam/doc/manual/8255x-10-100-mbps-ethernet-controller-software-dev-manual.pdf
-- page 30 says that this is all doing 32-bit arithmetic
on addresses and doesn't say that there is any special case
handling by the device of overflow of that addition.

Your commit message isn't very clear about what the failure
case is here, but I think the fix has to be something
different from this.

thanks
-- PMM