[PATCH-for-6.1 0/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[PATCH-for-6.1 0/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

Fix an assertion reported by OSS-Fuzz, add corresponding qtest.

The change simple enough for the next rc.

Philippe Mathieu-Daudé (3):
  hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
  hw/sd/sdcard: Fix assertion accessing out-of-range addresses with
    CMD30
  hw/sd/sdcard: Rename Write Protect Group variables

 hw/sd/sd.c                     | 37 ++++++++++++++++++++--------------
 tests/qtest/fuzz-sdcard-test.c | 36 +++++++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH-for-6.1 1/3] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT

[Philippe Mathieu-Daudé]

Per the 'Physical Layer Simplified Specification Version 3.01',
Table 4-22: 'Block Oriented Write Protection Commands'

  SEND_WRITE_PROT (CMD30)

  If the card provides write protection features, this command asks
  the card to send the status of the write protection bits [1].

  [1] 32 write protection bits (representing 32 write protect groups
  starting at the specified address) [...]
  The last (least significant) bit of the protection bits corresponds
  to the first addressed group. If the addresses of the last groups
  are outside the valid range, then the corresponding write protection
  bits shall be set to 0.

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 1f964e022b1..707dcc12a14 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t 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)) {
+        if (addr >= sd->size) {
+            /*
+             * If the addresses of the last groups are outside the valid range,
+             * then the corresponding write protection bits shall be set to 0.
+             */
+            continue;
+        }
+        if (test_bit(wpnum, sd->wp_groups)) {
             ret |= (1 << i);
         }
     }
-- 
2.31.1

Re: [PATCH-for-6.1 1/3] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT

[Alexander Bulekov]

On 210728 2017, Philippe Mathieu-Daudé wrote:
> Per the 'Physical Layer Simplified Specification Version 3.01',
> Table 4-22: 'Block Oriented Write Protection Commands'
> 
>   SEND_WRITE_PROT (CMD30)
> 
>   If the card provides write protection features, this command asks
>   the card to send the status of the write protection bits [1].
> 
>   [1] 32 write protection bits (representing 32 write protect groups
>   starting at the specified address) [...]
>   The last (least significant) bit of the protection bits corresponds
>   to the first addressed group. If the addresses of the last groups
>   are outside the valid range, then the corresponding write protection
>   bits shall be set to 0.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

-Alex

> ---
>  hw/sd/sd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1f964e022b1..707dcc12a14 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t 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)) {
> +        if (addr >= sd->size) {
> +            /*
> +             * If the addresses of the last groups are outside the valid range,
> +             * then the corresponding write protection bits shall be set to 0.
> +             */
> +            continue;
> +        }
> +        if (test_bit(wpnum, sd->wp_groups)) {
>              ret |= (1 << i);
>          }
>      }
> -- 
> 2.31.1
> 

Re: [PATCH-for-6.1 1/3] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT

[Peter Maydell]

On Wed, 28 Jul 2021 at 19:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Per the 'Physical Layer Simplified Specification Version 3.01',
> Table 4-22: 'Block Oriented Write Protection Commands'
>
>   SEND_WRITE_PROT (CMD30)
>
>   If the card provides write protection features, this command asks
>   the card to send the status of the write protection bits [1].
>
>   [1] 32 write protection bits (representing 32 write protect groups
>   starting at the specified address) [...]
>   The last (least significant) bit of the protection bits corresponds
>   to the first addressed group. If the addresses of the last groups
>   are outside the valid range, then the corresponding write protection
>   bits shall be set to 0.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 1f964e022b1..707dcc12a14 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t 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)) {
> +        if (addr >= sd->size) {
> +            /*
> +             * If the addresses of the last groups are outside the valid range,
> +             * then the corresponding write protection bits shall be set to 0.
> +             */
> +            continue;
> +        }
> +        if (test_bit(wpnum, sd->wp_groups)) {

Am I misreading it, or does this commit not actually change
the behaviour of the code ?

>              ret |= (1 << i);
>          }
>      }
> --
> 2.31.1

-- PMM

Re: [PATCH-for-6.1 1/3] hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT

[Philippe Mathieu-Daudé]

