[PATCH v2 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[PATCH v2 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[Philippe Mathieu-Daudé]

Yet another bug in the sdcard model found by libfuzzer:
https://bugs.launchpad.net/bugs/1895310

Since RFC: Settled migration issue

Philippe Mathieu-Daudé (6):
  hw/sd/sdcard: Add trace event for ERASE command (CMD38)
  hw/sd/sdcard: Introduce the INVALID_ADDRESS definition
  hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS
  hw/sd/sdcard: Reset both start/end addresses on error
  hw/sd/sdcard: Do not attempt to erase out of range addresses
  hw/sd/sdcard: Assert if accessing an illegal group

 hw/sd/sd.c         | 30 ++++++++++++++++++++++--------
 hw/sd/trace-events |  2 +-
 2 files changed, 23 insertions(+), 9 deletions(-)

-- 
2.26.2

[PATCH v2 1/6] hw/sd/sdcard: Add trace event for ERASE command (CMD38)

[Philippe Mathieu-Daudé]

Trace addresses provided to the ERASE command.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c         | 2 +-
 hw/sd/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 00128822224..2606b969e34 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -749,7 +749,7 @@ static void sd_erase(SDState *sd)
     uint64_t erase_start = sd->erase_start;
     uint64_t erase_end = sd->erase_end;
 
-    trace_sdcard_erase();
+    trace_sdcard_erase(sd->erase_start, sd->erase_end);
     if (!sd->erase_start || !sd->erase_end) {
         sd->card_status |= ERASE_SEQ_ERROR;
         return;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index a87d7355fb8..96c7ea5e52f 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -46,7 +46,7 @@ sdcard_reset(void) ""
 sdcard_set_blocklen(uint16_t length) "0x%03x"
 sdcard_inserted(bool readonly) "read_only: %u"
 sdcard_ejected(void) ""
-sdcard_erase(void) ""
+sdcard_erase(uint32_t first, uint32_t last) "addr first 0x%" PRIx32" last 0x%" PRIx32
 sdcard_lock(void) ""
 sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-- 
2.26.2

[PATCH v2 2/6] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition

[Philippe Mathieu-Daudé]

'0' is used as a value to indicate an invalid (or unset)
address. Use a definition instead of a magic value.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 2606b969e34..30ae435d669 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -53,6 +53,8 @@
 
 #define SDSC_MAX_CAPACITY   (2 * GiB)
 
+#define INVALID_ADDRESS     0
+
 typedef enum {
     sd_r0 = 0,    /* no response */
     sd_r1,        /* normal response command */
@@ -575,8 +577,8 @@ static void sd_reset(DeviceState *dev)
     sd->wpgrps_size = sect;
     sd->wp_groups = bitmap_new(sd->wpgrps_size);
     memset(sd->function_group, 0, sizeof(sd->function_group));
-    sd->erase_start = 0;
-    sd->erase_end = 0;
+    sd->erase_start = INVALID_ADDRESS;
+    sd->erase_end = INVALID_ADDRESS;
     sd->size = size;
     sd->blk_len = 0x200;
     sd->pwd_len = 0;
@@ -750,7 +752,8 @@ static void sd_erase(SDState *sd)
     uint64_t erase_end = sd->erase_end;
 
     trace_sdcard_erase(sd->erase_start, sd->erase_end);
-    if (!sd->erase_start || !sd->erase_end) {
+    if (sd->erase_start == INVALID_ADDRESS
+            || sd->erase_end == INVALID_ADDRESS) {
         sd->card_status |= ERASE_SEQ_ERROR;
         return;
     }
@@ -763,8 +766,8 @@ static void sd_erase(SDState *sd)
 
     erase_start = sd_addr_to_wpnum(erase_start);
     erase_end = sd_addr_to_wpnum(erase_end);
-    sd->erase_start = 0;
-    sd->erase_end = 0;
+    sd->erase_start = INVALID_ADDRESS;
+    sd->erase_end = INVALID_ADDRESS;
     sd->csd[14] |= 0x40;
 
     for (i = erase_start; i <= erase_end; i++) {
-- 
2.26.2

[PATCH v2 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Philippe Mathieu-Daudé]

As it is legal to WRITE/ERASE the address/block 0,
change the value of this definition to an illegal
address: UINT32_MAX.

Unfortunately this break the migration stream, so
bump the VMState version number. This affects some
ARM boards and the SDHCI_PCI device (which is only
used for testing).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Kevin O'Connor <kevin@koconnor.net>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 30ae435d669..4c05152f189 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -53,7 +53,7 @@
 
 #define SDSC_MAX_CAPACITY   (2 * GiB)
 
-#define INVALID_ADDRESS     0
+#define INVALID_ADDRESS     UINT32_MAX
 
 typedef enum {
     sd_r0 = 0,    /* no response */
@@ -666,8 +666,8 @@ static int sd_vmstate_pre_load(void *opaque)
 
 static const VMStateDescription sd_vmstate = {
     .name = "sd-card",
-    .version_id = 1,
-    .minimum_version_id = 1,
+    .version_id = 2,
+    .minimum_version_id = 2,
     .pre_load = sd_vmstate_pre_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(mode, SDState),
-- 
2.26.2

[PATCH v2 4/6] hw/sd/sdcard: Reset both start/end addresses on error

[Philippe Mathieu-Daudé]

From the Spec "4.3.5 Erase":

  The host should adhere to the following command
  sequence: ERASE_WR_BLK_START, ERASE_WR_BLK_END and
  ERASE (CMD38).

  If an erase (CMD38) or address setting (CMD32, 33)
  command is received out of sequence, the card shall
  set the ERASE_SEQ_ERROR bit in the status register
  and reset the whole sequence.

Reset both addresses if the ERASE command occured
out of sequence (one of the start/end address is
not set).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4c05152f189..ee7b64023aa 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -755,6 +755,8 @@ static void sd_erase(SDState *sd)
     if (sd->erase_start == INVALID_ADDRESS
             || sd->erase_end == INVALID_ADDRESS) {
         sd->card_status |= ERASE_SEQ_ERROR;
+        sd->erase_start = INVALID_ADDRESS;
+        sd->erase_end = INVALID_ADDRESS;
         return;
     }
 
-- 
2.26.2

[PATCH v2 5/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[Philippe Mathieu-Daudé]

While the Spec v3 is not very clear, v6 states:

  If the host provides an out of range address as an argument
  to CMD32 or CMD33, the card shall indicate OUT_OF_RANGE error
  in R1 (ERX) for CMD38.

If an address is out of range, do not attempt to erase it:
return R1 with the error bit set.

Buglink: https://bugs.launchpad.net/bugs/1895310
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index ee7b64023aa..4454d168e2f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -766,6 +766,13 @@ static void sd_erase(SDState *sd)
         erase_end *= 512;
     }
 
+    if (sd->erase_start > sd->size || sd->erase_end > sd->size) {
+        sd->card_status |= OUT_OF_RANGE;
+        sd->erase_start = INVALID_ADDRESS;
+        sd->erase_end = INVALID_ADDRESS;
+        return;
+    }
+
     erase_start = sd_addr_to_wpnum(erase_start);
     erase_end = sd_addr_to_wpnum(erase_end);
     sd->erase_start = INVALID_ADDRESS;
-- 
2.26.2

[PATCH v2 6/6] hw/sd/sdcard: Assert if accessing an illegal group

[Philippe Mathieu-Daudé]

We can not have more group than 'wpgrps_size'.
Assert if we are accessing a group above this limit.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4454d168e2f..c3febed2434 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -780,6 +780,7 @@ static void sd_erase(SDState *sd)
     sd->csd[14] |= 0x40;
 
     for (i = erase_start; i <= erase_end; i++) {
+        assert(i < sd->wpgrps_size);
         if (test_bit(i, sd->wp_groups)) {
             sd->card_status |= WP_ERASE_SKIP;
         }
@@ -794,6 +795,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
     wpnum = sd_addr_to_wpnum(addr);
 
     for (i = 0; i < 32; i++, wpnum++, addr += WPGROUP_SIZE) {
+        assert(wpnum < sd->wpgrps_size);
         if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
             ret |= (1 << i);
         }
-- 
2.26.2

Re: [PATCH v2 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[Alexander Bulekov]

On 201015 0838, Philippe Mathieu-Daudé wrote:
> Yet another bug in the sdcard model found by libfuzzer:
> https://bugs.launchpad.net/bugs/1895310
> 
> Since RFC: Settled migration issue
> 
> Philippe Mathieu-Daudé (6):
>   hw/sd/sdcard: Add trace event for ERASE command (CMD38)
>   hw/sd/sdcard: Introduce the INVALID_ADDRESS definition
>   hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS
>   hw/sd/sdcard: Reset both start/end addresses on error
>   hw/sd/sdcard: Do not attempt to erase out of range addresses
>   hw/sd/sdcard: Assert if accessing an illegal group
> 
>  hw/sd/sd.c         | 30 ++++++++++++++++++++++--------
>  hw/sd/trace-events |  2 +-
>  2 files changed, 23 insertions(+), 9 deletions(-)
> 
> -- 
> 2.26.2
> 

Hi Phil,
For this series:
Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thanks
-Alex

Re: [PATCH v2 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[Philippe Mathieu-Daudé]

On 10/17/20 8:31 PM, Alexander Bulekov wrote:
> On 201015 0838, Philippe Mathieu-Daudé wrote:
>> Yet another bug in the sdcard model found by libfuzzer:
>> https://bugs.launchpad.net/bugs/1895310
>>
>> Since RFC: Settled migration issue
>>
>> Philippe Mathieu-Daudé (6):
>>    hw/sd/sdcard: Add trace event for ERASE command (CMD38)
>>    hw/sd/sdcard: Introduce the INVALID_ADDRESS definition
>>    hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS
>>    hw/sd/sdcard: Reset both start/end addresses on error
>>    hw/sd/sdcard: Do not attempt to erase out of range addresses
>>    hw/sd/sdcard: Assert if accessing an illegal group
>>
>>   hw/sd/sd.c         | 30 ++++++++++++++++++++++--------
>>   hw/sd/trace-events |  2 +-
>>   2 files changed, 23 insertions(+), 9 deletions(-)
>>
>> -- 
>> 2.26.2
>>
> 
> Hi Phil,
> For this series:
> Tested-by: Alexander Bulekov <alxndr@bu.edu>

Thanks!

Series applied to sd-next tree.