[Qemu-devel] [PATCH for-3.1 0/2] hw/block/onenand: fix out-of-bounds read

[Qemu-devel] [PATCH for-3.1 0/2] hw/block/onenand: fix out-of-bounds read

[Peter Maydell]

An off-by-one error in a switch case in onenand_read() allowed
a misbehaving guest to read off the end of a block of memory.
This bug was revealed now that QEMU supports execution from
device memory: if started with no guest image loaded,
"qemu-system-arm -M n800" will now try to execute by reading
from this device's registers until it gets to the offset
triggering the segfault. Previously it would have run into
the "can't execute from device memory" assert first.

As a followup, patch 2 fixes some reporting so that we don't
trigger a hw_error() for reads of unimplemented registers.
With the two patches, execution now successfully results in
QEMU emulating the guest being off in the weeds...
    
NB: the onenand device is used only by the "n800" and "n810"
machines, which are usable only with TCG, not KVM, so the
segfault is not a security issue.

thanks
-- PMM

Peter Maydell (2):
  hw/block/onenand: Fix off-by-one error allowing out-of-bounds read
  hw/block/onenand: use qemu_log_mask() for reporting

 hw/block/onenand.c | 24 ++++++++++++++----------
 1 file changed, 14 insertions(+), 10 deletions(-)

-- 
2.19.1

[Qemu-devel] [PATCH for-3.1 1/2] hw/block/onenand: Fix off-by-one error allowing out-of-bounds read

[Peter Maydell]

An off-by-one error in a switch case in onenand_read() allowed
a misbehaving guest to read off the end of a block of memory.

NB: the onenand device is used only by the "n800" and "n810"
machines, which are usable only with TCG, not KVM, so this is
not a security issue.

Reported-by: Thomas Huth <thuth@redhat.com>
Suggested-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
I tweaked RTH's suggested fix to use an 0xbffe offset so
we don't overrun on an access to 0xbfff either.

 hw/block/onenand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 0cb8d7fa135..49ef68c9b14 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -608,7 +608,7 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
     int offset = addr >> s->shift;
 
     switch (offset) {
-    case 0x0000 ... 0xc000:
+    case 0x0000 ... 0xbffe:
         return lduw_le_p(s->boot[0] + addr);
 
     case 0xf000:	/* Manufacturer ID */
-- 
2.19.1

[Qemu-devel] [PATCH for-3.1 2/2] hw/block/onenand: use qemu_log_mask() for reporting

[Peter Maydell]

Update the onenand device to use qemu_log_mask() for reporting
guest errors and unimplemented features, rather than plain
fprintf() and hw_error().

(We leave the hw_error() in onenand_reset(), as that is
triggered by a failure to read the underlying block device
for the bootRAM, not by guest action.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/block/onenand.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/hw/block/onenand.c b/hw/block/onenand.c
index 49ef68c9b14..2b48609776d 100644
--- a/hw/block/onenand.c
+++ b/hw/block/onenand.c
@@ -28,6 +28,7 @@
 #include "exec/memory.h"
 #include "hw/sysbus.h"
 #include "qemu/error-report.h"
+#include "qemu/log.h"
 
 /* 11 for 2kB-page OneNAND ("2nd generation") and 10 for 1kB-page chips */
 #define PAGE_SHIFT	11
@@ -594,8 +595,8 @@ static void onenand_command(OneNANDState *s)
     default:
         s->status |= ONEN_ERR_CMD;
         s->intstatus |= ONEN_INT;
-        fprintf(stderr, "%s: unknown OneNAND command %x\n",
-                        __func__, s->command);
+        qemu_log_mask(LOG_GUEST_ERROR, "unknown OneNAND command %x\n",
+                      s->command);
     }
 
     onenand_intr_update(s);
@@ -657,12 +658,13 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
     case 0xff02:	/* ECC Result of spare area data */
     case 0xff03:	/* ECC Result of main area data */
     case 0xff04:	/* ECC Result of spare area data */
-        hw_error("%s: implement ECC\n", __func__);
+        qemu_log_mask(LOG_UNIMP,
+                      "onenand: ECC result registers unimplemented\n");
         return 0x0000;
     }
 
-    fprintf(stderr, "%s: unknown OneNAND register %x\n",
-                    __func__, offset);
+    qemu_log_mask(LOG_GUEST_ERROR, "read of unknown OneNAND register 0x%x\n",
+                  offset);
     return 0;
 }
 
@@ -706,8 +708,9 @@ static void onenand_write(void *opaque, hwaddr addr,
             break;
 
         default:
-            fprintf(stderr, "%s: unknown OneNAND boot command %"PRIx64"\n",
-                            __func__, value);
+            qemu_log_mask(LOG_GUEST_ERROR,
+                          "unknown OneNAND boot command %" PRIx64 "\n",
+                          value);
         }
         break;
 
@@ -757,8 +760,9 @@ static void onenand_write(void *opaque, hwaddr addr,
         break;
 
     default:
-        fprintf(stderr, "%s: unknown OneNAND register %x\n",
-                        __func__, offset);
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "write to unknown OneNAND register 0x%x\n",
+                      offset);
     }
 }
 
-- 
2.19.1

Re: [Qemu-devel] [PATCH for-3.1 2/2] hw/block/onenand: use qemu_log_mask() for reporting

[Philippe Mathieu-Daudé]

