[PATCH] hw/misc/edu: restrict dma access to dma buffer

[PATCH] hw/misc/edu: restrict dma access to dma buffer

[Torin Carey]

The EDU device doesn't enforce any bound checks on the addresses provided,
allowing users of the device to perform arbitrary reads and writes to QEMU's
address space.

Signed-off-by: Torin Carey <torin@tcarey.uk>
---
 hw/misc/edu.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

diff --git a/hw/misc/edu.c b/hw/misc/edu.c
index cece633e11..a4b01269e8 100644
--- a/hw/misc/edu.c
+++ b/hw/misc/edu.c
@@ -103,7 +103,7 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
     }
 }
 
-static void edu_check_range(uint64_t xfer_start, uint64_t xfer_size,
+static bool edu_check_range(uint64_t xfer_start, uint64_t xfer_size,
                 uint64_t dma_start, uint64_t dma_size)
 {
     uint64_t xfer_end = xfer_start + xfer_size;
@@ -115,13 +115,15 @@ static void edu_check_range(uint64_t xfer_start, uint64_t xfer_size,
      */
     if (dma_end >= dma_start && xfer_end >= xfer_start &&
         xfer_start >= dma_start && xfer_end <= dma_end) {
-        return;
+        return true;
     }
 
     qemu_log_mask(LOG_GUEST_ERROR,
                   "EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
                   " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
                   xfer_start, xfer_end - 1, dma_start, dma_end - 1);
+
+    return false;
 }
 
 static dma_addr_t edu_clamp_addr(const EduState *edu, dma_addr_t addr)
@@ -148,16 +150,18 @@ static void edu_dma_timer(void *opaque)
 
     if (EDU_DMA_DIR(edu->dma.cmd) == EDU_DMA_FROM_PCI) {
         uint64_t dst = edu->dma.dst;
-        edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE);
-        dst -= DMA_START;
-        pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
-                edu->dma_buf + dst, edu->dma.cnt);
+        if (edu_check_range(dst, edu->dma.cnt, DMA_START, DMA_SIZE)) {
+            dst -= DMA_START;
+            pci_dma_read(&edu->pdev, edu_clamp_addr(edu, edu->dma.src),
+                         edu->dma_buf + dst, edu->dma.cnt);
+        }
     } else {
         uint64_t src = edu->dma.src;
-        edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE);
-        src -= DMA_START;
-        pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
-                edu->dma_buf + src, edu->dma.cnt);
+        if (edu_check_range(src, edu->dma.cnt, DMA_START, DMA_SIZE)) {
+            src -= DMA_START;
+            pci_dma_write(&edu->pdev, edu_clamp_addr(edu, edu->dma.dst),
+                          edu->dma_buf + src, edu->dma.cnt);
+        }
     }
 
     edu->dma.cmd &= ~EDU_DMA_RUN;
-- 
2.47.3

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Jiri Slaby]

On 05. 11. 25, 13:18, Torin Carey wrote:
> The EDU device doesn't enforce any bound checks on the addresses provided,
> allowing users of the device to perform arbitrary reads and writes to QEMU's
> address space.

Hmm, it was the intention to crash qemu before:
commit 7b608e5d6c1d61430e81cd5c71b0277b99b03f3a
Author: Chris Friedt <chrisfriedt@gmail.com>
Date:   Tue Oct 18 08:25:51 2022 -0400

     hw: misc: edu: use qemu_log_mask instead of hw_error

     Log a guest error instead of a hardware error when
     the guest tries to DMA to / from an invalid address.



As with a standard device when you program it badly. I don't understand 
why the commit changed it to log only and let the code to corrupt the 
memory?

thanks,
-- 
js
suse labs

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Peter Maydell]

On Thu, 6 Nov 2025 at 06:29, Jiri Slaby <jslaby@suse.cz> wrote:
>
> On 05. 11. 25, 13:18, Torin Carey wrote:
> > The EDU device doesn't enforce any bound checks on the addresses provided,
> > allowing users of the device to perform arbitrary reads and writes to QEMU's
> > address space.
>
> Hmm, it was the intention to crash qemu before:
> commit 7b608e5d6c1d61430e81cd5c71b0277b99b03f3a
> Author: Chris Friedt <chrisfriedt@gmail.com>
> Date:   Tue Oct 18 08:25:51 2022 -0400
>
>      hw: misc: edu: use qemu_log_mask instead of hw_error
>
>      Log a guest error instead of a hardware error when
>      the guest tries to DMA to / from an invalid address.
>
>
>
> As with a standard device when you program it badly. I don't understand
> why the commit changed it to log only and let the code to corrupt the
> memory?

