[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Stefan Weil
Subject: Re: [PATCH] net: eepro100: validate various address values
Date: Thu, 18 Feb 2021 17:10:53 +0100
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:78.0) Gecko/20100101 Thunderbird/78.7.1

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:
-- 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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]