| Message ID | 20200605102230.21493-10-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/sd/sdcard: Fix CVE-2020-13253 & cleanups | expand |
On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > From: Philippe Mathieu-Daudé <f4bug@amsat.org> > > Some ACMD were incorrectly displayed. Fix by remembering if we > are processing a ACMD (with current_cmd_is_acmd) and add the > sd_current_cmd_name() helper, which display to correct name > regardless it is a CMD or ACMD. > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/sd/sd.c | 17 ++++++++++++++--- > 1 file changed, 14 insertions(+), 3 deletions(-) > > diff --git a/hw/sd/sd.c b/hw/sd/sd.c > index 952be36399..fad34ab184 100644 > --- a/hw/sd/sd.c > +++ b/hw/sd/sd.c > @@ -114,6 +114,7 @@ struct SDState { > uint8_t pwd[16]; > uint32_t pwd_len; > uint8_t function_group[6]; > + bool current_cmd_is_acmd; This is extra state, so strictly speaking it needs to be migrated (though the only thing we would get wrong is a possible wrong trace message after a migration load). > uint8_t current_cmd; > /* True if we will handle the next command as an ACMD. Note that this does > * *not* track the APP_CMD status bit! > @@ -1687,6 +1688,8 @@ int sd_do_command(SDState *sd, SDRequest *req, > req->cmd); > req->cmd &= 0x3f; > } > + sd->current_cmd = req->cmd; > + sd->current_cmd_is_acmd = sd->expecting_acmd; I'm not 100% sure about moving the update of sd->current_cmd down here -- if it's an illegal command that seems wrong. > if (sd->card_status & CARD_IS_LOCKED) { > if (!cmd_valid_while_locked(sd, req->cmd)) { > @@ -1714,7 +1717,6 @@ int sd_do_command(SDState *sd, SDRequest *req, > /* Valid command, we can update the 'state before command' bits. > * (Do this now so they appear in r1 responses.) > */ > - sd->current_cmd = req->cmd; > sd->card_status &= ~CURRENT_STATE; > sd->card_status |= (last_state << 9); > } > @@ -1775,6 +1777,15 @@ send_response: > return rsplen; > } thanks -- PMM
diff --git a/hw/sd/sd.c b/hw/sd/sd.c index 952be36399..fad34ab184 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -114,6 +114,7 @@ struct SDState { uint8_t pwd[16]; uint32_t pwd_len; uint8_t function_group[6]; + bool current_cmd_is_acmd; uint8_t current_cmd; /* True if we will handle the next command as an ACMD. Note that this does * *not* track the APP_CMD status bit! @@ -1687,6 +1688,8 @@ int sd_do_command(SDState *sd, SDRequest *req, req->cmd); req->cmd &= 0x3f; } + sd->current_cmd = req->cmd; + sd->current_cmd_is_acmd = sd->expecting_acmd; if (sd->card_status & CARD_IS_LOCKED) { if (!cmd_valid_while_locked(sd, req->cmd)) { @@ -1714,7 +1717,6 @@ int sd_do_command(SDState *sd, SDRequest *req, /* Valid command, we can update the 'state before command' bits. * (Do this now so they appear in r1 responses.) */ - sd->current_cmd = req->cmd; sd->card_status &= ~CURRENT_STATE; sd->card_status |= (last_state << 9); } @@ -1775,6 +1777,15 @@ send_response: return rsplen; } +static const char *sd_current_cmd_name(SDState *sd) +{ + if (sd->current_cmd_is_acmd) { + return sd_acmd_name(sd->current_cmd); + } else { + return sd_cmd_name(sd->current_cmd); + } +} + static void sd_blk_read(SDState *sd, uint64_t addr, uint32_t len) { trace_sdcard_read_block(addr, len); @@ -1813,7 +1824,7 @@ void sd_write_data(SDState *sd, uint8_t value) return; trace_sdcard_write_data(sd->proto_name, - sd_acmd_name(sd->current_cmd), + sd_current_cmd_name(sd), sd->current_cmd, value); switch (sd->current_cmd) { case 24: /* CMD24: WRITE_SINGLE_BLOCK */ @@ -1967,7 +1978,7 @@ uint8_t sd_read_data(SDState *sd) io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len; trace_sdcard_read_data(sd->proto_name, - sd_acmd_name(sd->current_cmd), + sd_current_cmd_name(sd), sd->current_cmd, io_len); switch (sd->current_cmd) { case 6: /* CMD6: SWITCH_FUNCTION */