All of lore.kernel.org
 help / color / mirror / Atom feed
* [PULL 00/13] SD/MMC patches for 2020-10-21
@ 2020-10-21 17:34 Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string Philippe Mathieu-Daudé
                   ` (13 more replies)
  0 siblings, 14 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

The following changes since commit ac793156f650ae2d77834932d72224175ee69086:

  Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2020102=
0-1' into staging (2020-10-20 21:11:35 +0100)

are available in the Git repository at:

  https://gitlab.com/philmd/qemu.git tags/sd-next-20201021

for you to fetch changes up to 84816fb63e5c57159b469a66052d1b2bc862ef77:

  hw/sd/sdcard: Assert if accessing an illegal group (2020-10-21 13:34:27 +02=
00)

----------------------------------------------------------------
SD/MMC patches

Fix two heap-overflow reported by Alexander Bulekov while fuzzing:
- https://bugs.launchpad.net/qemu/+bug/1892960
- https://bugs.launchpad.net/qemu/+bug/1895310

CI jobs results:
. https://cirrus-ci.com/build/6399328187056128
. https://gitlab.com/philmd/qemu/-/pipelines/205701966
. https://travis-ci.org/github/philmd/qemu/builds/737708930
----------------------------------------------------------------

Philippe Mathieu-Daud=C3=A9 (13):
  hw/sd/sdhci: Fix qemu_log_mask() format string
  hw/sd/sdhci: Document the datasheet used
  hw/sd/sdhci: Fix DMA Transfer Block Size field
  hw/sd/sdhci: Stop multiple transfers when block count is cleared
  hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses
  hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered
  hw/sd/sdhci: Yield if interrupt delivered during multiple transfer
  hw/sd/sdcard: Add trace event for ERASE command (CMD38)
  hw/sd/sdcard: Introduce the INVALID_ADDRESS definition
  hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS
  hw/sd/sdcard: Reset both start/end addresses on error
  hw/sd/sdcard: Do not attempt to erase out of range addresses
  hw/sd/sdcard: Assert if accessing an illegal group

 hw/sd/sd.c         | 30 ++++++++++++++++++++++--------
 hw/sd/sdhci.c      | 41 +++++++++++++++++++++++++++++++++++------
 hw/sd/trace-events |  2 +-
 3 files changed, 58 insertions(+), 15 deletions(-)

--=20
2.26.2



^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PULL 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 02/13] hw/sd/sdhci: Document the datasheet used Philippe Mathieu-Daudé
                   ` (12 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Richard Henderson, Philippe Mathieu-Daudé

Add missing newline character in qemu_log_mask() format.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200901140411.112150-2-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 69002130839..6d4603f5b19 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1112,7 +1112,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         /* Limit block size to the maximum buffer size */
         if (extract32(s->blksize, 0, 12) > s->buf_maxsz) {
             qemu_log_mask(LOG_GUEST_ERROR, "%s: Size 0x%x is larger than "
-                          "the maximum buffer 0x%x", __func__, s->blksize,
+                          "the maximum buffer 0x%x\n", __func__, s->blksize,
                           s->buf_maxsz);
 
             s->blksize = deposit32(s->blksize, 0, 12, s->buf_maxsz);
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 02/13] hw/sd/sdhci: Document the datasheet used
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 03/13] hw/sd/sdhci: Fix DMA Transfer Block Size field Philippe Mathieu-Daudé
                   ` (11 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Richard Henderson, Philippe Mathieu-Daudé

Add datasheet name in the file header.

We can not add the direct download link since there is a disclaimers
to agree first on the SD Association website (www.sdcard.org).

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200901140411.112150-3-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 6d4603f5b19..ed717ab549b 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1,6 +1,8 @@
 /*
  * SD Association Host Standard Specification v2.0 controller emulation
  *
+ * Datasheet: PartA2_SD_Host_Controller_Simplified_Specification_Ver2.00.pdf
+ *
  * Copyright (c) 2011 Samsung Electronics Co., Ltd.
  * Mitsyanko Igor <i.mitsyanko@samsung.com>
  * Peter A.G. Crosthwaite <peter.crosthwaite@petalogix.com>
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 03/13] hw/sd/sdhci: Fix DMA Transfer Block Size field
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 02/13] hw/sd/sdhci: Document the datasheet used Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 04/13] hw/sd/sdhci: Stop multiple transfers when block count is cleared Philippe Mathieu-Daudé
                   ` (10 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel
  Cc: Alexander Bulekov, Prasad J Pandit, Philippe Mathieu-Daudé,
	qemu-stable

The 'Transfer Block Size' field is 12-bit wide.

See section '2.2.2. Block Size Register (Offset 004h)' in datasheet.

Two different bug reproducer available:
- https://bugs.launchpad.net/qemu/+bug/1892960
- https://ruhr-uni-bochum.sciebo.de/s/NNWP2GfwzYKeKwE?path=%2Fsdhci_oob_write1

Cc: qemu-stable@nongnu.org
Buglink: https://bugs.launchpad.net/qemu/+bug/1892960
Fixes: d7dfca0807a ("hw/sdhci: introduce standard SD host controller")
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Prasad J Pandit <pjp@fedoraproject.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200901140411.112150-3-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index ed717ab549b..c849c3d99b8 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -1107,7 +1107,7 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         break;
     case SDHC_BLKSIZE:
         if (!TRANSFERRING_DATA(s->prnsts)) {
-            MASKED_WRITE(s->blksize, mask, value);
+            MASKED_WRITE(s->blksize, mask, extract32(value, 0, 12));
             MASKED_WRITE(s->blkcnt, mask >> 16, value >> 16);
         }
 
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 04/13] hw/sd/sdhci: Stop multiple transfers when block count is cleared
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 03/13] hw/sd/sdhci: Fix DMA Transfer Block Size field Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 05/13] hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses Philippe Mathieu-Daudé
                   ` (9 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

Clearing BlockCount stops multiple transfers.

See "SD Host Controller Simplified Specification Version 2.00":

- 2.2.3. Block Count Register (Offset 006h)
- Table 2-8 : Determination of Transfer Type

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200903172806.489710-2-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index c849c3d99b8..61e469bd32f 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -731,6 +731,12 @@ static void sdhci_do_adma(SDHCIState *s)
     ADMADescr dscr = {};
     int i;
 
+    if (s->trnmod & SDHC_TRNS_BLK_CNT_EN && !s->blkcnt) {
+        /* Stop Multiple Transfer */
+        sdhci_end_transfer(s);
+        return;
+    }
+
     for (i = 0; i < SDHC_ADMA_DESCS_PER_DELAY; ++i) {
         s->admaerr &= ~SDHC_ADMAERR_LENGTH_MISMATCH;
 
@@ -756,7 +762,6 @@ static void sdhci_do_adma(SDHCIState *s)
 
         switch (dscr.attr & SDHC_ADMA_ATTR_ACT_MASK) {
         case SDHC_ADMA_ATTR_ACT_TRAN:  /* data transfer */
-
             if (s->trnmod & SDHC_TRNS_READ) {
                 while (length) {
                     if (s->data_count == 0) {
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 05/13] hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 04/13] hw/sd/sdhci: Stop multiple transfers when block count is cleared Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 06/13] hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered Philippe Mathieu-Daudé
                   ` (8 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

If we have pending DMA requests scheduled, process them first.
So far we don't need to implement a bottom half to process them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200903172806.489710-3-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 61e469bd32f..4db77decf87 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -948,11 +948,21 @@ sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
     return true;
 }
 