It's a PCI device. Unless something in the spec of
the device says "if you try to DMA outside this range
it will be ignored", then typically devices will let you
DMA anywhere in the address space. If the guest chooses
to program the device to DMA somewhere silly, that's its choice.

Is there a spec for this device anywhere? If so, we should
follow that. If not, then it's a "make a best guess", and
"don't arbitrarily constrain DMA" is a reasonable guess.

The reason for the commit above is that devices should
not call hw_error() as that crashes QEMU itself.

-- PMM

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Jiri Slaby]

On 06. 11. 25, 11:38, Peter Maydell wrote:
> On Thu, 6 Nov 2025 at 06:29, Jiri Slaby <jslaby@suse.cz> wrote:
>>
>> On 05. 11. 25, 13:18, Torin Carey wrote:
>>> The EDU device doesn't enforce any bound checks on the addresses provided,
>>> allowing users of the device to perform arbitrary reads and writes to QEMU's
>>> address space.
>>
>> Hmm, it was the intention to crash qemu before:
>> commit 7b608e5d6c1d61430e81cd5c71b0277b99b03f3a
>> Author: Chris Friedt <chrisfriedt@gmail.com>
>> Date:   Tue Oct 18 08:25:51 2022 -0400
>>
>>       hw: misc: edu: use qemu_log_mask instead of hw_error
>>
>>       Log a guest error instead of a hardware error when
>>       the guest tries to DMA to / from an invalid address.
>>
>>
>>
>> As with a standard device when you program it badly. I don't understand
>> why the commit changed it to log only and let the code to corrupt the
>> memory?
> 
> It's a PCI device. Unless something in the spec of
> the device says "if you try to DMA outside this range
> it will be ignored", then typically devices will let you
> DMA anywhere in the address space. If the guest chooses
> to program the device to DMA somewhere silly, that's its choice.
> 
> Is there a spec for this device anywhere? If so, we should
> follow that. If not, then it's a "make a best guess", and
> "don't arbitrarily constrain DMA" is a reasonable guess.

It's an educational, fictional device, there is of course no spec for that.

> The reason for the commit above is that devices should
> not call hw_error() as that crashes QEMU itself.

But that was exactly my intention. Students should see an immediate 
crash, not random, undebuggable (in the given class hours) writes 
somewhere. And crashing a qemu instance was an intended pun.

thanks,
-- 
js
suse labs

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Jiri Slaby]

On 06. 11. 25, 11:45, Jiri Slaby wrote:
> On 06. 11. 25, 11:38, Peter Maydell wrote:
>> On Thu, 6 Nov 2025 at 06:29, Jiri Slaby <jslaby@suse.cz> wrote:
>>>
>>> On 05. 11. 25, 13:18, Torin Carey wrote:
>>>> The EDU device doesn't enforce any bound checks on the addresses 
>>>> provided,
>>>> allowing users of the device to perform arbitrary reads and writes 
>>>> to QEMU's
>>>> address space.
>>>
>>> Hmm, it was the intention to crash qemu before:
>>> commit 7b608e5d6c1d61430e81cd5c71b0277b99b03f3a
>>> Author: Chris Friedt <chrisfriedt@gmail.com>
>>> Date:   Tue Oct 18 08:25:51 2022 -0400
>>>
>>>       hw: misc: edu: use qemu_log_mask instead of hw_error
>>>
>>>       Log a guest error instead of a hardware error when
>>>       the guest tries to DMA to / from an invalid address.
>>>
>>>
>>>
>>> As with a standard device when you program it badly. I don't understand
>>> why the commit changed it to log only and let the code to corrupt the
>>> memory?
>>
>> It's a PCI device. Unless something in the spec of
>> the device says "if you try to DMA outside this range
>> it will be ignored", then typically devices will let you
>> DMA anywhere in the address space. If the guest chooses
>> to program the device to DMA somewhere silly, that's its choice.
>>
>> Is there a spec for this device anywhere? If so, we should
>> follow that. If not, then it's a "make a best guess", and
>> "don't arbitrarily constrain DMA" is a reasonable guess.
> 
> It's an educational, fictional device, there is of course no spec for that.
> 
>> The reason for the commit above is that devices should
>> not call hw_error() as that crashes QEMU itself.
> 
> But that was exactly my intention. Students should see an immediate 
> crash, not random, undebuggable (in the given class hours) writes 
> somewhere. And crashing a qemu instance was an intended pun.

Which effectively translates the intent into: either we crash again 
(revert the commit above), or we only log a warning (is it by default 
logged at all?), but won't corrupt memory = no write happens.

