[PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[Philippe Mathieu-Daudé]

Patches 2 & 3 fix CVE-2020-13253.
The rest are (accumulated) cleanups.

Supersedes: <20200604182502.24228-1-f4bug@amsat.org>

Philippe Mathieu-Daudé (11):
  MAINTAINERS: Cc qemu-block mailing list
  hw/sd/sdcard: Update coding style to make checkpatch.pl happy
  hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
  hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
  hw/sd/sdcard: Update the SDState documentation
  hw/sd/sdcard: Simplify cmd_valid_while_locked()
  hw/sd/sdcard: Constify sd_crc*()'s message argument
  hw/sd/sdcard: Make iolen unsigned
  hw/sd/sdcard: Correctly display the command name in trace events
  hw/sd/sdcard: Display offset in read/write_data() trace events
  hw/sd/sdcard: Simplify realize() a bit

 hw/sd/sd.c         | 122 +++++++++++++++++++++++++++++----------------
 MAINTAINERS        |   1 +
 hw/sd/trace-events |   4 +-
 3 files changed, 83 insertions(+), 44 deletions(-)

-- 
2.21.3

[PATCH v3 01/11] MAINTAINERS: Cc qemu-block mailing list

[Philippe Mathieu-Daudé]

We forgot to include the qemu-block mailing list while adding
this section in commit 076a0fc32a7. Fix this.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 MAINTAINERS | 1 +
 1 file changed, 1 insertion(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 3e7d9cb0a5..20b3a68d00 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1599,6 +1599,7 @@ F: hw/ssi/xilinx_*
 
 SD (Secure Card)
 M: Philippe Mathieu-Daudé <f4bug@amsat.org>
+L: qemu-block@nongnu.org
 S: Odd Fixes
 F: include/hw/sd/sd*
 F: hw/sd/core.c
-- 
2.21.3

[PATCH v3 02/11] hw/sd/sdcard: Update coding style to make checkpatch.pl happy

[Philippe Mathieu-Daudé]

To make the next commit easier to review, clean this code first.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 hw/sd/sd.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 3c06a0ac6d..f1b12b49db 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1154,8 +1154,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_start = addr;
             sd->data_offset = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+            }
             return sd_r1;
 
         default:
@@ -1170,8 +1171,9 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_start = addr;
             sd->data_offset = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+            }
             return sd_r1;
 
         default:
@@ -1216,12 +1218,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
-            if (sd_wp_addr(sd, sd->data_start))
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
-            if (sd->csd[14] & 0x30)
+            }
+            if (sd->csd[14] & 0x30) {
                 sd->card_status |= WP_VIOLATION;
+            }
             return sd_r1;
 
         default:
@@ -1240,12 +1245,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             sd->data_offset = 0;
             sd->blk_written = 0;
 
-            if (sd->data_start + sd->blk_len > sd->size)
+            if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
-            if (sd_wp_addr(sd, sd->data_start))
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
                 sd->card_status |= WP_VIOLATION;
-            if (sd->csd[14] & 0x30)
+            }
+            if (sd->csd[14] & 0x30) {
                 sd->card_status |= WP_VIOLATION;
+            }
             return sd_r1;
 
         default:
-- 
2.21.3

[PATCH v3 03/11] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Only move the state machine to ReceivingData if there is no
pending error.  This avoids later OOB access while processing
commands queued.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.3 Data Read

  Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

  4.3.4 Data Write

  Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

