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

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

[Philippe Mathieu-Daudé]

Patches 5 & 6 fix CVE-2020-13253.
The rest are (accumulated) cleanups.

Since v6: Handle -ENOMEDIUM error
Since v5: Fix incorrect use of sd_addr_to_wpnum() in sd_reset()

Missing review:
[PATCH 01/15] MAINTAINERS: Cc qemu-block mailing list
[PATCH 03/15] hw/sd/sdcard: Move some definitions to use them
[PATCH 04/15] hw/sd/sdcard: Use the HWBLOCK_SIZE definition
[PATCH 05/15] hw/sd/sdcard: Do not switch to ReceivingData if
[PATCH 07/15] hw/sd/sdcard: Move sd->size initialization
[PATCH 08/15] hw/sd/sdcard: Call sd_addr_to_wpnum where used, consider zero size
[PATCH 09/15] hw/sd/sdcard: Special case the -ENOMEDIUM error
[PATCH 10/15] hw/sd/sdcard: Check address is in range
[PATCH 14/15] hw/sd/sdcard: Make iolen unsigned
[PATCH 15/15] hw/sd/sdcard: Correctly display the command name in trace

$ git backport-diff -u v6
$ git backport-diff -u sd_cve_2020_13253-v6 -r origin/master..
Key:
[----] : patches are identical
[####] : number of functional differences between upstream/downstream patch
[down] : patch is downstream-only
The flags [FC] indicate (F)unctional and (C)ontextual differences, respectively

001/17:[----] [--] 'MAINTAINERS: Cc qemu-block mailing list'
002/17:[----] [--] 'hw/sd/sdcard: Update coding style to make checkpatch.pl happy'
003/17:[----] [--] 'hw/sd/sdcard: Move some definitions to use them earlier'
004/17:[----] [--] 'hw/sd/sdcard: Use the HWBLOCK_SIZE definition'
005/17:[----] [--] 'hw/sd/sdcard: Do not switch to ReceivingData if address is invalid'
006/17:[----] [--] 'hw/sd/sdcard: Restrict Class 6 commands to SCSD cards'
007/17:[down] 'hw/sd/sdcard: Move sd->size initialization'
008/17:[down] 'hw/sd/sdcard: Call sd_addr_to_wpnum where it is used, consider zero size'
009/17:[down] 'hw/sd/sdcard: Special case the -ENOMEDIUM error'
010/17:[0004] [FC] 'hw/sd/sdcard: Check address is in range'
011/17:[----] [--] 'hw/sd/sdcard: Update the SDState documentation'
012/17:[----] [--] 'hw/sd/sdcard: Simplify cmd_valid_while_locked()'
013/17:[----] [--] 'hw/sd/sdcard: Constify sd_crc*()'s message argument'
014/17:[----] [--] 'hw/sd/sdcard: Make iolen unsigned'
015/17:[----] [--] 'hw/sd/sdcard: Correctly display the command name in trace events'
016/17:[----] [--] 'hw/sd/sdcard: Display offset in read/write_data() trace events'
017/17:[----] [--] 'hw/sd/sdcard: Simplify realize() a bit'

Philippe Mathieu-Daudé (17):
  MAINTAINERS: Cc qemu-block mailing list
  hw/sd/sdcard: Update coding style to make checkpatch.pl happy
  hw/sd/sdcard: Move some definitions to use them earlier
  hw/sd/sdcard: Use the HWBLOCK_SIZE definition
  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: Move sd->size initialization
  hw/sd/sdcard: Call sd_addr_to_wpnum where it is used, consider zero
    size
  hw/sd/sdcard: Special case the -ENOMEDIUM error
  hw/sd/sdcard: Check address is in range
  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         | 189 +++++++++++++++++++++++++++++----------------
 MAINTAINERS        |   1 +
 hw/sd/trace-events |   4 +-
 3 files changed, 124 insertions(+), 70 deletions(-)

-- 
2.21.3

[PATCH v7 01/17] MAINTAINERS: Cc qemu-block mailing list

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

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 dec252f38b..9ad876c4a7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1628,6 +1628,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 v7 02/17] hw/sd/sdcard: Update coding style to make checkpatch.pl happy

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 97a9d32964..cac8d7d828 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1170,8 +1170,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:
@@ -1186,8 +1187,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:
@@ -1232,12 +1234,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:
@@ -1256,12 +1261,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 v7 03/17] hw/sd/sdcard: Move some definitions to use them earlier

[Philippe Mathieu-Daudé]

Move some definitions to use them earlier.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index cac8d7d828..4816b4a462 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -80,6 +80,12 @@ enum SDCardStates {
     sd_disconnect_state,
 };
 
+#define HWBLOCK_SHIFT   9                       /* 512 bytes */
+#define SECTOR_SHIFT    5                       /* 16 kilobytes */
+#define WPGROUP_SHIFT   7                       /* 2 megs */
+#define CMULT_SHIFT     9                       /* 512 times HWBLOCK_SIZE */
+#define WPGROUP_SIZE    (1 << (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT))
+
 struct SDState {
     DeviceState parent_obj;
 
@@ -367,12 +373,6 @@ static void sd_set_cid(SDState *sd)
     sd->cid[15] = (sd_crc7(sd->cid, 15) << 1) | 1;
 }
 