> thanks,
-- 
js
suse labs

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Peter Maydell]

On Thu, 6 Nov 2025 at 10:45, Jiri Slaby <jslaby@suse.cz> wrote:
>
> On 06. 11. 25, 11:38, Peter Maydell wrote:
> > On Thu, 6 Nov 2025 at 06:29, Jiri Slaby <jslaby@suse.cz> wrote:
> >>
> >> On 05. 11. 25, 13:18, Torin Carey wrote:
> >>> The EDU device doesn't enforce any bound checks on the addresses provided,
> >>> allowing users of the device to perform arbitrary reads and writes to QEMU's
> >>> address space.
> >>
> >> Hmm, it was the intention to crash qemu before:
> >> commit 7b608e5d6c1d61430e81cd5c71b0277b99b03f3a
> >> Author: Chris Friedt <chrisfriedt@gmail.com>
> >> Date:   Tue Oct 18 08:25:51 2022 -0400
> >>
> >>       hw: misc: edu: use qemu_log_mask instead of hw_error
> >>
> >>       Log a guest error instead of a hardware error when
> >>       the guest tries to DMA to / from an invalid address.
> >>
> >>
> >>
> >> As with a standard device when you program it badly. I don't understand
> >> why the commit changed it to log only and let the code to corrupt the
> >> memory?
> >
> > It's a PCI device. Unless something in the spec of
> > the device says "if you try to DMA outside this range
> > it will be ignored", then typically devices will let you
> > DMA anywhere in the address space. If the guest chooses
> > to program the device to DMA somewhere silly, that's its choice.
> >
> > Is there a spec for this device anywhere? If so, we should
> > follow that. If not, then it's a "make a best guess", and
> > "don't arbitrarily constrain DMA" is a reasonable guess.
>
> It's an educational, fictional device, there is of course no spec for that.

I think that for teaching purposes you would want a decent
spec for the device: there will be a steady stream of new
students who need to know how it works.

In fact I've just noticed we do have a spec, in docs/specs/edu.rst.
(It doesn't specify the behaviour if you attempt to DMA outside
the range it documents.)

> > The reason for the commit above is that devices should
> > not call hw_error() as that crashes QEMU itself.
>
> But that was exactly my intention. Students should see an immediate
> crash, not random, undebuggable (in the given class hours) writes
> somewhere. And crashing a qemu instance was an intended pun.

Sorry, your educational device doesn't get to break QEMU's
usual rules. (Eventually we might be able to get rid of
hw_error() altogether, though it's hardly a high priority.)
People debugging drivers can turn on the GUEST_ERROR logging
which should be a big clue.

Incidentally, restricting DMA to "4K starting at 0x40000"
makes the device not usable on all machine types -- there is
no guarantee that the machine even has any RAM there at all.

thanks
-- PMM

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Jiri Slaby]

On 06. 11. 25, 11:53, Peter Maydell wrote:
>>> The reason for the commit above is that devices should
>>> not call hw_error() as that crashes QEMU itself.
>>
>> But that was exactly my intention. Students should see an immediate
>> crash, not random, undebuggable (in the given class hours) writes
>> somewhere. And crashing a qemu instance was an intended pun.
> 
> Sorry, your educational device doesn't get to break QEMU's
> usual rules.

The device is unusual.

> (Eventually we might be able to get rid of
> hw_error() altogether, though it's hardly a high priority.)

OK, then plan B, don't corrupt.

> People debugging drivers can turn on the GUEST_ERROR logging
> which should be a big clue.

Hm, so errors are not logged by default? Neither the man page, nor 
google yields anything useful on how to enable this. This does not look 
very promising.

> Incidentally, restricting DMA to "4K starting at 0x40000"
> makes the device not usable on all machine types -- there is
> no guarantee that the machine even has any RAM there at all.

It's not 0x40000 in the physical space at all.

thanks,
-- 
js
suse labs

Re: [PATCH] hw/misc/edu: restrict dma access to dma buffer

[Peter Maydell]

On Thu, 6 Nov 2025 at 11:01, Jiri Slaby <jslaby@suse.cz> wrote:
>
> On 06. 11. 25, 11:53, Peter Maydell wrote:
> > People debugging drivers can turn on the GUEST_ERROR logging
> > which should be a big clue.
>
> Hm, so errors are not logged by default? Neither the man page, nor
> google yields anything useful on how to enable this. This does not look
> very promising.

"-d guest_errors". There are other error categories which
you can find via '-d help'.

thanks
-- PMM