On 8/2/21 2:00 PM, Peter Maydell wrote:
> On Wed, 28 Jul 2021 at 19:18, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Per the 'Physical Layer Simplified Specification Version 3.01',
>> Table 4-22: 'Block Oriented Write Protection Commands'
>>
>>   SEND_WRITE_PROT (CMD30)
>>
>>   If the card provides write protection features, this command asks
>>   the card to send the status of the write protection bits [1].
>>
>>   [1] 32 write protection bits (representing 32 write protect groups
>>   starting at the specified address) [...]
>>   The last (least significant) bit of the protection bits corresponds
>>   to the first addressed group. If the addresses of the last groups
>>   are outside the valid range, then the corresponding write protection
>>   bits shall be set to 0.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>  hw/sd/sd.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
>> index 1f964e022b1..707dcc12a14 100644
>> --- a/hw/sd/sd.c
>> +++ b/hw/sd/sd.c
>> @@ -822,7 +822,14 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t 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)) {
>> +        if (addr >= sd->size) {
>> +            /*
>> +             * If the addresses of the last groups are outside the valid range,
>> +             * then the corresponding write protection bits shall be set to 0.
>> +             */
>> +            continue;
>> +        }
>> +        if (test_bit(wpnum, sd->wp_groups)) {
> 
> Am I misreading it, or does this commit not actually change
> the behaviour of the code ?

Yes, I don't want to change the behaviour but document it
better.

> 
>>              ret |= (1 << i);
>>          }
>>      }
>> --
>> 2.31.1
> 
> -- PMM
> 