-#define HWBLOCK_SHIFT	9			/* 512 bytes */
-#define SECTOR_SHIFT	5			/* 16 kilobytes */
-#define WPGROUP_SHIFT	7			/* 2 megs */
-#define CMULT_SHIFT	9			/* 512 times HWBLOCK_SIZE */
-#define WPGROUP_SIZE	(1 << (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT))
-
 static const uint8_t sd_csd_rw_mask[16] = {
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
     0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0xfc, 0xfe,
-- 
2.21.3

[PATCH v7 04/17] hw/sd/sdcard: Use the HWBLOCK_SIZE definition

[Philippe Mathieu-Daudé]

Replace the following different uses of the same value by
the same HWBLOCK_SIZE definition:
  - 512 (magic value)
  - 0x200 (magic value)
  - 1 << HWBLOCK_SHIFT

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 4816b4a462..04451fdad2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -81,6 +81,7 @@ enum SDCardStates {
 };
 
 #define HWBLOCK_SHIFT   9                       /* 512 bytes */
+#define HWBLOCK_SIZE    (1 << HWBLOCK_SHIFT)
 #define SECTOR_SHIFT    5                       /* 16 kilobytes */
 #define WPGROUP_SHIFT   7                       /* 2 megs */
 #define CMULT_SHIFT     9                       /* 512 times HWBLOCK_SIZE */
@@ -129,7 +130,7 @@ struct SDState {
     uint32_t blk_written;
     uint64_t data_start;
     uint32_t data_offset;
-    uint8_t data[512];
+    uint8_t data[HWBLOCK_SIZE];
     qemu_irq readonly_cb;
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
@@ -410,7 +411,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
             ((HWBLOCK_SHIFT << 6) & 0xc0);
         sd->csd[14] = 0x00;	/* File format group */
     } else {			/* SDHC */
-        size /= 512 * KiB;
+        size /= HWBLOCK_SIZE * KiB;
         size -= 1;
         sd->csd[0] = 0x40;
         sd->csd[1] = 0x0e;
@@ -574,7 +575,7 @@ static void sd_reset(DeviceState *dev)
     sd->erase_start = 0;
     sd->erase_end = 0;
     sd->size = size;
-    sd->blk_len = 0x200;
+    sd->blk_len = HWBLOCK_SIZE;
     sd->pwd_len = 0;
     sd->expecting_acmd = false;
     sd->dat_lines = 0xf;
@@ -685,7 +686,7 @@ static const VMStateDescription sd_vmstate = {
         VMSTATE_UINT32(blk_written, SDState),
         VMSTATE_UINT64(data_start, SDState),
         VMSTATE_UINT32(data_offset, SDState),
-        VMSTATE_UINT8_ARRAY(data, SDState, 512),
+        VMSTATE_UINT8_ARRAY(data, SDState, HWBLOCK_SIZE),
         VMSTATE_UNUSED_V(1, 512),
         VMSTATE_BOOL(enable, SDState),
         VMSTATE_END_OF_LIST()
@@ -754,8 +755,8 @@ static void sd_erase(SDState *sd)
 
     if (FIELD_EX32(sd->ocr, OCR, CARD_CAPACITY)) {
         /* High capacity memory card: erase units are 512 byte blocks */
-        erase_start *= 512;
-        erase_end *= 512;
+        erase_start *= HWBLOCK_SIZE;
+        erase_end *= HWBLOCK_SIZE;
     }
 
     erase_start = sd_addr_to_wpnum(erase_start);
@@ -1149,7 +1150,7 @@ static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 16:	/* CMD16:  SET_BLOCKLEN */
         switch (sd->state) {
         case sd_transfer_state:
-            if (req.arg > (1 << HWBLOCK_SHIFT)) {
+            if (req.arg > HWBLOCK_SIZE) {
                 sd->card_status |= BLOCK_LEN_ERROR;
             } else {
                 trace_sdcard_set_blocklen(req.arg);
@@ -1961,7 +1962,7 @@ uint8_t sd_read_data(SDState *sd)
     if (sd->card_status & (ADDRESS_ERROR | WP_VIOLATION))
         return 0x00;
 
-    io_len = (sd->ocr & (1 << 30)) ? 512 : sd->blk_len;
+    io_len = (sd->ocr & (1 << 30)) ? HWBLOCK_SIZE : sd->blk_len;
 
     trace_sdcard_read_data(sd->proto_name,
                            sd_acmd_name(sd->current_cmd),
-- 
2.21.3

[PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

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.

WP_VIOLATION errors are not modified: the error bit is set, we
stay in receive-data state, wait for a stop command. All further
data transfer is ignored. See the check on sd->card_status at the
beginning of sd_read_data() and sd_write_data().

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>
---
v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)
---
 hw/sd/sd.c | 34 ++++++++++++++++++++++------------
 1 file changed, 22 insertions(+), 12 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 04451fdad2..7e0d684aca 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1167,13 +1167,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:
@@ -1184,13 +1186,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:
@@ -1230,14 +1234,17 @@ 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;
+            }
+
             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;
             }
@@ -1257,14 +1264,17 @@ 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;
+            }
+
             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;
             }
-- 
2.21.3

[PATCH v7 06/17] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Philippe Mathieu-Daudé]

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.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 7e0d684aca..871c30a67f 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -922,6 +922,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 v7 07/17] hw/sd/sdcard: Move sd->size initialization

[Philippe Mathieu-Daudé]

Move sd->size initialization earlier to make the following
patches easier to review.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 871c30a67f..078b0e81ee 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -558,12 +558,13 @@ static void sd_reset(DeviceState *dev)
 
     sect = sd_addr_to_wpnum(size) + 1;
 
+    sd->size = size;
     sd->state = sd_idle_state;
     sd->rca = 0x0000;
     sd_set_ocr(sd);
     sd_set_scr(sd);
     sd_set_cid(sd);
-    sd_set_csd(sd, size);
+    sd_set_csd(sd, sd->size);
     sd_set_cardstatus(sd);
     sd_set_sdstatus(sd);
 
@@ -574,7 +575,6 @@ static void sd_reset(DeviceState *dev)
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = 0;
     sd->erase_end = 0;
-    sd->size = size;
     sd->blk_len = HWBLOCK_SIZE;
     sd->pwd_len = 0;
     sd->expecting_acmd = false;
-- 
2.21.3

[PATCH v7 08/17] hw/sd/sdcard: Call sd_addr_to_wpnum where it is used, consider zero size

[Philippe Mathieu-Daudé]

Avoid setting 'sect' variable just once (its name is confuse
anyway). Directly set 'sd->wpgrps_size'. Special case when
size is zero.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 078b0e81ee..e5adcc8055 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -556,8 +556,6 @@ static void sd_reset(DeviceState *dev)
     }
     size = sect << 9;
 
-    sect = sd_addr_to_wpnum(size) + 1;
-
     sd->size = size;
     sd->state = sd_idle_state;
     sd->rca = 0x0000;
@@ -570,7 +568,11 @@ static void sd_reset(DeviceState *dev)
 
     g_free(sd->wp_groups);
     sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
-    sd->wpgrps_size = sect;
+    if (sd->size) {
+        sd->wpgrps_size = sd_addr_to_wpnum(sd, sd->size) + 1;
+    } else {
+        sd->wpgrps_size = 1;
+    }
     sd->wp_groups = bitmap_new(sd->wpgrps_size);
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = 0;
-- 
2.21.3

[PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

As we have no interest in the underlying block geometry,
directly call blk_getlength(). We have to care about machines
creating SD card with not drive attached (probably incorrect
API use). Simply emit a warning when such Frankenstein cards
of zero size are reset.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index e5adcc8055..548745614e 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -545,18 +545,30 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr)
 static void sd_reset(DeviceState *dev)
 {
     SDState *sd = SD_CARD(dev);
-    uint64_t size;
-    uint64_t sect;
 
     trace_sdcard_reset();
     if (sd->blk) {
-        blk_get_geometry(sd->blk, &sect);
-    } else {
-        sect = 0;
-    }
-    size = sect << 9;
+        int64_t size = blk_getlength(sd->blk);
+
+        if (size == -ENOMEDIUM) {
+            /*
+             * FIXME blk should be set once per device in sd_realize(),
+             * and we shouldn't be checking it in sed_reset(). But this
+             * is how the reparent currently works.
+             */
+            char *id = object_get_canonical_path_component(OBJECT(dev));
+
+            warn_report("sd-card '%s' created with no drive.",
+                        id ? id : "unknown");
+            g_free(id);
+            size = 0;
+        }
+        assert(size >= 0);
+        sd->size = size;
+    } else {
+        sd->size = 0;
+    }
 
-    sd->size = size;
     sd->state = sd_idle_state;
     sd->rca = 0x0000;
     sd_set_ocr(sd);
-- 
2.21.3

[PATCH v7 10/17] hw/sd/sdcard: Check address is in range

[Philippe Mathieu-Daudé]

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 | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 548745614e..5d1b314a32 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);
+
     return addr >> (HWBLOCK_SHIFT + SECTOR_SHIFT + WPGROUP_SHIFT);
 }
 
@@ -773,8 +775,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;
@@ -791,7 +793,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)) {
@@ -833,7 +835,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)
@@ -1345,7 +1347,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;
@@ -1364,7 +1366,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;
-- 
2.21.3

[PATCH v7 11/17] hw/sd/sdcard: Update the SDState documentation

[Philippe Mathieu-Daudé]

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

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Reviewed-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 5d1b314a32..723e66bbf2 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -103,11 +103,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 v7 12/17] hw/sd/sdcard: Simplify cmd_valid_while_locked()

[Philippe Mathieu-Daudé]

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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 723e66bbf2..2946fe3040 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1678,7 +1678,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)
@@ -1689,13 +1689,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,
@@ -1721,7 +1720,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 v7 13/17] hw/sd/sdcard: Constify sd_crc*()'s message argument

[Philippe Mathieu-Daudé]

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 2946fe3040..364a6d1fcd 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -255,11 +255,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 --) {
@@ -271,11 +271,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 v7 14/17] hw/sd/sdcard: Make iolen unsigned

[Philippe Mathieu-Daudé]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

I/O request length can not be negative.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v4: Use uint32_t (pm215)
---
 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 364a6d1fcd..3e9faa8add 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1981,7 +1981,7 @@ uint8_t sd_read_data(SDState *sd)
 {
     /* TODO: Append CRCs */
     uint8_t ret;
-    int io_len;
+    uint32_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..d0cd7c6ec4 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, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # milkymist-memcard.c
-- 
2.21.3

[PATCH v7 15/17] hw/sd/sdcard: Correctly display the command name in trace events

[Philippe Mathieu-Daudé]

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 3e9faa8add..eb549a52e1 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -125,6 +125,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!
@@ -1718,6 +1719,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)) {
@@ -1745,7 +1748,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);
     }
@@ -1806,6 +1808,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);
@@ -1844,7 +1855,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 */
@@ -1998,7 +2009,7 @@ uint8_t sd_read_data(SDState *sd)
     io_len = (sd->ocr & (1 << 30)) ? HWBLOCK_SIZE : 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 v7 16/17] hw/sd/sdcard: Display offset in read/write_data() trace events

[Philippe Mathieu-Daudé]

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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 eb549a52e1..304fa4143a 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1855,8 +1855,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;
@@ -2009,8 +2009,8 @@ uint8_t sd_read_data(SDState *sd)
     io_len = (sd->ocr & (1 << 30)) ? HWBLOCK_SIZE : 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 d0cd7c6ec4..946923223b 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, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
+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, uint32_t length) "%s %20s/ CMD%02d addr 0x%" PRIx64 " ofs 0x%" PRIx32 " len %" PRIu32
 sdcard_set_voltage(uint16_t millivolts) "%u mV"
 
 # milkymist-memcard.c
-- 
2.21.3

[PATCH v7 17/17] hw/sd/sdcard: Simplify realize() a bit

[Philippe Mathieu-Daudé]

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

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
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 304fa4143a..8ef6715665 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2154,12 +2154,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 v7 03/17] hw/sd/sdcard: Move some definitions to use them earlier

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Move some definitions to use them earlier.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v7 04/17] hw/sd/sdcard: Use the HWBLOCK_SIZE definition

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Replace the following different uses of the same value by
> the same HWBLOCK_SIZE definition:
>   - 512 (magic value)
>   - 0x200 (magic value)
>   - 1 << HWBLOCK_SHIFT
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

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

thanks
-- PMM

Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
>
> WP_VIOLATION errors are not modified: the error bit is set, we
> stay in receive-data state, wait for a stop command. All further
> data transfer is ignored. See the check on sd->card_status at the
> beginning of sd_read_data() and sd_write_data().
>
> 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>
> ---
> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)

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

thanks
-- PMM

Re: [PATCH v7 07/17] hw/sd/sdcard: Move sd->size initialization

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Move sd->size initialization earlier to make the following
> patches easier to review.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 4 ++--

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

thanks
-- PMM

Re: [PATCH v7 08/17] hw/sd/sdcard: Call sd_addr_to_wpnum where it is used, consider zero size

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Avoid setting 'sect' variable just once (its name is confuse
> anyway). Directly set 'sd->wpgrps_size'. Special case when
> size is zero.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 078b0e81ee..e5adcc8055 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -556,8 +556,6 @@ static void sd_reset(DeviceState *dev)
>      }
>      size = sect << 9;
>
> -    sect = sd_addr_to_wpnum(size) + 1;
> -
>      sd->size = size;
>      sd->state = sd_idle_state;
>      sd->rca = 0x0000;
> @@ -570,7 +568,11 @@ static void sd_reset(DeviceState *dev)
>
>      g_free(sd->wp_groups);
>      sd->wp_switch = sd->blk ? blk_is_read_only(sd->blk) : false;
> -    sd->wpgrps_size = sect;
> +    if (sd->size) {
> +        sd->wpgrps_size = sd_addr_to_wpnum(sd, sd->size) + 1;
> +    } else {
> +        sd->wpgrps_size = 1;
> +    }

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

