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.3On 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
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
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
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
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
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
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
© 2016 - 2025 Red Hat, Inc.