Fixes: CVE-2020-13253
Cc: Prasad J Pandit <pjp@fedoraproject.org>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Buglink: https://bugs.launchpad.net/qemu/+bug/1880822
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 64 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f1b12b49db..90d5ff6209 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1150,13 +1150,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 17:	/* CMD17:  READ_SINGLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->state = sd_sendingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
 
             if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
             }
+
+            sd->state = sd_sendingdata_state;
+            sd->data_start = addr;
+            sd->data_offset = 0;
             return sd_r1;
 
         default:
@@ -1167,13 +1169,15 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->state = sd_sendingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
 
             if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
             }
+
+            sd->state = sd_sendingdata_state;
+            sd->data_start = addr;
+            sd->data_offset = 0;
             return sd_r1;
 
         default:
@@ -1213,20 +1217,24 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (sd->data_start + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+            if (sd->csd[14] & 0x30) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            if (sd->csd[14] & 0x30) {
-                sd->card_status |= WP_VIOLATION;
-            }
             return sd_r1;
 
         default:
@@ -1240,20 +1248,24 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (sd->data_start + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+            if (sd->csd[14] & 0x30) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            if (sd->csd[14] & 0x30) {
-                sd->card_status |= WP_VIOLATION;
-            }
             return sd_r1;
 
         default:
-- 
2.21.3

[PATCH v3 04/11] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Only SCSD cards support Class 6 (Block Oriented Write Protection)
commands.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.14 Command Functional Difference in Card Capacity Types

  * Write Protected Group

  SDHC and SDXC do not support write-protected groups. Issuing
  CMD28, CMD29 and CMD30 generates the ILLEGAL_COMMAND error.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 90d5ff6209..4cc1ecf9f9 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -905,6 +905,11 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
         sd->multi_blk_cnt = 0;
     }
 
+    if (sd_cmd_class[req.cmd] == 6 && FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
+        /* Only Standard Capacity cards support class 6 commands */
+        return sd_illegal;
+    }
+
     switch (req.cmd) {
     /* Basic commands (Class 0 and Class 1) */
     case 0:	/* CMD0:   GO_IDLE_STATE */
-- 
2.21.3

[PATCH v3 05/11] hw/sd/sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Add more descriptive comments to keep a clear separation
between static property vs runtime changeable.

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4cc1ecf9f9..9bfd65ac34 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -92,11 +92,14 @@ struct SDState {
     uint32_t card_status;
     uint8_t sd_status[64];
 
-    /* Configurable properties */
+    /* Static properties */
+
     uint8_t spec_version;
     BlockBackend *blk;
     bool spi;
 
+    /* Runtime changeables */
+
     uint32_t mode;    /* current card mode, one of SDCardModes */
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t vhs;
-- 
2.21.3

[PATCH v3 06/11] hw/sd/sdcard: Simplify cmd_valid_while_locked()

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

cmd_valid_while_locked() only needs to read SDRequest->cmd,
pass it directly and make it const.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 9bfd65ac34..7799a3c621 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1647,7 +1647,7 @@ static sd_rsp_type_t sd_app_command(SDState *sd,
     return sd_illegal;
 }
 
-static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
+static int cmd_valid_while_locked(SDState *sd, const uint8_t cmd)
 {
     /* Valid commands in locked state:
      * basic class (0)
@@ -1658,13 +1658,12 @@ static int cmd_valid_while_locked(SDState *sd, SDRequest *req)
      * Anything else provokes an "illegal command" response.
      */
     if (sd->expecting_acmd) {
-        return req->cmd == 41 || req->cmd == 42;
+        return cmd == 41 || cmd == 42;
     }
-    if (req->cmd == 16 || req->cmd == 55) {
+    if (cmd == 16 || cmd == 55) {
         return 1;
     }
-    return sd_cmd_class[req->cmd] == 0
-            || sd_cmd_class[req->cmd] == 7;
+    return sd_cmd_class[cmd] == 0 || sd_cmd_class[cmd] == 7;
 }
 
 int sd_do_command(SDState *sd, SDRequest *req,
@@ -1690,7 +1689,7 @@ int sd_do_command(SDState *sd, SDRequest *req,
     }
 
     if (sd->card_status & CARD_IS_LOCKED) {
-        if (!cmd_valid_while_locked(sd, req)) {
+        if (!cmd_valid_while_locked(sd, req->cmd)) {
             sd->card_status |= ILLEGAL_COMMAND;
             sd->expecting_acmd = false;
             qemu_log_mask(LOG_GUEST_ERROR, "SD: Card is locked\n");
-- 
2.21.3

[PATCH v3 07/11] hw/sd/sdcard: Constify sd_crc*()'s message argument

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

CRC functions don't modify the buffer argument,
make it const.

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 7799a3c621..9d51138b11 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -242,11 +242,11 @@ static const int sd_cmd_class[SDMMC_CMD_MAX] = {
     7,  7, 10,  7,  9,  9,  9,  8,  8, 10,  8,  8,  8,  8,  8,  8,
 };
 
-static uint8_t sd_crc7(void *message, size_t width)
+static uint8_t sd_crc7(const void *message, size_t width)
 {
     int i, bit;
     uint8_t shift_reg = 0x00;
-    uint8_t *msg = (uint8_t *) message;
+    const uint8_t *msg = (const uint8_t *)message;
 
     for (i = 0; i < width; i ++, msg ++)
         for (bit = 7; bit >= 0; bit --) {
@@ -258,11 +258,11 @@ static uint8_t sd_crc7(void *message, size_t width)
     return shift_reg;
 }
 
-static uint16_t sd_crc16(void *message, size_t width)
+static uint16_t sd_crc16(const void *message, size_t width)
 {
     int i, bit;
     uint16_t shift_reg = 0x0000;
-    uint16_t *msg = (uint16_t *) message;
+    const uint16_t *msg = (const uint16_t *)message;
     width <<= 1;
 
     for (i = 0; i < width; i ++, msg ++)
-- 
2.21.3

[PATCH v3 08/11] hw/sd/sdcard: Make iolen unsigned

[Philippe Mathieu-Daudé]

I/O request length can not be negative.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
 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 9d51138b11..952be36399 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1950,7 +1950,7 @@ uint8_t sd_read_data(SDState *sd)
 {
     /* TODO: Append CRCs */
     uint8_t ret;
-    int io_len;
+    size_t io_len;
 
     if (!sd->blk || !blk_is_inserted(sd->blk) || !sd->enable)
         return 0x00;
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index 5f09d32eb2..f892c05867 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -52,7 +52,7 @@ sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
-sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, int length) "%s %20s/ CMD%02d len %d"
+sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, size_t length) "%s %20s/ CMD%02d len %zu"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # milkymist-memcard.c
-- 
2.21.3

[PATCH v3 09/11] hw/sd/sdcard: Correctly display the command name in trace events

[Philippe Mathieu-Daudé]

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;
     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 */
-- 
2.21.3

[PATCH v3 10/11] hw/sd/sdcard: Display offset in read/write_data() trace events

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Having 'base address' and 'relative offset' displayed
separately is more helpful than the absolute address.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index fad34ab184..a1b25ed36f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1824,8 +1824,8 @@ void sd_write_data(SDState *sd, uint8_t value)
         return;
 
     trace_sdcard_write_data(sd->proto_name,
-                            sd_current_cmd_name(sd),
-                            sd->current_cmd, value);
+                            sd_current_cmd_name(sd), sd->current_cmd,
+                            sd->data_start, sd->data_offset, value);
     switch (sd->current_cmd) {
     case 24:	/* CMD24:  WRITE_SINGLE_BLOCK */
         sd->data[sd->data_offset ++] = value;
@@ -1978,8 +1978,8 @@ 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_current_cmd_name(sd),
-                           sd->current_cmd, io_len);
+                           sd_current_cmd_name(sd), sd->current_cmd,
+                           sd->data_start, sd->data_offset, io_len);
     switch (sd->current_cmd) {
     case 6:	/* CMD6:   SWITCH_FUNCTION */
         ret = sd->data[sd->data_offset ++];
diff --git a/hw/sd/trace-events b/hw/sd/trace-events
index f892c05867..1529ad4c6d 100644
--- a/hw/sd/trace-events
+++ b/hw/sd/trace-events
@@ -51,8 +51,8 @@ sdcard_lock(void) ""
 sdcard_unlock(void) ""
 sdcard_read_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
 sdcard_write_block(uint64_t addr, uint32_t len) "addr 0x%" PRIx64 " size 0x%x"
-sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint8_t value) "%s %20s/ CMD%02d value 0x%02x"
-sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, size_t length) "%s %20s/ CMD%02d len %zu"
+sdcard_write_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint64_t address, uint32_t offset, uint8_t value) "%s %20s/ CMD%02d addr 0x%" PRIx64 " ofs 0x%" PRIx32 " val 0x%02x"
+sdcard_read_data(const char *proto, const char *cmd_desc, uint8_t cmd, uint64_t address, uint32_t offset, size_t length) "%s %20s/ CMD%02d addr 0x%" PRIx64 " ofs 0x%" PRIx32 " len %zu"
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # milkymist-memcard.c
-- 
2.21.3

[PATCH v3 11/11] hw/sd/sdcard: Simplify realize() a bit

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <f4bug@amsat.org>

We don't need to check if sd->blk is set twice.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index a1b25ed36f..060ca9d993 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2123,12 +2123,12 @@ static void sd_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    if (sd->blk && blk_is_read_only(sd->blk)) {
-        error_setg(errp, "Cannot use read-only drive as SD card");
-        return;
-    }
-
     if (sd->blk) {
+        if (blk_is_read_only(sd->blk)) {
+            error_setg(errp, "Cannot use read-only drive as SD card");
+            return;
+        }
+
         ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
                            BLK_PERM_ALL, errp);
         if (ret < 0) {
-- 
2.21.3

Re: [PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[Philippe Mathieu-Daudé]

Hi Alistair,

On 6/5/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> Patches 2 & 3 fix CVE-2020-13253.
> The rest are (accumulated) cleanups.
> 
> Supersedes: <20200604182502.24228-1-f4bug@amsat.org>
> 
> Philippe Mathieu-Daudé (11):
>   MAINTAINERS: Cc qemu-block mailing list
>   hw/sd/sdcard: Update coding style to make checkpatch.pl happy
>   hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
>   hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
>   hw/sd/sdcard: Update the SDState documentation
>   hw/sd/sdcard: Simplify cmd_valid_while_locked()
>   hw/sd/sdcard: Constify sd_crc*()'s message argument
>   hw/sd/sdcard: Make iolen unsigned
>   hw/sd/sdcard: Correctly display the command name in trace events
>   hw/sd/sdcard: Display offset in read/write_data() trace events
>   hw/sd/sdcard: Simplify realize() a bit

I forgot to Cc you.

Since you already reviewed a bunch of SD patches in the
past, do you mind having a look a this series? It should
be quite trivial.

Thanks!

Phil.

Re: [PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[Philippe Mathieu-Daudé]

On 6/5/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> Patches 2 & 3 fix CVE-2020-13253.

Ping for the CVE fix?...

> The rest are (accumulated) cleanups.
> 
> Supersedes: <20200604182502.24228-1-f4bug@amsat.org>
> 
> Philippe Mathieu-Daudé (11):
>   MAINTAINERS: Cc qemu-block mailing list
>   hw/sd/sdcard: Update coding style to make checkpatch.pl happy
>   hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
>   hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
>   hw/sd/sdcard: Update the SDState documentation
>   hw/sd/sdcard: Simplify cmd_valid_while_locked()
>   hw/sd/sdcard: Constify sd_crc*()'s message argument
>   hw/sd/sdcard: Make iolen unsigned
>   hw/sd/sdcard: Correctly display the command name in trace events
>   hw/sd/sdcard: Display offset in read/write_data() trace events
>   hw/sd/sdcard: Simplify realize() a bit
> 
>  hw/sd/sd.c         | 122 +++++++++++++++++++++++++++++----------------
>  MAINTAINERS        |   1 +
>  hw/sd/trace-events |   4 +-
>  3 files changed, 83 insertions(+), 44 deletions(-)
> 

Re: [PATCH v3 04/11] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:23, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Only SCSD cards support Class 6 (Block Oriented Write Protection)
> commands.
>
>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>
>   4.3.14 Command Functional Difference in Card Capacity Types
>
>   * Write Protected Group
>
>   SDHC and SDXC do not support write-protected groups. Issuing
>   CMD28, CMD29 and CMD30 generates the ILLEGAL_COMMAND error.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 02/11] hw/sd/sdcard: Update coding style to make checkpatch.pl happy

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> To make the next commit easier to review, clean this code first.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  hw/sd/sd.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 03/11] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Only move the state machine to ReceivingData if there is no
> pending error.  This avoids later OOB access while processing
> commands queued.
>
>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>
>   4.3.3 Data Read
>
>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>   occurred and no data transfer is performed.
>
>   4.3.4 Data Write
>
>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>   occurred and no data transfer is performed.

It's not clear from the spec that this should also
apply to WP_VIOLATION errors. The text about WP_VIOLATION
suggests that it is handled by aborting the data transfer
(ie set the error bit, stay in receive-data state, wait for
a stop command, but ignore all further data transfer),
which is I think distinct from "rejecting" the command.

If that theory is right then moving the check for the
ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
tests should stay as they are, I think.

NB: is the buffer overrun we're trying to protect against
caused by passing sd_wp_addr() a bad address? Maybe we
should assert in sd_addr_to_wpnum() that the address is
in range, as a defence.

thanks
-- PMM

Re: [PATCH v3 05/11] hw/sd/sdcard: Update the SDState documentation

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:22, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 06/11] hw/sd/sdcard: Simplify cmd_valid_while_locked()

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:27, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> cmd_valid_while_locked() only needs to read SDRequest->cmd,
> pass it directly and make it const.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 08/11] hw/sd/sdcard: Make iolen unsigned

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:27, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> I/O request length can not be negative.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
>  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 9d51138b11..952be36399 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1950,7 +1950,7 @@ uint8_t sd_read_data(SDState *sd)
>  {
>      /* TODO: Append CRCs */
>      uint8_t ret;
> -    int io_len;
> +    size_t io_len;

size_t seems an odd choice -- we initialize it with
    io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;

where sd->blk_len is a uint32_t, and we use it mainly with
            BLK_READ_BLOCK(sd->data_start, io_len);
where BLK_READ_BLOCK is a rather unnecessary macroization
of sd_blk_read(), which takes a uint32_t.

thanks
-- PMM

Re: [PATCH v3 09/11] hw/sd/sdcard: Correctly display the command name in trace events

[Peter Maydell]

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

Re: [PATCH v3 10/11] hw/sd/sdcard: Display offset in read/write_data() trace events

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:32, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Having 'base address' and 'relative offset' displayed
> separately is more helpful than the absolute address.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 11/11] hw/sd/sdcard: Simplify realize() a bit

[Peter Maydell]

On Fri, 5 Jun 2020 at 11:28, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> We don't need to check if sd->blk is set twice.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index a1b25ed36f..060ca9d993 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2123,12 +2123,12 @@ static void sd_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>
> -    if (sd->blk && blk_is_read_only(sd->blk)) {
> -        error_setg(errp, "Cannot use read-only drive as SD card");
> -        return;
> -    }
> -
>      if (sd->blk) {
> +        if (blk_is_read_only(sd->blk)) {
> +            error_setg(errp, "Cannot use read-only drive as SD card");
> +            return;
> +        }
> +
>          ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE,
>                             BLK_PERM_ALL, errp);
>          if (ret < 0) {

Originally written with the pattern of "check all the error cases
first; then do actual work". But if you prefer this way around

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[Alistair Francis]

On Mon, Jun 8, 2020 at 10:48 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> Hi Alistair,
>
> On 6/5/20 12:22 PM, Philippe Mathieu-Daudé wrote:
> > Patches 2 & 3 fix CVE-2020-13253.
> > The rest are (accumulated) cleanups.
> >
> > Supersedes: <20200604182502.24228-1-f4bug@amsat.org>
> >
> > Philippe Mathieu-Daudé (11):
> >   MAINTAINERS: Cc qemu-block mailing list
> >   hw/sd/sdcard: Update coding style to make checkpatch.pl happy
> >   hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
> >   hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
> >   hw/sd/sdcard: Update the SDState documentation
> >   hw/sd/sdcard: Simplify cmd_valid_while_locked()
> >   hw/sd/sdcard: Constify sd_crc*()'s message argument
> >   hw/sd/sdcard: Make iolen unsigned
> >   hw/sd/sdcard: Correctly display the command name in trace events
> >   hw/sd/sdcard: Display offset in read/write_data() trace events
> >   hw/sd/sdcard: Simplify realize() a bit
>
> I forgot to Cc you.
>
> Since you already reviewed a bunch of SD patches in the
> past, do you mind having a look a this series? It should
> be quite trivial.

Hey,

Sorry I took so long but I have reviewed a few. Let me know if there
are anymore you want reviewed.

Alistair

>
> Thanks!
>
> Phil.
>
>

Re: [PATCH v3 00/11] hw/sd/sdcard: Fix CVE-2020-13253 & cleanups

[Philippe Mathieu-Daudé]

On 7/6/20 6:31 PM, Alistair Francis wrote:
> On Mon, Jun 8, 2020 at 10:48 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> Hi Alistair,
>>
>> On 6/5/20 12:22 PM, Philippe Mathieu-Daudé wrote:
>>> Patches 2 & 3 fix CVE-2020-13253.
>>> The rest are (accumulated) cleanups.
>>>
>>> Supersedes: <20200604182502.24228-1-f4bug@amsat.org>
>>>
>>> Philippe Mathieu-Daudé (11):
>>>   MAINTAINERS: Cc qemu-block mailing list
>>>   hw/sd/sdcard: Update coding style to make checkpatch.pl happy
>>>   hw/sd/sdcard: Do not switch to ReceivingData if address is invalid
>>>   hw/sd/sdcard: Restrict Class 6 commands to SCSD cards
>>>   hw/sd/sdcard: Update the SDState documentation
>>>   hw/sd/sdcard: Simplify cmd_valid_while_locked()
>>>   hw/sd/sdcard: Constify sd_crc*()'s message argument
>>>   hw/sd/sdcard: Make iolen unsigned
>>>   hw/sd/sdcard: Correctly display the command name in trace events
>>>   hw/sd/sdcard: Display offset in read/write_data() trace events
>>>   hw/sd/sdcard: Simplify realize() a bit
>>
>> I forgot to Cc you.
>>
>> Since you already reviewed a bunch of SD patches in the
>> past, do you mind having a look a this series? It should
>> be quite trivial.
> 
> Hey,
> 
> Sorry I took so long but I have reviewed a few. Let me know if there
> are anymore you want reviewed.

No problem, just in time :)

Thank you a lot for all your reviews!

Phil.

> 
> Alistair

Re: [PATCH v3 03/11] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

On 6/15/20 4:06 PM, Peter Maydell wrote:
> On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Only move the state machine to ReceivingData if there is no
>> pending error.  This avoids later OOB access while processing
>> commands queued.
>>
>>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>>
>>   4.3.3 Data Read
>>
>>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>   occurred and no data transfer is performed.
>>
>>   4.3.4 Data Write
>>
>>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>   occurred and no data transfer is performed.
> 
> It's not clear from the spec that this should also
> apply to WP_VIOLATION errors. The text about WP_VIOLATION
> suggests that it is handled by aborting the data transfer
> (ie set the error bit, stay in receive-data state, wait for
> a stop command, but ignore all further data transfer),
> which is I think distinct from "rejecting" the command.
> 
> If that theory is right then moving the check for the
> ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
> tests should stay as they are, I think.

I found the correct behavior in table '4.10.1 Card Status':

* OUT_OF_RANGE
  ============
  Type: E R X

  The command's argument was out of the allowed range for this card.

* ADDRESS_ERROR
  =============
  Type: E R X

  A misaligned address which did not match the block length was
  used in the command.

* WP_VIOLATION
  ============
  Type: E R X

  Set when the host attempts to write to a protected block or to
  the temporary or permanent write protected card.

With 'Type':

- E: Error bit.
- R: Detected and set for the actual command response.
- X: Detected and set during command execution. The host can get
     the status by issuing a command with R1 response.

Block Read
==========
[...]
When the last block of user area is read using CMD18, the host should
ignore OUT_OF_RANGE error that may occur even the sequence is correct.
If the host uses partial blocks whose accumulated length is not block
aligned and block misalignment is not allowed, the card shall detect
a block misalignment at the beginning of the first misaligned block,
set the ADDRESS_ERROR error bit in the status register, abort
transmission and wait in the Data State for a stop command.


So I understand we want OUT_OF_RANGE (returned via R1).

Re: [PATCH v3 03/11] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

On 7/13/20 6:36 PM, Philippe Mathieu-Daudé wrote:
> On 6/15/20 4:06 PM, Peter Maydell wrote:
>> On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> Only move the state machine to ReceivingData if there is no
>>> pending error.  This avoids later OOB access while processing
>>> commands queued.
>>>
>>>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>>>
>>>   4.3.3 Data Read
>>>
>>>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>>   occurred and no data transfer is performed.
>>>
>>>   4.3.4 Data Write
>>>
>>>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>>   occurred and no data transfer is performed.
>>
>> It's not clear from the spec that this should also
>> apply to WP_VIOLATION errors. The text about WP_VIOLATION
>> suggests that it is handled by aborting the data transfer
>> (ie set the error bit, stay in receive-data state, wait for
>> a stop command, but ignore all further data transfer),
>> which is I think distinct from "rejecting" the command.
>>
>> If that theory is right then moving the check for the
>> ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
>> tests should stay as they are, I think.
> 
> I found the correct behavior in table '4.10.1 Card Status':
> 
> * OUT_OF_RANGE
>   ============
>   Type: E R X
> 
>   The command's argument was out of the allowed range for this card.
> 
> * ADDRESS_ERROR
>   =============
>   Type: E R X
> 
>   A misaligned address which did not match the block length was
>   used in the command.
> 
> * WP_VIOLATION
>   ============
>   Type: E R X
> 
>   Set when the host attempts to write to a protected block or to
>   the temporary or permanent write protected card.
> 
> With 'Type':
> 
> - E: Error bit.
> - R: Detected and set for the actual command response.
> - X: Detected and set during command execution. The host can get
>      the status by issuing a command with R1 response.
> 
> Block Read
> ==========
> [...]
> When the last block of user area is read using CMD18, the host should
> ignore OUT_OF_RANGE error that may occur even the sequence is correct.
> If the host uses partial blocks whose accumulated length is not block
> aligned and block misalignment is not allowed, the card shall detect
> a block misalignment at the beginning of the first misaligned block,
> set the ADDRESS_ERROR error bit in the status register, abort
> transmission and wait in the Data State for a stop command.
> 
> 
> So I understand we want OUT_OF_RANGE (returned via R1).

We always returned ADDRESS_ERROR and guests never complained, so I don't
plan to modify this bit for 5.1. What matters is "command is rejected
if error occurred and no data transfer is performed".