Re: [Qemu-arm] [PATCH v1] highbank: validate register offset before access

[Philippe Mathieu-Daudé]

Hi Prasad,

Thanks for updating this patch so quickly :)

On 11/11/2017 05:11 AM, P J P wrote:
> From: Prasad J Pandit <address@hidden>
> 
> An 'offset' parameter sent to highbank register r/w functions
> could be greater than number(NUM_REGS=0x200) of hb registers,
> leading to an OOB access issue. Add check to avoid it.
> 
> Reported-by: Moguofang (Dennis mo) <address@hidden>
> Signed-off-by: Prasad J Pandit <address@hidden>
> ---
>  hw/arm/highbank.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> Update: log error message(via qemu_log_mask) before returning
>   -> https://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01914.html
> 
> diff --git a/hw/arm/highbank.c b/hw/arm/highbank.c
> index 354c6b25a8..8494dc6a48 100644
> --- a/hw/arm/highbank.c
> +++ b/hw/arm/highbank.c
> @@ -34,6 +34,7 @@
>  #include "hw/ide/ahci.h"
>  #include "hw/cpu/a9mpcore.h"
>  #include "hw/cpu/a15mpcore.h"
> +#include "qemu/log.h"
>  
>  #define SMP_BOOT_ADDR           0x100
>  #define SMP_BOOT_REG            0x40
> @@ -117,14 +118,26 @@ static void hb_regs_write(void *opaque, hwaddr offset,
>          }
>      }
>  
> +    if (offset / 4 >= NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "highbank: bad write offset 0x%x\n", (uint32_t)offset);

I'd rather use:

           "highbank: bad write offset 0x%" HWADDR_PRIx "\n", offset);

> +        return;
> +    }
>      regs[offset/4] = value;
>  }
>  
>  static uint64_t hb_regs_read(void *opaque, hwaddr offset,
>                               unsigned size)
>  {
> +    uint32_t value;
>      uint32_t *regs = opaque;
> -    uint32_t value = regs[offset/4];
> +
> +    if (offset / 4 >= NUM_REGS) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "highbank: bad read offset 0x%x\n", (uint32_t)offset);

Ditto HWADDR_PRIx.

> +        return 0;
> +    }
> +    value = regs[offset/4];

  +    value = regs[offset / 4];

With checkpatch fixed:
Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

>  
>      if ((offset == 0x100) || (offset == 0x108) || (offset == 0x10C)) {
>          value |= 0x30000000;
>