thanks
-- PMM

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> As we have no interest in the underlying block geometry,
> directly call blk_getlength(). We have to care about machines
> creating SD card with not drive attached (probably incorrect
> API use). Simply emit a warning when such Frankenstein cards
> of zero size are reset.

Which machines create SD cards without a backing block device?

I have a feeling that also the monitor "change" and "eject"
commands can remove the backing block device from the SD card
object.

thanks
-- PMM

Re: [PATCH v7 10/17] hw/sd/sdcard: Check address is in range

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> 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>
> --

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

thanks
-- PMM

Re: [PATCH v7 13/17] hw/sd/sdcard: Constify sd_crc*()'s message argument

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

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

thanks
-- PMM

Re: [PATCH v7 14/17] hw/sd/sdcard: Make iolen unsigned

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> I/O request length can not be negative.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---

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

thanks
-- PMM

Re: [PATCH v7 15/17] hw/sd/sdcard: Correctly display the command name in trace events

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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>

I think my comments on v3 of this patch still apply:
https://patchew.org/QEMU/20200605102230.21493-1-philmd@redhat.com/20200605102230.21493-10-philmd@redhat.com/

thanks
-- PMM

Re: [PATCH v7 01/17] MAINTAINERS: Cc qemu-block mailing list

[Peter Maydell]

On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> 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 +
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH v7 15/17] hw/sd/sdcard: Correctly display the command name in trace events

[Philippe Mathieu-Daudé]

On 7/3/20 3:28 PM, Peter Maydell wrote:
> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> 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>
> 
> I think my comments on v3 of this patch still apply:
> https://patchew.org/QEMU/20200605102230.21493-1-philmd@redhat.com/20200605102230.21493-10-philmd@redhat.com/

I agree with your comments. I didn't addressed them because for some
unknown reason this mail ended tagged as spam. I'll address that now.

Thanks for reviewing the rest of this series!

> 
> thanks
> -- PMM
> 

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/3/20 3:23 PM, Peter Maydell wrote:
> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> As we have no interest in the underlying block geometry,
>> directly call blk_getlength(). We have to care about machines
>> creating SD card with not drive attached (probably incorrect
>> API use). Simply emit a warning when such Frankenstein cards
>> of zero size are reset.
> 
> Which machines create SD cards without a backing block device?

The Aspeed machines:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html

> I have a feeling that also the monitor "change" and "eject"
> commands can remove the backing block device from the SD card
> object.

This is what I wanted to talk about on IRC. This seems wrong to me,
we should eject the card and destroy it, and recreate a new card
when plugging in another backing block device.

Keep the reparenting on the bus layer, not on the card.

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
> On 7/3/20 3:23 PM, Peter Maydell wrote:
>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> As we have no interest in the underlying block geometry,
>>> directly call blk_getlength(). We have to care about machines
>>> creating SD card with not drive attached (probably incorrect
>>> API use). Simply emit a warning when such Frankenstein cards
>>> of zero size are reset.
>>
>> Which machines create SD cards without a backing block device?
> 
> The Aspeed machines:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
> 
>> I have a feeling that also the monitor "change" and "eject"
>> commands can remove the backing block device from the SD card
>> object.
> 
> This is what I wanted to talk about on IRC. This seems wrong to me,
> we should eject the card and destroy it, and recreate a new card
> when plugging in another backing block device.
> 
> Keep the reparenting on the bus layer, not on the card.

I was wrong, the current code is correct:

void sdbus_reparent_card(SDBus *from, SDBus *to)
{
    SDState *card = get_card(from);
    SDCardClass *sc;
    bool readonly;

    /* We directly reparent the card object rather than implementing this
     * as a hotpluggable connection because we don't want to expose SD cards
     * to users as being hotpluggable, and we can get away with it in this
     * limited use case. This could perhaps be implemented more cleanly in
     * future by adding support to the hotplug infrastructure for "device
     * can be hotplugged only via code, not by user".
     */

    if (!card) {
        return;
    }

    sc = SD_CARD_GET_CLASS(card);
    readonly = sc->get_readonly(card);

    sdbus_set_inserted(from, false);
    qdev_set_parent_bus(DEVICE(card), &to->qbus);
    sdbus_set_inserted(to, true);
    sdbus_set_readonly(to, readonly);
}

What I don't understand is why create a sdcard with no block backend.

Maybe this is old code before the null-co block backend existed? I
haven't checked the git history yet.

I'll try to restrict sdcard with only block backend and see if
something break (I doubt) at least it simplifies the code.
But I need to update the Aspeed machines first.

