[Qemu-devel] [PATCH] apb: Fix out-of-bounds array write access

[Qemu-devel] [PATCH] apb: Fix out-of-bounds array write access

[Stefan Weil]

The array regs is declared with IOMMU_NREGS (3) elements and accessed
using IOMMU_CTRL (0) and IOMMU_BASE (8). In most cases, those values
are right shifted before being used as an index which results in indices
0 and 1. In one case, this right shift was missing for IOMMU_BASE which
results in an out-of-bounds write access with index 8.

The patch adds the missing shift operation also for IOMMU_CTRL where
it is needed only for cosmetic reasons.

Signed-off-by: Stefan Weil <sw@weilnetz.de>
---

Any reason why the array is declared with 3 elements when only the first 2
are used?

Regards,
Stefan

 hw/pci-host/apb.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
index 1497008..887338e 100644
--- a/hw/pci-host/apb.c
+++ b/hw/pci-host/apb.c
@@ -1,4 +1,4 @@
-/*
+re/*
  * QEMU Ultrasparc APB PCI host
  *
  * Copyright (c) 2006 Fabrice Bellard
@@ -333,7 +333,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
             is->regs[IOMMU_CTRL >> 3] &= 0xffffffffULL;
             is->regs[IOMMU_CTRL >> 3] |= val << 32;
         } else {
-            is->regs[IOMMU_CTRL] = val;
+            is->regs[IOMMU_CTRL >> 3] = val;
         }
         break;
     case IOMMU_CTRL + 0x4:
@@ -345,7 +345,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
             is->regs[IOMMU_BASE >> 3] &= 0xffffffffULL;
             is->regs[IOMMU_BASE >> 3] |= val << 32;
         } else {
-            is->regs[IOMMU_BASE] = val;
+            is->regs[IOMMU_BASE >> 3] = val;
         }
         break;
     case IOMMU_BASE + 0x4:
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH] apb: Fix out-of-bounds array write access

[Stefan Weil]

Am 09.06.2014 16:19, schrieb Stefan Weil:
> The array regs is declared with IOMMU_NREGS (3) elements and accessed
> using IOMMU_CTRL (0) and IOMMU_BASE (8). In most cases, those values
> are right shifted before being used as an index which results in indices
> 0 and 1. In one case, this right shift was missing for IOMMU_BASE which
> results in an out-of-bounds write access with index 8.
> 
> The patch adds the missing shift operation also for IOMMU_CTRL where
> it is needed only for cosmetic reasons.
> 
> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
> 
> Any reason why the array is declared with 3 elements when only the first 2
> are used?
> 
> Regards,
> Stefan
> 
>  hw/pci-host/apb.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 1497008..887338e 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
Snip here ---->
> @@ -1,4 +1,4 @@
> -/*
> +re/*

Please remove this part before applying the patch. It was added
accidentally when I added a comment to the patch file


>   * QEMU Ultrasparc APB PCI host
>   *
>   * Copyright (c) 2006 Fabrice Bellard
<---- until here.
> @@ -333,7 +333,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
>              is->regs[IOMMU_CTRL >> 3] &= 0xffffffffULL;
>              is->regs[IOMMU_CTRL >> 3] |= val << 32;
>          } else {
> -            is->regs[IOMMU_CTRL] = val;
> +            is->regs[IOMMU_CTRL >> 3] = val;
>          }
>          break;
>      case IOMMU_CTRL + 0x4:
> @@ -345,7 +345,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
>              is->regs[IOMMU_BASE >> 3] &= 0xffffffffULL;
>              is->regs[IOMMU_BASE >> 3] |= val << 32;
>          } else {
> -            is->regs[IOMMU_BASE] = val;
> +            is->regs[IOMMU_BASE >> 3] = val;
>          }
>          break;
>      case IOMMU_BASE + 0x4:
> 

Re: [Qemu-devel] [PATCH] apb: Fix out-of-bounds array write access

[Mark Cave-Ayland]

On 09/06/14 15:19, Stefan Weil wrote:

Hi Stefan,

> The array regs is declared with IOMMU_NREGS (3) elements and accessed
> using IOMMU_CTRL (0) and IOMMU_BASE (8). In most cases, those values
> are right shifted before being used as an index which results in indices
> 0 and 1. In one case, this right shift was missing for IOMMU_BASE which
> results in an out-of-bounds write access with index 8.

Ah yes, this is correct. Thanks for finding this!

> The patch adds the missing shift operation also for IOMMU_CTRL where
> it is needed only for cosmetic reasons.

Fine with me too.

> Signed-off-by: Stefan Weil <sw@weilnetz.de>
> ---
>
> Any reason why the array is declared with 3 elements when only the first 2
> are used?

There is a 3rd register which can be used to flush the IOMMU TLB 
translations, although it isn't really meaningful here. In the end I 
decided to let the code fall into the "unimplemented" section just so it 
would be possible to log any accesses in case it affected some of the 
other diagnostic registers.

> Regards,
> Stefan
>
>   hw/pci-host/apb.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
> index 1497008..887338e 100644
> --- a/hw/pci-host/apb.c
> +++ b/hw/pci-host/apb.c
> @@ -1,4 +1,4 @@
> -/*
> +re/*
>    * QEMU Ultrasparc APB PCI host
>    *
>    * Copyright (c) 2006 Fabrice Bellard
> @@ -333,7 +333,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
>               is->regs[IOMMU_CTRL >> 3] &= 0xffffffffULL;
>               is->regs[IOMMU_CTRL >> 3] |= val << 32;
>           } else {
> -            is->regs[IOMMU_CTRL] = val;
> +            is->regs[IOMMU_CTRL >> 3] = val;
>           }
>           break;
>       case IOMMU_CTRL + 0x4:
> @@ -345,7 +345,7 @@ static void iommu_config_write(void *opaque, hwaddr addr,
>               is->regs[IOMMU_BASE >> 3] &= 0xffffffffULL;
>               is->regs[IOMMU_BASE >> 3] |= val << 32;
>           } else {
> -            is->regs[IOMMU_BASE] = val;
> +            is->regs[IOMMU_BASE >> 3] = val;
>           }
>           break;
>       case IOMMU_BASE + 0x4:

I've just done a test with these changes (minus the typo on the first 
line) and Linux still seems fine, so happy for this to be applied.


ATB,

Mark.

Re: [Qemu-devel] [PATCH] apb: Fix out-of-bounds array write access

[Mark Cave-Ayland]

On 09/06/14 15:19, Stefan Weil wrote:

> The array regs is declared with IOMMU_NREGS (3) elements and accessed
> using IOMMU_CTRL (0) and IOMMU_BASE (8). In most cases, those values
> are right shifted before being used as an index which results in indices
> 0 and 1. In one case, this right shift was missing for IOMMU_BASE which
> results in an out-of-bounds write access with index 8.
>
> The patch adds the missing shift operation also for IOMMU_CTRL where
> it is needed only for cosmetic reasons.
>
> Signed-off-by: Stefan Weil <sw@weilnetz.de>

BTW I forgot to add - do I need to send a further pull request for this 
or is it intended to go via trivial?


ATB,

Mark.