[v1] hw/sd/sdcard: Fix assertion accessing out-of-range addresses with CMD30

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

Posted by Philippe Mathieu-Daudé 4 years, 6 months ago

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

Posted by Alexander Bulekov 4 years, 6 months ago

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

Posted by Peter Maydell 4 years, 6 months ago

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

Posted by Philippe Mathieu-Daudé 4 years, 6 months ago

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
>