The problem when not using block backend, is the size is 0,
so the next patch abort in sd_reset() due to:

  static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
  {
      assert(addr < sd->size);

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>
>>>> As we have no interest in the underlying block geometry,
>>>> directly call blk_getlength(). We have to care about machines
>>>> creating SD card with not drive attached (probably incorrect
>>>> API use). Simply emit a warning when such Frankenstein cards
>>>> of zero size are reset.
>>>
>>> Which machines create SD cards without a backing block device?
>>
>> The Aspeed machines:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html

Also all boards using:

hw/sd/milkymist-memcard.c:278:    /* FIXME use a qdev drive property
instead of drive_get_next() */
hw/sd/pl181.c:506:    /* FIXME use a qdev drive property instead of
drive_get_next() */
hw/sd/ssi-sd.c:253:    /* FIXME use a qdev drive property instead of
drive_get_next() */

I.e.:

static void pl181_realize(DeviceState *dev, Error **errp)
{
    PL181State *s = PL181(dev);
    DriveInfo *dinfo;

    /* FIXME use a qdev drive property instead of drive_get_next() */
    dinfo = drive_get_next(IF_SD);
    s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
    if (s->card == NULL) {
        error_setg(errp, "sd_init failed");
    }
}



>>
>>> I have a feeling that also the monitor "change" and "eject"
>>> commands can remove the backing block device from the SD card
>>> object.
>>
>> This is what I wanted to talk about on IRC. This seems wrong to me,
>> we should eject the card and destroy it, and recreate a new card
>> when plugging in another backing block device.
>>
>> Keep the reparenting on the bus layer, not on the card.
> 
> I was wrong, the current code is correct:
> 
> void sdbus_reparent_card(SDBus *from, SDBus *to)
> {
>     SDState *card = get_card(from);
>     SDCardClass *sc;
>     bool readonly;
> 
>     /* We directly reparent the card object rather than implementing this
>      * as a hotpluggable connection because we don't want to expose SD cards
>      * to users as being hotpluggable, and we can get away with it in this
>      * limited use case. This could perhaps be implemented more cleanly in
>      * future by adding support to the hotplug infrastructure for "device
>      * can be hotplugged only via code, not by user".
>      */
> 
>     if (!card) {
>         return;
>     }
> 
>     sc = SD_CARD_GET_CLASS(card);
>     readonly = sc->get_readonly(card);
> 
>     sdbus_set_inserted(from, false);
>     qdev_set_parent_bus(DEVICE(card), &to->qbus);
>     sdbus_set_inserted(to, true);
>     sdbus_set_readonly(to, readonly);
> }
> 
> What I don't understand is why create a sdcard with no block backend.
> 
> Maybe this is old code before the null-co block backend existed? I
> haven't checked the git history yet.
> 
> I'll try to restrict sdcard with only block backend and see if
> something break (I doubt) at least it simplifies the code.
> But I need to update the Aspeed machines first.
> 
> The problem when not using block backend, is the size is 0,
> so the next patch abort in sd_reset() due to:
> 
>   static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>   {
>       assert(addr < sd->size);
> 

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
>> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>>
>>>>> As we have no interest in the underlying block geometry,
>>>>> directly call blk_getlength(). We have to care about machines
>>>>> creating SD card with not drive attached (probably incorrect
>>>>> API use). Simply emit a warning when such Frankenstein cards
>>>>> of zero size are reset.
>>>>
>>>> Which machines create SD cards without a backing block device?
>>>
>>> The Aspeed machines:
>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
> 
> Also all boards using:
> 
> hw/sd/milkymist-memcard.c:278:    /* FIXME use a qdev drive property
> instead of drive_get_next() */
> hw/sd/pl181.c:506:    /* FIXME use a qdev drive property instead of
> drive_get_next() */
> hw/sd/ssi-sd.c:253:    /* FIXME use a qdev drive property instead of
> drive_get_next() */
> 
> I.e.:
> 
> static void pl181_realize(DeviceState *dev, Error **errp)
> {
>     PL181State *s = PL181(dev);
>     DriveInfo *dinfo;
> 
>     /* FIXME use a qdev drive property instead of drive_get_next() */
>     dinfo = drive_get_next(IF_SD);
>     s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>     if (s->card == NULL) {
>         error_setg(errp, "sd_init failed");
>     }
> }

Doh I was pretty sure this series was merged:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html

Time to respin I guess, addressing your comment...
https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html

> 
>>>
>>>> I have a feeling that also the monitor "change" and "eject"
>>>> commands can remove the backing block device from the SD card
>>>> object.
>>>
>>> This is what I wanted to talk about on IRC. This seems wrong to me,
>>> we should eject the card and destroy it, and recreate a new card
>>> when plugging in another backing block device.
>>>
>>> Keep the reparenting on the bus layer, not on the card.
>>
>> I was wrong, the current code is correct:
>>
>> void sdbus_reparent_card(SDBus *from, SDBus *to)
>> {
>>     SDState *card = get_card(from);
>>     SDCardClass *sc;
>>     bool readonly;
>>
>>     /* We directly reparent the card object rather than implementing this
>>      * as a hotpluggable connection because we don't want to expose SD cards
>>      * to users as being hotpluggable, and we can get away with it in this
>>      * limited use case. This could perhaps be implemented more cleanly in
>>      * future by adding support to the hotplug infrastructure for "device
>>      * can be hotplugged only via code, not by user".
>>      */
>>
>>     if (!card) {
>>         return;
>>     }
>>
>>     sc = SD_CARD_GET_CLASS(card);
>>     readonly = sc->get_readonly(card);
>>
>>     sdbus_set_inserted(from, false);
>>     qdev_set_parent_bus(DEVICE(card), &to->qbus);
>>     sdbus_set_inserted(to, true);
>>     sdbus_set_readonly(to, readonly);
>> }
>>
>> What I don't understand is why create a sdcard with no block backend.
>>
>> Maybe this is old code before the null-co block backend existed? I
>> haven't checked the git history yet.
>>
>> I'll try to restrict sdcard with only block backend and see if
>> something break (I doubt) at least it simplifies the code.
>> But I need to update the Aspeed machines first.
>>
>> The problem when not using block backend, is the size is 0,
>> so the next patch abort in sd_reset() due to:
>>
>>   static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>>   {
>>       assert(addr < sd->size);
>>
> 
> 

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/5/20 12:18 AM, Philippe Mathieu-Daudé wrote:
> On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
>> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
>>> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>>>
>>>>>> As we have no interest in the underlying block geometry,
>>>>>> directly call blk_getlength(). We have to care about machines
>>>>>> creating SD card with not drive attached (probably incorrect
>>>>>> API use). Simply emit a warning when such Frankenstein cards
>>>>>> of zero size are reset.
>>>>>
>>>>> Which machines create SD cards without a backing block device?
>>>>
>>>> The Aspeed machines:
>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
>>
>> Also all boards using:
>>
>> hw/sd/milkymist-memcard.c:278:    /* FIXME use a qdev drive property
>> instead of drive_get_next() */
>> hw/sd/pl181.c:506:    /* FIXME use a qdev drive property instead of
>> drive_get_next() */
>> hw/sd/ssi-sd.c:253:    /* FIXME use a qdev drive property instead of
>> drive_get_next() */
>>
>> I.e.:
>>
>> static void pl181_realize(DeviceState *dev, Error **errp)
>> {
>>     PL181State *s = PL181(dev);
>>     DriveInfo *dinfo;
>>
>>     /* FIXME use a qdev drive property instead of drive_get_next() */
>>     dinfo = drive_get_next(IF_SD);
>>     s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>>     if (s->card == NULL) {
>>         error_setg(errp, "sd_init failed");
>>     }
>> }
> 
> Doh I was pretty sure this series was merged:
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html
> 
> Time to respin I guess, addressing your comment...
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html

Not straight forward at first glance, so probably too late for soft
freeze.

Meanwhile patches 1-8 are reviewed and sufficient to fix
CVE-2020-13253. The problematic patch is #9, your "Check address is
in range" suggestion. Patches 11-14 are also reviewed and can go in.

Peter, can you consider taking them via your ARM queue? If you prefer
I can prepare a pull request.

I'll keep working on this during the next dev window.

Thanks,

Phil.

> 
>>
>>>>
>>>>> I have a feeling that also the monitor "change" and "eject"
>>>>> commands can remove the backing block device from the SD card
>>>>> object.
>>>>
>>>> This is what I wanted to talk about on IRC. This seems wrong to me,
>>>> we should eject the card and destroy it, and recreate a new card
>>>> when plugging in another backing block device.
>>>>
>>>> Keep the reparenting on the bus layer, not on the card.
>>>
>>> I was wrong, the current code is correct:
>>>
>>> void sdbus_reparent_card(SDBus *from, SDBus *to)
>>> {
>>>     SDState *card = get_card(from);
>>>     SDCardClass *sc;
>>>     bool readonly;
>>>
>>>     /* We directly reparent the card object rather than implementing this
>>>      * as a hotpluggable connection because we don't want to expose SD cards
>>>      * to users as being hotpluggable, and we can get away with it in this
>>>      * limited use case. This could perhaps be implemented more cleanly in
>>>      * future by adding support to the hotplug infrastructure for "device
>>>      * can be hotplugged only via code, not by user".
>>>      */
>>>
>>>     if (!card) {
>>>         return;
>>>     }
>>>
>>>     sc = SD_CARD_GET_CLASS(card);
>>>     readonly = sc->get_readonly(card);
>>>
>>>     sdbus_set_inserted(from, false);
>>>     qdev_set_parent_bus(DEVICE(card), &to->qbus);
>>>     sdbus_set_inserted(to, true);
>>>     sdbus_set_readonly(to, readonly);
>>> }
>>>
>>> What I don't understand is why create a sdcard with no block backend.
>>>
>>> Maybe this is old code before the null-co block backend existed? I
>>> haven't checked the git history yet.
>>>
>>> I'll try to restrict sdcard with only block backend and see if
>>> something break (I doubt) at least it simplifies the code.
>>> But I need to update the Aspeed machines first.
>>>
>>> The problem when not using block backend, is the size is 0,
>>> so the next patch abort in sd_reset() due to:
>>>
>>>   static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>>>   {
>>>       assert(addr < sd->size);
>>>
>>
>>
> 

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/5/20 12:26 AM, Philippe Mathieu-Daudé wrote:
> On 7/5/20 12:18 AM, Philippe Mathieu-Daudé wrote:
>> On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
>>> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>>>>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>>>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>>>>>
>>>>>>> As we have no interest in the underlying block geometry,
>>>>>>> directly call blk_getlength(). We have to care about machines
>>>>>>> creating SD card with not drive attached (probably incorrect
>>>>>>> API use). Simply emit a warning when such Frankenstein cards
>>>>>>> of zero size are reset.
>>>>>>
>>>>>> Which machines create SD cards without a backing block device?
>>>>>
>>>>> The Aspeed machines:
>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
>>>
>>> Also all boards using:
>>>
>>> hw/sd/milkymist-memcard.c:278:    /* FIXME use a qdev drive property
>>> instead of drive_get_next() */
>>> hw/sd/pl181.c:506:    /* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>> hw/sd/ssi-sd.c:253:    /* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>>
>>> I.e.:
>>>
>>> static void pl181_realize(DeviceState *dev, Error **errp)
>>> {
>>>     PL181State *s = PL181(dev);
>>>     DriveInfo *dinfo;
>>>
>>>     /* FIXME use a qdev drive property instead of drive_get_next() */
>>>     dinfo = drive_get_next(IF_SD);
>>>     s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>>>     if (s->card == NULL) {
>>>         error_setg(errp, "sd_init failed");
>>>     }
>>> }
>>
>> Doh I was pretty sure this series was merged:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html
>>
>> Time to respin I guess, addressing your comment...
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
> 
> Not straight forward at first glance, so probably too late for soft
> freeze.

I looked in backups of my previous computer and found the branch with
your comment addressed... Not sure why I never send the series, but
this explains why I was sure it was already fixed.

> 
> Meanwhile patches 1-8 are reviewed and sufficient to fix
> CVE-2020-13253. The problematic patch is #9, your "Check address is
> in range" suggestion. Patches 11-14 are also reviewed and can go in.
> 
> Peter, can you consider taking them via your ARM queue? If you prefer
> I can prepare a pull request.
> 
> I'll keep working on this during the next dev window.
> 
> Thanks,
> 
> Phil.
> 
>>
>>>
>>>>>
>>>>>> I have a feeling that also the monitor "change" and "eject"
>>>>>> commands can remove the backing block device from the SD card
>>>>>> object.
>>>>>
>>>>> This is what I wanted to talk about on IRC. This seems wrong to me,
>>>>> we should eject the card and destroy it, and recreate a new card
>>>>> when plugging in another backing block device.
>>>>>
>>>>> Keep the reparenting on the bus layer, not on the card.
>>>>
>>>> I was wrong, the current code is correct:
>>>>
>>>> void sdbus_reparent_card(SDBus *from, SDBus *to)
>>>> {
>>>>     SDState *card = get_card(from);
>>>>     SDCardClass *sc;
>>>>     bool readonly;
>>>>
>>>>     /* We directly reparent the card object rather than implementing this
>>>>      * as a hotpluggable connection because we don't want to expose SD cards
>>>>      * to users as being hotpluggable, and we can get away with it in this
>>>>      * limited use case. This could perhaps be implemented more cleanly in
>>>>      * future by adding support to the hotplug infrastructure for "device
>>>>      * can be hotplugged only via code, not by user".
>>>>      */
>>>>
>>>>     if (!card) {
>>>>         return;
>>>>     }
>>>>
>>>>     sc = SD_CARD_GET_CLASS(card);
>>>>     readonly = sc->get_readonly(card);
>>>>
>>>>     sdbus_set_inserted(from, false);
>>>>     qdev_set_parent_bus(DEVICE(card), &to->qbus);
>>>>     sdbus_set_inserted(to, true);
>>>>     sdbus_set_readonly(to, readonly);
>>>> }
>>>>
>>>> What I don't understand is why create a sdcard with no block backend.
>>>>
>>>> Maybe this is old code before the null-co block backend existed? I
>>>> haven't checked the git history yet.
>>>>
>>>> I'll try to restrict sdcard with only block backend and see if
>>>> something break (I doubt) at least it simplifies the code.
>>>> But I need to update the Aspeed machines first.
>>>>
>>>> The problem when not using block backend, is the size is 0,
>>>> so the next patch abort in sd_reset() due to:
>>>>
>>>>   static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>>>>   {
>>>>       assert(addr < sd->size);
>>>>
>>>
>>>
>>
> 

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> As we have no interest in the underlying block geometry,
>> directly call blk_getlength(). We have to care about machines
>> creating SD card with not drive attached (probably incorrect
>> API use). Simply emit a warning when such Frankenstein cards
>> of zero size are reset.
>
> Which machines create SD cards without a backing block device?
>
> I have a feeling that also the monitor "change" and "eject"
> commands can remove the backing block device from the SD card
> object.

Correct:

    static const BlockDevOps sd_block_ops = {
        .change_media_cb = sd_cardchange,
    };

This is TYPE_SD_CARD's ("sd-card").  What exactly does that device
model?

If it's the sd-card, then the modelling is odd.  A physical SD card gets
plugged and unplugged as a whole.  This model can't.  Instead, you
change "media".  Isn't the SD card the medium?

The other device models with removable media (IDE & SCSI CD drives,
floppy drives) model the receptacle for media.  On media change, the
drive stays put, and only the medium changes, both in the physical world
and in our virtual world.

Our "sd-card" seems to be an "SD card drive".  But then I wonder what
the thing at the other end of TYPE_SD_BUS ("sd-bus") actually models.
Any ideas?

Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Peter Maydell]

On Mon, 6 Jul 2020 at 06:52, Markus Armbruster <armbru@redhat.com> wrote:
> This is TYPE_SD_CARD's ("sd-card").  What exactly does that device
> model?

It is I think supposed to be an SD card. The modelling
may well be odd -- it's a qomification of a pre-existing
abstraction layer that predates QOM and qbus.

> If it's the sd-card, then the modelling is odd.  A physical SD card gets
> plugged and unplugged as a whole.  This model can't.  Instead, you
> change "media".  Isn't the SD card the medium?

I suspect this is because the requirement to change SD
card contents existed and was implemented via changing
the block backend before the concept of a hot-pluggable
QOM object even existed. So now we have something we
wish to maintain back-compat for (in terms of monitor
commands to change/eject doing what they've always done),
some SD controllers which haven't been entirely QOMified
(I see Philippe just posted a series to do that cleanup,
which is great), and some parts of the code which have been
updated but by people (ie me) who understand the QOM parts
of things but don't have any understanding of the operations
the block layer provides and so converted the device/SD code's
API to the rest of QEMU but left its interaction with the block
layer using the same APIs that the pre-QOM code used.

> The other device models with removable media (IDE & SCSI CD drives,
> floppy drives) model the receptacle for media.  On media change, the
> drive stays put, and only the medium changes, both in the physical world
> and in our virtual world.
>
> Our "sd-card" seems to be an "SD card drive".  But then I wonder what
> the thing at the other end of TYPE_SD_BUS ("sd-bus") actually models.
> Any ideas?

The thing at the other end of the TYPE_SD_BUS is the SD controller
(ie the bit of hardware which presents a registers-and-interrupts
interface to the system on one end and has an SD card slot on the
other end). The link between them has operations like "do command",
"read data", "write data", "get readonly status", which are abstractions
of the h/w operations on the pins between an SD controller
and the SD card. It also has an operation for "tell the controller
I'm actually an inserted card". So the "sd-card" device implements
the logic that in real h/w really is inside the microcontroller
on the SD card (and so is identical for all SD cards regardless
of machine type), and the sd controller device implements the logic
that's in the sd controller chip in the machine proper (which
can vary a lot between machines, from very-simple "software
does all the work and the controller just waves the wires up and
down" to much more sophisticated setups).

thanks
-- PMM

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

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:40 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> To make the next commit easier to review, clean this code first.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 97a9d32964..cac8d7d828 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1170,8 +1170,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:
> @@ -1186,8 +1187,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:
> @@ -1232,12 +1234,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:
> @@ -1256,12 +1261,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
>
>

Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:42 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
>
> WP_VIOLATION errors are not modified: the error bit is set, we
> stay in receive-data state, wait for a stop command. All further
> data transfer is ignored. See the check on sd->card_status at the
> beginning of sd_read_data() and sd_write_data().
>
> 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>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)
> ---
>  hw/sd/sd.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 04451fdad2..7e0d684aca 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1167,13 +1167,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:
> @@ -1184,13 +1186,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:
> @@ -1230,14 +1234,17 @@ 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;
> +            }
> +
>              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;
>              }
> @@ -1257,14 +1264,17 @@ 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;
> +            }
> +
>              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;
>              }
> --
> 2.21.3
>
>

