On 221015 1710, Chris Friedt wrote:
> From: Christopher Friedt <cfri...@meta.com>
>
> In the case that size1 was zero, because of the explicit
> 'end1 > addr' check, the range check would fail and the error
> message would read as shown below. The correct comparison
> is 'end1 >= addr' (or 'addr <= end1').
>
> EDU: DMA range 0x40000-0x3ffff out of bounds (0x40000-0x40fff)!
>
> At the opposite end, in the case that size1 was 4096, within()
> would fail because of the non-inclusive check 'end1 < end2',
> which should have been 'end1 <= end2'. The error message would
> previously say
>
> EDU: DMA range 0x40000-0x40fff out of bounds (0x40000-0x40fff)!
>
> This change
> 1. renames local variables to be more less ambiguous
> 2. fixes the two off-by-one errors described above.
>
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1254
>
> Signed-off-by: Christopher Friedt <cfri...@meta.com>
Reviewed-by: Alexander Bulekov <alx...@bu.edu>
As a side-note, seems strange that edu_check_range will abort the entire
VM if the check fails, rather than handling the error more elegantly.
Maybe that's useful for students developing kernel drivers against the
device.
> ---
> hw/misc/edu.c | 25 ++++++++++++-------------
> 1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/hw/misc/edu.c b/hw/misc/edu.c
> index e935c418d4..52afbd792a 100644
> --- a/hw/misc/edu.c
> +++ b/hw/misc/edu.c
> @@ -103,25 +103,24 @@ static void edu_lower_irq(EduState *edu, uint32_t val)
> }
> }
>
> -static bool within(uint64_t addr, uint64_t start, uint64_t end)
> +static void edu_check_range(uint64_t xfer_start, uint64_t xfer_size,
> + uint64_t dma_start, uint64_t dma_size)
> {
> - return start <= addr && addr < end;
> -}
> -
> -static void edu_check_range(uint64_t addr, uint64_t size1, uint64_t start,
> - uint64_t size2)
> -{
> - uint64_t end1 = addr + size1;
> - uint64_t end2 = start + size2;
> -
> - if (within(addr, start, end2) &&
> - end1 > addr && within(end1, start, end2)) {
> + uint64_t xfer_end = xfer_start + xfer_size;
> + uint64_t dma_end = dma_start + dma_size;
> +
> + /*
> + * 1. ensure we aren't overflowing
> + * 2. ensure that xfer is within dma address range
> + */
> + if (dma_end >= dma_start && xfer_end >= xfer_start &&
> + xfer_start >= dma_start && xfer_end <= dma_end) {
> return;
> }
>
> hw_error("EDU: DMA range 0x%016"PRIx64"-0x%016"PRIx64
> " out of bounds (0x%016"PRIx64"-0x%016"PRIx64")!",
> - addr, end1 - 1, start, end2 - 1);
> + xfer_start, xfer_end - 1, dma_start, dma_end - 1);
> }
>
> static dma_addr_t edu_clamp_addr(const EduState *edu, dma_addr_t addr)
> --
> 2.36.1
>
>