+static void sdhci_resume_pending_transfer(SDHCIState *s)
+{
+    timer_del(s->transfer_timer);
+    sdhci_data_transfer(s);
+}
+
 static uint64_t sdhci_read(void *opaque, hwaddr offset, unsigned size)
 {
     SDHCIState *s = (SDHCIState *)opaque;
     uint32_t ret = 0;
 
+    if (timer_pending(s->transfer_timer)) {
+        sdhci_resume_pending_transfer(s);
+    }
+
     switch (offset & ~0x3) {
     case SDHC_SYSAD:
         ret = s->sdmasysad;
@@ -1096,6 +1106,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
     uint32_t value = val;
     value <<= shift;
 
+    if (timer_pending(s->transfer_timer)) {
+        sdhci_resume_pending_transfer(s);
+    }
+
     switch (offset & ~0x3) {
     case SDHC_SYSAD:
         s->sdmasysad = (s->sdmasysad & mask) | value;
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 06/13] hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 05/13] hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 07/13] hw/sd/sdhci: Yield if interrupt delivered during multiple transfer Philippe Mathieu-Daudé
                   ` (7 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200903172806.489710-4-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 4db77decf87..b93ecefd20c 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -218,9 +218,14 @@ static uint8_t sdhci_slotint(SDHCIState *s)
          ((s->norintsts & SDHC_NIS_REMOVE) && (s->wakcon & SDHC_WKUP_ON_RMV));
 }
 
-static inline void sdhci_update_irq(SDHCIState *s)
+/* Return true if IRQ was pending and delivered */
+static bool sdhci_update_irq(SDHCIState *s)
 {
-    qemu_set_irq(s->irq, sdhci_slotint(s));
+    bool pending = sdhci_slotint(s);
+
+    qemu_set_irq(s->irq, pending);
+
+    return pending;
 }
 
 static void sdhci_raise_insertion_irq(void *opaque)
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 07/13] hw/sd/sdhci: Yield if interrupt delivered during multiple transfer
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 06/13] hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 08/13] hw/sd/sdcard: Add trace event for ERASE command (CMD38) Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

The Descriptor Table has a bit to allow the DMA to generates
Interrupt when the operation of the descriptor line is completed
(see "1.13.4. Descriptor Table" of 'SD Host Controller Simplified
Specification Version 2.00').

If we have pending interrupt and the descriptor requires it
to be generated as soon as it is completed, reschedule pending
transfers and yield to the CPU.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20200903172806.489710-5-f4bug@amsat.org>
---
 hw/sd/sdhci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index b93ecefd20c..2f8b74a84f7 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -837,7 +837,10 @@ static void sdhci_do_adma(SDHCIState *s)
                 s->norintsts |= SDHC_NIS_DMA;
             }
 
-            sdhci_update_irq(s);
+            if (sdhci_update_irq(s) && !(dscr.attr & SDHC_ADMA_ATTR_END)) {
+                /* IRQ delivered, reschedule current transfer */
+                break;
+            }
         }
 
         /* ADMA transfer terminates if blkcnt == 0 or by END attribute */
-- 
2.26.2



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 08/13] hw/sd/sdcard: Add trace event for ERASE command (CMD38)
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 07/13] hw/sd/sdhci: Yield if interrupt delivered during multiple transfer Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 09/13] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

Trace addresses provided to the ERASE command.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20201015063824.212980-2-f4bug@amsat.org>
---
 hw/sd/sd.c         | 2 +-
 hw/sd/trace-events | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 09/13] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (7 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 08/13] hw/sd/sdcard: Add trace event for ERASE command (CMD38) Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 10/13] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

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

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20201015063824.212980-3-f4bug@amsat.org>
---
 hw/sd/sd.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 10/13] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (8 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 09/13] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 11/13] hw/sd/sdcard: Reset both start/end addresses on error Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

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

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

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20201015063824.212980-4-f4bug@amsat.org>
---
 hw/sd/sd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 11/13] hw/sd/sdcard: Reset both start/end addresses on error
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (9 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 10/13] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 12/13] hw/sd/sdcard: Do not attempt to erase out of range addresses Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

From the Spec "4.3.5 Erase":

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

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

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

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20201015063824.212980-5-f4bug@amsat.org>
---
 hw/sd/sd.c | 2 ++
 1 file changed, 2 insertions(+)

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 12/13] hw/sd/sdcard: Do not attempt to erase out of range addresses
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (10 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 11/13] hw/sd/sdcard: Reset both start/end addresses on error Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-21 17:34 ` [PULL 13/13] hw/sd/sdcard: Assert if accessing an illegal group Philippe Mathieu-Daudé
  2020-10-22 11:33 ` [PULL 00/13] SD/MMC patches for 2020-10-21 Peter Maydell
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

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

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

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

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

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PULL 13/13] hw/sd/sdcard: Assert if accessing an illegal group
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (11 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 12/13] hw/sd/sdcard: Do not attempt to erase out of range addresses Philippe Mathieu-Daudé
@ 2020-10-21 17:34 ` Philippe Mathieu-Daudé
  2020-10-22 11:33 ` [PULL 00/13] SD/MMC patches for 2020-10-21 Peter Maydell
  13 siblings, 0 replies; 15+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-21 17:34 UTC (permalink / raw)
  To: qemu-devel; +Cc: Alexander Bulekov, Philippe Mathieu-Daudé

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

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Alexander Bulekov <alxndr@bu.edu>
Message-Id: <20201015063824.212980-7-f4bug@amsat.org>
---
 hw/sd/sd.c | 2 ++
 1 file changed, 2 insertions(+)

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



^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PULL 00/13] SD/MMC patches for 2020-10-21
  2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
                   ` (12 preceding siblings ...)
  2020-10-21 17:34 ` [PULL 13/13] hw/sd/sdcard: Assert if accessing an illegal group Philippe Mathieu-Daudé
@ 2020-10-22 11:33 ` Peter Maydell
  13 siblings, 0 replies; 15+ messages in thread