Re: [PATCH v7 06/17] hw/sd/sdcard: Restrict Class 6 commands to SCSD cards

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/sd/sd.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 7e0d684aca..871c30a67f 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -922,6 +922,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
>
>

Re: [PATCH v7 11/17] hw/sd/sdcard: Update the SDState documentation

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:45 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Add more descriptive comments to keep a clear separation
> between static property vs runtime changeable.
>
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 5d1b314a32..723e66bbf2 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -103,11 +103,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
>
>

Re: [PATCH v7 12/17] hw/sd/sdcard: Simplify cmd_valid_while_locked()

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> cmd_valid_while_locked() only needs to read SDRequest->cmd,
> pass it directly and make it const.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 723e66bbf2..2946fe3040 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1678,7 +1678,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)
> @@ -1689,13 +1689,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,
> @@ -1721,7 +1720,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
>
>

Re: [PATCH v7 14/17] hw/sd/sdcard: Make iolen unsigned

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:51 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> From: Philippe Mathieu-Daudé <philmd@redhat.com>
>
> I/O request length can not be negative.
>
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
> v4: Use uint32_t (pm215)
> ---
>  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 364a6d1fcd..3e9faa8add 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1981,7 +1981,7 @@ uint8_t sd_read_data(SDState *sd)
>  {
>      /* TODO: Append CRCs */
>      uint8_t ret;
> -    int io_len;
> +    uint32_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..d0cd7c6ec4 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, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
>
>  # milkymist-memcard.c
> --
> 2.21.3
>
>

Re: [PATCH v7 16/17] hw/sd/sdcard: Display offset in read/write_data() trace events

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:44 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Having 'base address' and 'relative offset' displayed
> separately is more helpful than the absolute address.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 eb549a52e1..304fa4143a 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1855,8 +1855,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;
> @@ -2009,8 +2009,8 @@ uint8_t sd_read_data(SDState *sd)
>      io_len = (sd->ocr & (1 << 30)) ? HWBLOCK_SIZE : 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 d0cd7c6ec4..946923223b 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, uint32_t length) "%s %20s/ CMD%02d len %" PRIu32
> +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, uint32_t length) "%s %20s/ CMD%02d addr 0x%" PRIx64 " ofs 0x%" PRIx32 " len %" PRIu32
>  sdcard_set_voltage(uint16_t millivolts) "%u mV"
>
>  # milkymist-memcard.c
> --
> 2.21.3
>
>

