On 6/26/20 7:43 PM, Philippe Mathieu-Daudé wrote: > 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.mayd...@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <f4...@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);
No, the current code is correct... > >> + >> 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; ... but here this should be: sect = sd_addr_to_wpnum(sd, size - 1) + 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; >> >