From: Peter Maydell @ 2020-10-22 11:33 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: Alexander Bulekov, QEMU Developers

On Wed, 21 Oct 2020 at 18:37, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The following changes since commit ac793156f650ae2d77834932d72224175ee69086:
>
>   Merge remote-tracking branch 'remotes/pmaydell/tags/pull-target-arm-2020102=
> 0-1' into staging (2020-10-20 21:11:35 +0100)
>
> are available in the Git repository at:
>
>   https://gitlab.com/philmd/qemu.git tags/sd-next-20201021
>
> for you to fetch changes up to 84816fb63e5c57159b469a66052d1b2bc862ef77:
>
>   hw/sd/sdcard: Assert if accessing an illegal group (2020-10-21 13:34:27 +02=
> 00)
>
> ----------------------------------------------------------------
> SD/MMC patches
>
> Fix two heap-overflow reported by Alexander Bulekov while fuzzing:
> - https://bugs.launchpad.net/qemu/+bug/1892960
> - https://bugs.launchpad.net/qemu/+bug/1895310
>
> CI jobs results:
> . https://cirrus-ci.com/build/6399328187056128
> . https://gitlab.com/philmd/qemu/-/pipelines/205701966
> . https://travis-ci.org/github/philmd/qemu/builds/737708930
> ----------------------------------------------------------------


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-10-22 11:42 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-21 17:34 [PULL 00/13] SD/MMC patches for 2020-10-21 Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 01/13] hw/sd/sdhci: Fix qemu_log_mask() format string Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 02/13] hw/sd/sdhci: Document the datasheet used Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 03/13] hw/sd/sdhci: Fix DMA Transfer Block Size field Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 04/13] hw/sd/sdhci: Stop multiple transfers when block count is cleared Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 05/13] hw/sd/sdhci: Resume pending DMA transfers on MMIO accesses Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 06/13] hw/sd/sdhci: Let sdhci_update_irq() return if IRQ was delivered Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 07/13] hw/sd/sdhci: Yield if interrupt delivered during multiple transfer Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 08/13] hw/sd/sdcard: Add trace event for ERASE command (CMD38) Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 09/13] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 10/13] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 11/13] hw/sd/sdcard: Reset both start/end addresses on error Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 12/13] hw/sd/sdcard: Do not attempt to erase out of range addresses Philippe Mathieu-Daudé
2020-10-21 17:34 ` [PULL 13/13] hw/sd/sdcard: Assert if accessing an illegal group Philippe Mathieu-Daudé
2020-10-22 11:33 ` [PULL 00/13] SD/MMC patches for 2020-10-21 Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.