Re: [PATCH v7 17/17] hw/sd/sdcard: Simplify realize() a bit

[Alistair Francis]

On Tue, Jun 30, 2020 at 6:46 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> We don't need to check if sd->blk is set twice.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  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 304fa4143a..8ef6715665 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2154,12 +2154,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 v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

On Tue, Jun 30, 2020 at 3:39 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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.
>
> WP_VIOLATION errors are not modified: the error bit is set, we
> stay in receive-data state, wait for a stop command. All further
> data transfer is ignored. See the check on sd->card_status at the
> beginning of sd_read_data() and sd_write_data().
>
> 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>
> ---
> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)
> ---
>  hw/sd/sd.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 04451fdad2..7e0d684aca 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -1167,13 +1167,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:
> @@ -1184,13 +1186,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;

Unfortunately this breaks guests (Linux at least) when sd->size is not
a power of 2,
as Linux doesn't expect unrealistic SD card sizes.

>              }
> +
> +            sd->state = sd_sendingdata_state;
> +            sd->data_start = addr;
> +            sd->data_offset = 0;
>              return sd_r1;
>
>          default:
> @@ -1230,14 +1234,17 @@ 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;
> +            }
> +
>              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;
>              }
> @@ -1257,14 +1264,17 @@ 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;
> +            }
> +
>              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;
>              }
> --
> 2.21.3
>

Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

