Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses

[Cédric Le Goater]

On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> s->cur_addr can be made to point outside s->storage, either by
> writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
> is a signed integer and sign-extends into the 64-bit cur_addr),
> or just by writing an address beyond the size of the flash being
> emulated.  Avoid the sign extension to make the code cleaner, and
> on top of that mask s->cur_addr to s->size.
> 
> Signed-off-by: Paolo Bonzini <address@hidden>

Yes. We can trash memory very easily with the m25p80 object so we 
clearly want this fix.

Maybe, we could add a guest error when going beyond storage and 
the case doing flash_write8() needs some care also. Anyhow, that
can be covered in another patch.

Reviewed-by: Cédric Le Goater <address@hidden>

Thanks,

C. 

> ---
>  hw/block/m25p80.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index dd6714d..76a9bcf 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -586,18 +586,16 @@ static inline int get_addr_length(Flash *s)
>  
>  static void complete_collecting_data(Flash *s)
>  {
> -    int i;
> -
> -    s->cur_addr = 0;
> +    int i, n;
>  
> -    for (i = 0; i < get_addr_length(s); ++i) {
> +    n = get_addr_length(s);
> +    s->cur_addr = (n == 3 ? s->ear : 0);
> +    for (i = 0; i < n; ++i) {
>          s->cur_addr <<= 8;
>          s->cur_addr |= s->data[i];
>      }
>  
> -    if (get_addr_length(s) == 3) {
> -        s->cur_addr += s->ear * MAX_3BYTES_SIZE;
> -    }
> +    s->cur_addr &= s->size - 1;
>  
>      s->state = STATE_IDLE;
>  
> @@ -1099,14 +1097,14 @@ static uint32_t m25p80_transfer8(SSISlave *ss, 
> uint32_t tx)
>          DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
>                     s->cur_addr, (uint8_t)tx);
>          flash_write8(s, s->cur_addr, (uint8_t)tx);
> -        s->cur_addr++;
> +        s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
>          break;
>  
>      case STATE_READ:
>          r = s->storage[s->cur_addr];
>          DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
>                     (uint8_t)r);
> -        s->cur_addr = (s->cur_addr + 1) % s->size;
> +        s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
>          break;
>  
>      case STATE_COLLECTING_DATA:
>