Re: [PATCH v5 08/15] hw/sd/sdcard: Check address is in range

[Philippe Mathieu-Daudé]

On 6/26/20 6:40 PM, Philippe Mathieu-Daudé wrote:
> As a defense, assert if the requested address is out of the card area.
> 
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 22392e5084..2689a27b49 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -537,8 +537,10 @@ static void sd_response_r7_make(SDState *sd, uint8_t 
> *response)
>      stl_be_p(response, sd->vhs);
>  }
>  
> -static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
> +static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>  {
> +    assert(addr < sd->size);

This should be:

       assert(addr <= sd->size);

> +
>      return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
>  }
>  
> @@ -575,7 +577,7 @@ static void sd_reset(DeviceState *dev)
>      sd_set_cardstatus(sd);
>      sd_set_sdstatus(sd);
>  
> -    sect = sd_addr_to_wpnum(size) + 1;
> +    sect = sd_addr_to_wpnum(sd, size) + 1;
>      g_free(sd->wp_groups);
>      sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
>      sd->wpgrps_size = sect;
> @@ -759,8 +761,8 @@ static void sd_erase(SDState *sd)
>          erase_end *= HWBLOCK_SIZE;
>      }
>  
> -    erase_start = sd_addr_to_wpnum(erase_start);
> -    erase_end = sd_addr_to_wpnum(erase_end);
> +    erase_start = sd_addr_to_wpnum(sd, erase_start);
> +    erase_end = sd_addr_to_wpnum(sd, erase_end);
>      sd->erase_start = 0;
>      sd->erase_end = 0;
>      sd->csd[14] |= 0x40;
> @@ -777,7 +779,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>      uint32_t i, wpnum;
>      uint32_t ret = 0;
>  
> -    wpnum = sd_addr_to_wpnum(addr);
> +    wpnum = sd_addr_to_wpnum(sd, addr);
>  
>      for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
>          if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
> @@ -819,7 +821,7 @@ static void sd_function_switch(SDState *sd, uint32_t arg)
>  
>  static inline bool sd_wp_addr(SDState *sd, uint64_t addr)
>  {
> -    return test_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +    return test_bit(sd_addr_to_wpnum(sd, addr), sd->wp_groups);
>  }
>  
>  static void sd_lock_command(SDState *sd)
> @@ -1331,7 +1333,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>              }
>  
>              sd->state = sd_programming_state;
> -            set_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +            set_bit(sd_addr_to_wpnum(sd, addr), sd->wp_groups);
>              /* Bzzzzzzztt .... Operation complete.  */
>              sd->state = sd_transfer_state;
>              return sd_r1b;
> @@ -1350,7 +1352,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, 
> SDRequest req)
>              }
>  
>              sd->state = sd_programming_state;
> -            clear_bit(sd_addr_to_wpnum(addr), sd->wp_groups);
> +            clear_bit(sd_addr_to_wpnum(sd, addr), sd->wp_groups);
>              /* Bzzzzzzztt .... Operation complete.  */
>              sd->state = sd_transfer_state;
>              return sd_r1b;
>