On 7/7/20 10:30 AM, Philippe Mathieu-Daudé wrote:
> On Tue, Jun 30, 2020 at 3:39 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> 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.
>>
>> WP_VIOLATION errors are not modified: the error bit is set, we
>> stay in receive-data state, wait for a stop command. All further
>> data transfer is ignored. See the check on sd->card_status at the
>> beginning of sd_read_data() and sd_write_data().
>>
>> 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>
>> ---
>> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)
>> ---
>>  hw/sd/sd.c | 34 ++++++++++++++++++++++------------
>>  1 file changed, 22 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 04451fdad2..7e0d684aca 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -1167,13 +1167,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:
>> @@ -1184,13 +1186,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;
> 
> Unfortunately this breaks guests (Linux at least) when sd->size is not
> a power of 2,
> as Linux doesn't expect unrealistic SD card sizes.

I can use blk_truncate() to expand the image (which must be RW anyway)
to the ceil pow2 with something like:

-- >8 --
diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index b44999c864..052934f867 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -2121,11 +2121,28 @@ static void sd_realize(DeviceState *dev, Error
**errp)
     }

     if (sd->blk) {
+        int64_t blk_size;
+
         if (blk_is_read_only(sd->blk)) {
             error_setg(errp, "Cannot use read-only drive as SD card");
             return;
         }

+        blk_size = blk_getlength(sd->blk);
+        if (blk_size > 0) {
+            int64_t blk_size_aligned = pow2ceil(blk_size);
+
+            if (blk_size != blk_size_aligned) {
+                ret = blk_truncate(sd->blk, blk_size_aligned, false,
+                                   PREALLOC_MODE_FALLOC,
+                                   BDRV_REQ_ZERO_WRITE, errp);
+                if (ret < 0) {
+                    error_prepend(errp, "Could not resize image: ");
+                    return;
+                }
+            }
+        }
+
         ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ |
BLK_PERM_WRITE,
                            BLK_PERM_ALL, errp);
         if (ret < 0) {
---

> 
>>              }
>> +
>> +            sd->state = sd_sendingdata_state;
>> +            sd->data_start = addr;
>> +            sd->data_offset = 0;
>>              return sd_r1;
>>
>>          default:
>> @@ -1230,14 +1234,17 @@ 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;
>> +            }
>> +
>>              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;
>>              }
>> @@ -1257,14 +1264,17 @@ 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;
>> +            }
>> +
>>              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;
>>              }
>> --
>> 2.21.3
>>

Re: [PATCH v7 05/17] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

[Philippe Mathieu-Daudé]

On 7/7/20 12:24 PM, Philippe Mathieu-Daudé wrote:
> On 7/7/20 10:30 AM, Philippe Mathieu-Daudé wrote:
>> On Tue, Jun 30, 2020 at 3:39 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>>
>>> 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.
>>>
>>> WP_VIOLATION errors are not modified: the error bit is set, we
>>> stay in receive-data state, wait for a stop command. All further
>>> data transfer is ignored. See the check on sd->card_status at the
>>> beginning of sd_read_data() and sd_write_data().
>>>
>>> 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>
>>> ---
>>> v4: Only modify ADDRESS_ERROR, not WP_VIOLATION (pm215)
>>> ---
>>>  hw/sd/sd.c | 34 ++++++++++++++++++++++------------
>>>  1 file changed, 22 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>>> index 04451fdad2..7e0d684aca 100644
>>> --- a/hw/sd/sd.c
>>> +++ b/hw/sd/sd.c
>>> @@ -1167,13 +1167,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:
>>> @@ -1184,13 +1186,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;
>>
>> Unfortunately this breaks guests (Linux at least) when sd->size is not
>> a power of 2,
>> as Linux doesn't expect unrealistic SD card sizes.

I'll go with Peter's suggestion from IRC:
"insist the blk device is the right size and make it an error if not".

> 
> I can use blk_truncate() to expand the image (which must be RW anyway)
> to the ceil pow2 with something like:
> 
> -- >8 --
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index b44999c864..052934f867 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -2121,11 +2121,28 @@ static void sd_realize(DeviceState *dev, Error
> **errp)
>      }
> 
>      if (sd->blk) {
> +        int64_t blk_size;
> +
>          if (blk_is_read_only(sd->blk)) {
>              error_setg(errp, "Cannot use read-only drive as SD card");
>              return;
>          }
> 
> +        blk_size = blk_getlength(sd->blk);
> +        if (blk_size > 0) {
> +            int64_t blk_size_aligned = pow2ceil(blk_size);
> +
> +            if (blk_size != blk_size_aligned) {
> +                ret = blk_truncate(sd->blk, blk_size_aligned, false,
> +                                   PREALLOC_MODE_FALLOC,
> +                                   BDRV_REQ_ZERO_WRITE, errp);
> +                if (ret < 0) {
> +                    error_prepend(errp, "Could not resize image: ");
> +                    return;
> +                }
> +            }
> +        }
> +
>          ret = blk_set_perm(sd->blk, BLK_PERM_CONSISTENT_READ |
> BLK_PERM_WRITE,
>                             BLK_PERM_ALL, errp);
>          if (ret < 0) {
> ---