[PATCH-for-6.1 2/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

OSS-Fuzz found sending illegal addresses when querying the write
protection bits triggers the assertion added in commit 84816fb63e5
("hw/sd/sdcard: Assert if accessing an illegal group"):

  qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
  Assertion `wpnum < sd->wpgrps_size' failed.
  #3 0x7f62a8b22c91 in __assert_fail
  #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
  #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
  #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
  #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
  #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
  #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
  #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5

It is legal for the CMD30 to query for out-of-range addresses.
Such invalid addresses are simply ignored in the response (write
protection bits set to 0).

Note, we had an off-by-one in the wpgrps_size check since commit
a1bb27b1e98. Since we have a total of 'wpgrps_size' bits, the latest
valid group bit is 'wpgrps_size - 1'.

Since we now check the group bit is in range, remove the assertion.

Include the qtest reproducer provided by Alexander Bulekov:

  $ make check-qtest-i386
  ...
  Running test qtest-i386/fuzz-sdcard-test
  qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum < sd->wpgrps_size' failed.

Cc: qemu-stable@nongnu.org
Reported-by: OSS-Fuzz (Issue 29225)
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/495
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c                     |  4 ++--
 tests/qtest/fuzz-sdcard-test.c | 36 ++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 707dcc12a14..273af75c1be 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -820,8 +820,8 @@ 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);
+    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
+                i++, wpnum++, addr += WPGROUP_SIZE) {
         if (addr >= sd->size) {
             /*
              * If the addresses of the last groups are outside the valid range,
diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
index 96602eac7e5..ae14305344a 100644
--- a/tests/qtest/fuzz-sdcard-test.c
+++ b/tests/qtest/fuzz-sdcard-test.c
@@ -52,6 +52,41 @@ static void oss_fuzz_29225(void)
     qtest_quit(s);
 }
 
+/*
+ * https://gitlab.com/qemu-project/qemu/-/issues/495
+ * Used to trigger:
+ *  Assertion `wpnum < sd->wpgrps_size' failed.
+ */
+static void oss_fuzz_36217(void)
+{
+    QTestState *s;
+
+    s = qtest_init(" -display none -m 32 -nodefaults -nographic"
+                   " -device sdhci-pci,sd-spec-version=3 "
+                   "-device sd-card,drive=d0 "
+                   "-drive if=none,index=0,file=null-co://,format=raw,id=d0");
+
+    qtest_outl(s, 0xcf8, 0x80001010);
+    qtest_outl(s, 0xcfc, 0xe0000000);
+    qtest_outl(s, 0xcf8, 0x80001004);
+    qtest_outw(s, 0xcfc, 0x02);
+    qtest_bufwrite(s, 0xe000002c, "\x05", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x37", 0x1);
+    qtest_bufwrite(s, 0xe000000a, "\x01", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x29", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x02", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x03", 0x1);
+    qtest_bufwrite(s, 0xe0000005, "\x01", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x06", 0x1);
+    qtest_bufwrite(s, 0xe000000c, "\x05", 0x1);
+    qtest_bufwrite(s, 0xe000000e, "\x20", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x08", 0x1);
+    qtest_bufwrite(s, 0xe000000b, "\x3d", 0x1);
+    qtest_bufwrite(s, 0xe000000f, "\x1e", 0x1);
+
+    qtest_quit(s);
+}
+
 int main(int argc, char **argv)
 {
     const char *arch = qtest_get_arch();
@@ -60,6 +95,7 @@ int main(int argc, char **argv)
 
    if (strcmp(arch, "i386") == 0) {
         qtest_add_func("fuzz/sdcard/oss_fuzz_29225", oss_fuzz_29225);
+        qtest_add_func("fuzz/sdcard/oss_fuzz_36217", oss_fuzz_36217);
    }
 
    return g_test_run();
-- 
2.31.1

Re: [PATCH-for-6.1 2/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

Peter, Bin, could you review this (trivial) patch?
This is a regression since 6.1 I'd like to get fixed
for v6.1.0-rc2.

On 7/28/21 8:17 PM, Philippe Mathieu-Daudé wrote:
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers the assertion added in commit 84816fb63e5
> ("hw/sd/sdcard: Assert if accessing an illegal group"):
> 
>   qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
>   Assertion `wpnum < sd->wpgrps_size' failed.
>   #3 0x7f62a8b22c91 in __assert_fail
>   #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
>   #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
>   #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
>   #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
>   #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
>   #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
>   #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5
> 
> It is legal for the CMD30 to query for out-of-range addresses.
> Such invalid addresses are simply ignored in the response (write
> protection bits set to 0).
> 
> Note, we had an off-by-one in the wpgrps_size check since commit
> a1bb27b1e98. Since we have a total of 'wpgrps_size' bits, the latest
> valid group bit is 'wpgrps_size - 1'.
> 
> Since we now check the group bit is in range, remove the assertion.
> 
> Include the qtest reproducer provided by Alexander Bulekov:
> 
>   $ make check-qtest-i386
>   ...
>   Running test qtest-i386/fuzz-sdcard-test
>   qemu-system-i386: ../hw/sd/sd.c:824: sd_wpbits: Assertion `wpnum < sd->wpgrps_size' failed.
> 
> Cc: qemu-stable@nongnu.org
> Reported-by: OSS-Fuzz (Issue 29225)
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/495
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/sd/sd.c                     |  4 ++--
>  tests/qtest/fuzz-sdcard-test.c | 36 ++++++++++++++++++++++++++++++++++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 707dcc12a14..273af75c1be 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -820,8 +820,8 @@ 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);
> +    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
> +                i++, wpnum++, addr += WPGROUP_SIZE) {
>          if (addr >= sd->size) {
>              /*
>               * If the addresses of the last groups are outside the valid range,
> diff --git a/tests/qtest/fuzz-sdcard-test.c b/tests/qtest/fuzz-sdcard-test.c
> index 96602eac7e5..ae14305344a 100644
> --- a/tests/qtest/fuzz-sdcard-test.c
> +++ b/tests/qtest/fuzz-sdcard-test.c
> @@ -52,6 +52,41 @@ static void oss_fuzz_29225(void)
>      qtest_quit(s);
>  }
>  
> +/*
> + * https://gitlab.com/qemu-project/qemu/-/issues/495
> + * Used to trigger:
> + *  Assertion `wpnum < sd->wpgrps_size' failed.
> + */
> +static void oss_fuzz_36217(void)
> +{
> +    QTestState *s;
> +
> +    s = qtest_init(" -display none -m 32 -nodefaults -nographic"
> +                   " -device sdhci-pci,sd-spec-version=3 "
> +                   "-device sd-card,drive=d0 "
> +                   "-drive if=none,index=0,file=null-co://,format=raw,id=d0");
> +
> +    qtest_outl(s, 0xcf8, 0x80001010);
> +    qtest_outl(s, 0xcfc, 0xe0000000);
> +    qtest_outl(s, 0xcf8, 0x80001004);
> +    qtest_outw(s, 0xcfc, 0x02);
> +    qtest_bufwrite(s, 0xe000002c, "\x05", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x37", 0x1);
> +    qtest_bufwrite(s, 0xe000000a, "\x01", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x29", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x02", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x03", 0x1);
> +    qtest_bufwrite(s, 0xe0000005, "\x01", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x06", 0x1);
> +    qtest_bufwrite(s, 0xe000000c, "\x05", 0x1);
> +    qtest_bufwrite(s, 0xe000000e, "\x20", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x08", 0x1);
> +    qtest_bufwrite(s, 0xe000000b, "\x3d", 0x1);
> +    qtest_bufwrite(s, 0xe000000f, "\x1e", 0x1);
> +
> +    qtest_quit(s);
> +}
> +
>  int main(int argc, char **argv)
>  {
>      const char *arch = qtest_get_arch();
> @@ -60,6 +95,7 @@ int main(int argc, char **argv)
>  
>     if (strcmp(arch, "i386") == 0) {
>          qtest_add_func("fuzz/sdcard/oss_fuzz_29225", oss_fuzz_29225);
> +        qtest_add_func("fuzz/sdcard/oss_fuzz_36217", oss_fuzz_36217);
>     }
>  
>     return g_test_run();
> 

Re: [PATCH-for-6.1 2/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Peter Maydell]

On Wed, 28 Jul 2021 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> OSS-Fuzz found sending illegal addresses when querying the write
> protection bits triggers the assertion added in commit 84816fb63e5
> ("hw/sd/sdcard: Assert if accessing an illegal group"):
>
>   qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
>   Assertion `wpnum < sd->wpgrps_size' failed.
>   #3 0x7f62a8b22c91 in __assert_fail
>   #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
>   #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
>   #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
>   #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
>   #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
>   #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
>   #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5
>
> It is legal for the CMD30 to query for out-of-range addresses.
> Such invalid addresses are simply ignored in the response (write
> protection bits set to 0).
>
> Note, we had an off-by-one in the wpgrps_size check since commit
> a1bb27b1e98. Since we have a total of 'wpgrps_size' bits, the latest
> valid group bit is 'wpgrps_size - 1'.

The commit message says "wpgrps_size - 1" is valid...

> @@ -820,8 +820,8 @@ 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);
> +    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;

...but the code change makes the loop terminate when
wpnum == wpgrps_size - 1, so we don't execute the loop
body for wpgrps_size -1.

Which is correct ?

> +                i++, wpnum++, addr += WPGROUP_SIZE) {

thanks
-- PMM

Re: [PATCH-for-6.1 2/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

On 8/2/21 2:03 PM, Peter Maydell wrote:
> On Wed, 28 Jul 2021 at 19:19, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> OSS-Fuzz found sending illegal addresses when querying the write
>> protection bits triggers the assertion added in commit 84816fb63e5
>> ("hw/sd/sdcard: Assert if accessing an illegal group"):
>>
>>   qemu-fuzz-i386-target-generic-fuzz-sdhci-v3: ../hw/sd/sd.c:824: uint32_t sd_wpbits(SDState *, uint64_t):
>>   Assertion `wpnum < sd->wpgrps_size' failed.
>>   #3 0x7f62a8b22c91 in __assert_fail
>>   #4 0x5569adcec405 in sd_wpbits hw/sd/sd.c:824:9
>>   #5 0x5569adce5f6d in sd_normal_command hw/sd/sd.c:1389:38
>>   #6 0x5569adce3870 in sd_do_command hw/sd/sd.c:1737:17
>>   #7 0x5569adcf1566 in sdbus_do_command hw/sd/core.c:100:16
>>   #8 0x5569adcfc192 in sdhci_send_command hw/sd/sdhci.c:337:12
>>   #9 0x5569adcfa3a3 in sdhci_write hw/sd/sdhci.c:1186:9
>>   #10 0x5569adfb3447 in memory_region_write_accessor softmmu/memory.c:492:5
>>
>> It is legal for the CMD30 to query for out-of-range addresses.
>> Such invalid addresses are simply ignored in the response (write
>> protection bits set to 0).
>>
>> Note, we had an off-by-one in the wpgrps_size check since commit
>> a1bb27b1e98. Since we have a total of 'wpgrps_size' bits, the latest
>> valid group bit is 'wpgrps_size - 1'.
> 
> The commit message says "wpgrps_size - 1" is valid...
> 
>> @@ -820,8 +820,8 @@ 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);
>> +    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
> 
> ...but the code change makes the loop terminate when
> wpnum == wpgrps_size - 1, so we don't execute the loop
> body for wpgrps_size -1.
> 
> Which is correct ?

The problem is in sd_reset(), this code is hard for me to follow
(and I plan to refactor it during next dev cycle):

        blk_get_geometry(sd->blk, &sect);
    size = sect << 9;
    sect = sd_addr_to_wpnum(size) + 1;
    sd->wpgrps_size = sect;
    sd->wp_groups = bitmap_new(sd->wpgrps_size);

CID.WP_GRP_SIZE is defined as:

  The size of a write protected group. The content of this register
  is a 7-bit binary coded value, defining the number of erase sectors
  (see SECTOR_SIZE). The actual size is computed by increasing this
  number by one. A value of zero means one erase sector, 127 means
  128 erase sectors.

I think there is a confusion, wpgrps_size holds the real number of erase
sectors (used by the model, not returned in the CID.WP_GRP_SIZE
register). CID.WP_GRP_SIZE should be (wpgrps_size - 1).

Currently we iterate 1 sector number outside of the flash area.
To avoid that I used 'wpnum < sd->wpgrps_size - 1' instead of
'wpnum <= sd->wpgrps_size - 1'.

But with the fix you suggested responding to the cover, we don't hit
this case anymore. So I'll take it and clean the rest later.

Thanks,

Phil.

> 
>> +                i++, wpnum++, addr += WPGROUP_SIZE) {
> 
> thanks
> -- PMM
> 

[PATCH-for-6.2 3/3] hw/sd/sdcard: Rename Write Protect Group variables

[Philippe Mathieu-Daudé]

'wp_groups' holds a bitmap, rename it as 'wp_group_bmap'.
'wpgrps_size' is the bitmap size (in bits), rename it as
'wp_group_bits'.

Patch created mechanically using:

  $ sed -i -e s/wp_groups/wp_group_bmap/ \
           -e s/wpgrps_size/wp_group_bits/ hw/sd/sd.c

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

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index 273af75c1be..75dcd3f7f65 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -116,8 +116,8 @@ struct SDState {
     int32_t state;    /* current card state, one of SDCardStates */
     uint32_t vhs;
     bool wp_switch;
-    unsigned long *wp_groups;
-    int32_t wpgrps_size;
+    unsigned long *wp_group_bmap;
+    int32_t wp_group_bits;
     uint64_t size;
     uint32_t blk_len;
     uint32_t multi_blk_cnt;
@@ -567,10 +567,10 @@ static void sd_reset(DeviceState *dev)
     sd_set_cardstatus(sd);
     sd_set_sdstatus(sd);
 
-    g_free(sd->wp_groups);
+    g_free(sd->wp_group_bmap);
     sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
-    sd->wpgrps_size = sect;
-    sd->wp_groups = bitmap_new(sd->wpgrps_size);
+    sd->wp_group_bits = sect;
+    sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
     memset(sd->function_group, 0, sizeof(sd->function_group));
     sd->erase_start = INVALID_ADDRESS;
     sd->erase_end = INVALID_ADDRESS;
@@ -673,7 +673,7 @@ static const VMStateDescription sd_vmstate = {
         VMSTATE_UINT32(card_status, SDState),
         VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
         VMSTATE_UINT32(vhs, SDState),
-        VMSTATE_BITMAP(wp_groups, SDState, 0, wpgrps_size),
+        VMSTATE_BITMAP(wp_group_bmap, SDState, 0, wp_group_bits),
         VMSTATE_UINT32(blk_len, SDState),
         VMSTATE_UINT32(multi_blk_cnt, SDState),
         VMSTATE_UINT32(erase_start, SDState),
@@ -803,8 +803,8 @@ static void sd_erase(SDState *sd)
         if (sdsc) {
             /* Only SDSC cards support write protect groups */
             wpnum = sd_addr_to_wpnum(erase_addr);
-            assert(wpnum < sd->wpgrps_size);
-            if (test_bit(wpnum, sd->wp_groups)) {
+            assert(wpnum < sd->wp_group_bits);
+            if (test_bit(wpnum, sd->wp_group_bmap)) {
                 sd->card_status |= WP_ERASE_SKIP;
                 continue;
             }
@@ -820,7 +820,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
 
     wpnum = sd_addr_to_wpnum(addr);
 
-    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
+    for (i = 0; i < 32 && wpnum < sd->wp_group_bits - 1;
                 i++, wpnum++, addr += WPGROUP_SIZE) {
         if (addr >= sd->size) {
             /*
@@ -829,7 +829,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
              */
             continue;
         }
-        if (test_bit(wpnum, sd->wp_groups)) {
+        if (test_bit(wpnum, sd->wp_group_bmap)) {
             ret |= (1 << i);
         }
     }
@@ -869,7 +869,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(addr), sd->wp_group_bmap);
 }
 
 static void sd_lock_command(SDState *sd)
@@ -897,7 +897,7 @@ static void sd_lock_command(SDState *sd)
             sd->card_status |= LOCK_UNLOCK_FAILED;
             return;
         }
-        bitmap_zero(sd->wp_groups, sd->wpgrps_size);
+        bitmap_zero(sd->wp_group_bmap, sd->wp_group_bits);
         sd->csd[14] &= ~0x10;
         sd->card_status &= ~CARD_IS_LOCKED;
         sd->pwd_len = 0;
@@ -1348,7 +1348,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(addr), sd->wp_group_bmap);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
@@ -1370,7 +1370,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(addr), sd->wp_group_bmap);
             /* Bzzzzzzztt .... Operation complete.  */
             sd->state = sd_transfer_state;
             return sd_r1b;
-- 
2.31.1

Re: [PATCH-for-6.2 3/3] hw/sd/sdcard: Rename Write Protect Group variables

[Alexander Bulekov]

On 210728 2017, Philippe Mathieu-Daudé wrote:
> 'wp_groups' holds a bitmap, rename it as 'wp_group_bmap'.
> 'wpgrps_size' is the bitmap size (in bits), rename it as
> 'wp_group_bits'.
> 
> Patch created mechanically using:
> 
>   $ sed -i -e s/wp_groups/wp_group_bmap/ \
>            -e s/wpgrps_size/wp_group_bits/ hw/sd/sd.c
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Alexander Bulekov <alxndr@bu.edu>

-Alex

> ---
>  hw/sd/sd.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index 273af75c1be..75dcd3f7f65 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -116,8 +116,8 @@ struct SDState {
>      int32_t state;    /* current card state, one of SDCardStates */
>      uint32_t vhs;
>      bool wp_switch;
> -    unsigned long *wp_groups;
> -    int32_t wpgrps_size;
> +    unsigned long *wp_group_bmap;
> +    int32_t wp_group_bits;
>      uint64_t size;
>      uint32_t blk_len;
>      uint32_t multi_blk_cnt;
> @@ -567,10 +567,10 @@ static void sd_reset(DeviceState *dev)
>      sd_set_cardstatus(sd);
>      sd_set_sdstatus(sd);
>  
> -    g_free(sd->wp_groups);
> +    g_free(sd->wp_group_bmap);
>      sd->wp_switch = sd->blk ? !blk_is_writable(sd->blk) : false;
> -    sd->wpgrps_size = sect;
> -    sd->wp_groups = bitmap_new(sd->wpgrps_size);
> +    sd->wp_group_bits = sect;
> +    sd->wp_group_bmap = bitmap_new(sd->wp_group_bits);
>      memset(sd->function_group, 0, sizeof(sd->function_group));
>      sd->erase_start = INVALID_ADDRESS;
>      sd->erase_end = INVALID_ADDRESS;
> @@ -673,7 +673,7 @@ static const VMStateDescription sd_vmstate = {
>          VMSTATE_UINT32(card_status, SDState),
>          VMSTATE_PARTIAL_BUFFER(sd_status, SDState, 1),
>          VMSTATE_UINT32(vhs, SDState),
> -        VMSTATE_BITMAP(wp_groups, SDState, 0, wpgrps_size),
> +        VMSTATE_BITMAP(wp_group_bmap, SDState, 0, wp_group_bits),
>          VMSTATE_UINT32(blk_len, SDState),
>          VMSTATE_UINT32(multi_blk_cnt, SDState),
>          VMSTATE_UINT32(erase_start, SDState),
> @@ -803,8 +803,8 @@ static void sd_erase(SDState *sd)
>          if (sdsc) {
>              /* Only SDSC cards support write protect groups */
>              wpnum = sd_addr_to_wpnum(erase_addr);
> -            assert(wpnum < sd->wpgrps_size);
> -            if (test_bit(wpnum, sd->wp_groups)) {
> +            assert(wpnum < sd->wp_group_bits);
> +            if (test_bit(wpnum, sd->wp_group_bmap)) {
>                  sd->card_status |= WP_ERASE_SKIP;
>                  continue;
>              }
> @@ -820,7 +820,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>  
>      wpnum = sd_addr_to_wpnum(addr);
>  
> -    for (i = 0; i < 32 && wpnum < sd->wpgrps_size - 1;
> +    for (i = 0; i < 32 && wpnum < sd->wp_group_bits - 1;
>                  i++, wpnum++, addr += WPGROUP_SIZE) {
>          if (addr >= sd->size) {
>              /*
> @@ -829,7 +829,7 @@ static uint32_t sd_wpbits(SDState *sd, uint64_t addr)
>               */
>              continue;
>          }
> -        if (test_bit(wpnum, sd->wp_groups)) {
> +        if (test_bit(wpnum, sd->wp_group_bmap)) {
>              ret |= (1 << i);
>          }
>      }
> @@ -869,7 +869,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(addr), sd->wp_group_bmap);
>  }
>  
>  static void sd_lock_command(SDState *sd)
> @@ -897,7 +897,7 @@ static void sd_lock_command(SDState *sd)
>              sd->card_status |= LOCK_UNLOCK_FAILED;
>              return;
>          }
> -        bitmap_zero(sd->wp_groups, sd->wpgrps_size);
> +        bitmap_zero(sd->wp_group_bmap, sd->wp_group_bits);
>          sd->csd[14] &= ~0x10;
>          sd->card_status &= ~CARD_IS_LOCKED;
>          sd->pwd_len = 0;
> @@ -1348,7 +1348,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(addr), sd->wp_group_bmap);
>              /* Bzzzzzzztt .... Operation complete.  */
>              sd->state = sd_transfer_state;
>              return sd_r1b;
> @@ -1370,7 +1370,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(addr), sd->wp_group_bmap);
>              /* Bzzzzzzztt .... Operation complete.  */
>              sd->state = sd_transfer_state;
>              return sd_r1b;
> -- 
> 2.31.1
> 

Re: [PATCH-for-6.1 0/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Peter Maydell]

On Wed, 28 Jul 2021 at 19:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Fix an assertion reported by OSS-Fuzz, add corresponding qtest.
>
> The change simple enough for the next rc.
>
> Philippe Mathieu-Daudé (3):
>   hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
>   hw/sd/sdcard: Fix assertion accessing out-of-range addresses with
>     CMD30
>   hw/sd/sdcard: Rename Write Protect Group variables

I've left review comments on individual patches, but my suspicion
is that the fix for this assertion failure is just "the
assert should be after the test for 'addr < sd->size', not before",
something like:

@@ -821,8 +821,12 @@ 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) {
+        if (addr >= sd->size) {
+            /* Out of range groups report as zero */
+            continue;
+        }
         assert(wpnum < sd->wpgrps_size);
-        if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
+        if (test_bit(wpnum, sd->wp_groups)) {
             ret |= (1 << i);
         }
     }

-- PMM

Re: [PATCH-for-6.1 0/3] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

[Philippe Mathieu-Daudé]

On 8/2/21 2:10 PM, Peter Maydell wrote:
> On Wed, 28 Jul 2021 at 19:17, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Fix an assertion reported by OSS-Fuzz, add corresponding qtest.
>>
>> The change simple enough for the next rc.
>>
>> Philippe Mathieu-Daudé (3):
>>   hw/sd/sdcard: Document out-of-range addresses for SEND_WRITE_PROT
>>   hw/sd/sdcard: Fix assertion accessing out-of-range addresses with
>>     CMD30
>>   hw/sd/sdcard: Rename Write Protect Group variables
> 
> I've left review comments on individual patches, but my suspicion
> is that the fix for this assertion failure is just "the
> assert should be after the test for 'addr < sd->size', not before",
> something like:
> 
> @@ -821,8 +821,12 @@ 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) {
> +        if (addr >= sd->size) {
> +            /* Out of range groups report as zero */
> +            continue;
> +        }
>          assert(wpnum < sd->wpgrps_size);
> -        if (addr < sd->size && test_bit(wpnum, sd->wp_groups)) {
> +        if (test_bit(wpnum, sd->wp_groups)) {
>              ret |= (1 << i);
>          }
>      }

It is simpler and works :) Thanks!