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

[Stefan Weil <sw@weilnetz.de>]

Am 18.02.21 um 15:41 schrieb 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.


I agree with Peter. The hardware emulation in QEMU should try to do the 
same as the real hardware.

Assertions (not with assert(), but with g_assert() and related 
functions) are good to catch implementation errors in QEMU code, but not 
to catch bad programming in guest code.

In this special case the emulation code already includes a hack to catch 
endless loops caused by buggy guest code: it has a limit of 16 cycles 
and then prints a log message. This is a compromise to emulate the real 
hardware as good as possible without making QEMU running an endless loop 
or requiring an extra thread to emulate the hardware loop until it is 
stopped by new I/O operations.

Stefan