On 15/11/18 15:35, Peter Maydell wrote:
> Update the onenand device to use qemu_log_mask() for reporting
> guest errors and unimplemented features, rather than plain
> fprintf() and hw_error().
> 
> (We leave the hw_error() in onenand_reset(), as that is
> triggered by a failure to read the underlying block device
> for the bootRAM, not by guest action.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>   hw/block/onenand.c | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index 49ef68c9b14..2b48609776d 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -28,6 +28,7 @@
>   #include "exec/memory.h"
>   #include "hw/sysbus.h"
>   #include "qemu/error-report.h"
> +#include "qemu/log.h"
>   
>   /* 11 for 2kB-page OneNAND ("2nd generation") and 10 for 1kB-page chips */
>   #define PAGE_SHIFT	11
> @@ -594,8 +595,8 @@ static void onenand_command(OneNANDState *s)
>       default:
>           s->status |= ONEN_ERR_CMD;
>           s->intstatus |= ONEN_INT;
> -        fprintf(stderr, "%s: unknown OneNAND command %x\n",
> -                        __func__, s->command);
> +        qemu_log_mask(LOG_GUEST_ERROR, "unknown OneNAND command %x\n",
> +                      s->command);
>       }
>   
>       onenand_intr_update(s);
> @@ -657,12 +658,13 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
>       case 0xff02:	/* ECC Result of spare area data */
>       case 0xff03:	/* ECC Result of main area data */
>       case 0xff04:	/* ECC Result of spare area data */
> -        hw_error("%s: implement ECC\n", __func__);
> +        qemu_log_mask(LOG_UNIMP,
> +                      "onenand: ECC result registers unimplemented\n");
>           return 0x0000;
>       }
>   
> -    fprintf(stderr, "%s: unknown OneNAND register %x\n",
> -                    __func__, offset);
> +    qemu_log_mask(LOG_GUEST_ERROR, "read of unknown OneNAND register 0x%x\n",
> +                  offset);
>       return 0;
>   }
>   
> @@ -706,8 +708,9 @@ static void onenand_write(void *opaque, hwaddr addr,
>               break;
>   
>           default:
> -            fprintf(stderr, "%s: unknown OneNAND boot command %"PRIx64"\n",
> -                            __func__, value);
> +            qemu_log_mask(LOG_GUEST_ERROR,
> +                          "unknown OneNAND boot command %" PRIx64 "\n",
> +                          value);
>           }
>           break;
>   
> @@ -757,8 +760,9 @@ static void onenand_write(void *opaque, hwaddr addr,
>           break;
>   
>       default:
> -        fprintf(stderr, "%s: unknown OneNAND register %x\n",
> -                        __func__, offset);
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "write to unknown OneNAND register 0x%x\n",
> +                      offset);
>       }
>   }
>   
> 

Re: [Qemu-devel] [PATCH for-3.1 1/2] hw/block/onenand: Fix off-by-one error allowing out-of-bounds read

[Philippe Mathieu-Daudé]

On 15/11/18 15:35, Peter Maydell wrote:
> An off-by-one error in a switch case in onenand_read() allowed
> a misbehaving guest to read off the end of a block of memory.
> 
> NB: the onenand device is used only by the "n800" and "n810"
> machines, which are usable only with TCG, not KVM, so this is
> not a security issue.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I tweaked RTH's suggested fix to use an 0xbffe offset so
> we don't overrun on an access to 0xbfff either.

Correct.

> 
>   hw/block/onenand.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/block/onenand.c b/hw/block/onenand.c
> index 0cb8d7fa135..49ef68c9b14 100644
> --- a/hw/block/onenand.c
> +++ b/hw/block/onenand.c
> @@ -608,7 +608,7 @@ static uint64_t onenand_read(void *opaque, hwaddr addr,
>       int offset = addr >> s->shift;
>   
>       switch (offset) {
> -    case 0x0000 ... 0xc000:
> +    case 0x0000 ... 0xbffe:
>           return lduw_le_p(s->boot[0] + addr);
>   
>       case 0xf000:	/* Manufacturer ID */
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH for-3.1 1/2] hw/block/onenand: Fix off-by-one error allowing out-of-bounds read

[Richard Henderson]

On 11/15/18 3:35 PM, Peter Maydell wrote:
> An off-by-one error in a switch case in onenand_read() allowed
> a misbehaving guest to read off the end of a block of memory.
> 
> NB: the onenand device is used only by the "n800" and "n810"
> machines, which are usable only with TCG, not KVM, so this is
> not a security issue.
> 
> Reported-by: Thomas Huth <thuth@redhat.com>
> Suggested-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> I tweaked RTH's suggested fix to use an 0xbffe offset so
> we don't overrun on an access to 0xbfff either.
> 
>  hw/block/onenand.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH for-3.1 2/2] hw/block/onenand: use qemu_log_mask() for reporting

[Richard Henderson]

On 11/15/18 3:35 PM, Peter Maydell wrote:
> Update the onenand device to use qemu_log_mask() for reporting
> guest errors and unimplemented features, rather than plain
> fprintf() and hw_error().
> 
> (We leave the hw_error() in onenand_reset(), as that is
> triggered by a failure to read the underlying block device
> for the bootRAM, not by guest action.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/block/onenand.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~

Re: [Qemu-devel] [PATCH for-3.1 2/2] hw/block/onenand: use qemu_log_mask() for reporting

[Thomas Huth]

On 2018-11-15 15:35, Peter Maydell wrote:
> Update the onenand device to use qemu_log_mask() for reporting
> guest errors and unimplemented features, rather than plain
> fprintf() and hw_error().
> 
> (We leave the hw_error() in onenand_reset(), as that is
> triggered by a failure to read the underlying block device
> for the bootRAM, not by guest action.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/block/onenand.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
Reviewed-by: Thomas Huth <thuth@redhat.com>