[RFC PATCH 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[RFC PATCH 0/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[Philippe Mathieu-Daudé]

Yet another bug in the sdcard model found by libfuzzer:
https://bugs.launchpad.net/bugs/1895310

The bug is fixed, but there is a migration issue to
be resolved... so posting as RFC.

Philippe Mathieu-Daudé (6):
  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/trace-events |  2 +-
 2 files changed, 23 insertions(+), 9 deletions(-)

-- 
2.26.2

[RFC PATCH 1/6] hw/sd/sdcard: Add trace event for ERASE command (CMD38)

[Philippe Mathieu-Daudé]

Trace addresses provided to the ERASE command.

Signed-off-by: Philippe Mathieu-Daudé <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

[RFC PATCH 2/6] hw/sd/sdcard: Introduce the INVALID_ADDRESS definition

[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>
---
 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

[RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[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.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
Cc: Markus Armbruster <armbru@redhat.com>

Same problem I had with the pflash device last year...
This break migration :(
What is the best way to do this?
---
 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

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Markus Armbruster]

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

> As it is legal to WRITE/ERASE the address/block 0,
> change the value of this definition to an illegal
> address: UINT32_MAX.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Cc: Markus Armbruster <armbru@redhat.com>
>
> Same problem I had with the pflash device last year...
> This break migration :(
> What is the best way to do this?

Remind me: did we solve the problem with pflash, and if yes, how?

> ---
>  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),

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Philippe Mathieu-Daudé]

+Paolo & Kevin.

On 9/21/20 10:40 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> As it is legal to WRITE/ERASE the address/block 0,
>> change the value of this definition to an illegal
>> address: UINT32_MAX.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Cc: Markus Armbruster <armbru@redhat.com>
>>
>> Same problem I had with the pflash device last year...
>> This break migration :(
>> What is the best way to do this?
> 
> Remind me: did we solve the problem with pflash, and if yes, how?

No we can't. The best I could do is add a comment and as this
is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
Document use of non-CFI compliant command '0x00'").

I now consider the device in maintenance-only
mode and won't add any new features.

I started working on a new implementation, hoping it can be a
drop in replacement. Laszlo still has hope that QEMU pflash
device will support sector locking so firmware developers could
test upgrading fw in VMs.

Back to the SDcard, it might be less critical, so a migration
breaking change might be acceptable. I'm only aware of Paolo
and Kevin using this device for testing. Not sure of its
importance in production.

> 
>> ---
>>  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),
> 
> 

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Markus Armbruster]

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

> +Paolo & Kevin.
>
> On 9/21/20 10:40 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>> 
>>> As it is legal to WRITE/ERASE the address/block 0,
>>> change the value of this definition to an illegal
>>> address: UINT32_MAX.
>>>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>
>>> Same problem I had with the pflash device last year...
>>> This break migration :(
>>> What is the best way to do this?
>> 
>> Remind me: did we solve the problem with pflash, and if yes, how?
>
> No we can't. The best I could do is add a comment and as this
> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
> Document use of non-CFI compliant command '0x00'").
>
> I now consider the device in maintenance-only
> mode and won't add any new features.
>
> I started working on a new implementation, hoping it can be a
> drop in replacement. Laszlo still has hope that QEMU pflash
> device will support sector locking so firmware developers could
> test upgrading fw in VMs.
>
> Back to the SDcard, it might be less critical, so a migration
> breaking change might be acceptable. I'm only aware of Paolo
> and Kevin using this device for testing. Not sure of its
> importance in production.

Neither am I.

Which machine types include this device by default?

How can a non-default device be added, and to which machine types?

I gather the fix changes device state incompatibly.  Always, or only in
certain states?  I'm asking because if device state remains compatible
most of the time, we might be able use subsection trickery to keep
migration working most of the time.  Has been done before, I think.

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Dr. David Alan Gilbert]

* Markus Armbruster (armbru@redhat.com) wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> 
> > +Paolo & Kevin.
> >
> > On 9/21/20 10:40 AM, Markus Armbruster wrote:
> >> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> >> 
> >>> As it is legal to WRITE/ERASE the address/block 0,
> >>> change the value of this definition to an illegal
> >>> address: UINT32_MAX.
> >>>
> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>> ---
> >>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>> Cc: Markus Armbruster <armbru@redhat.com>
> >>>
> >>> Same problem I had with the pflash device last year...
> >>> This break migration :(
> >>> What is the best way to do this?
> >> 
> >> Remind me: did we solve the problem with pflash, and if yes, how?
> >
> > No we can't. The best I could do is add a comment and as this
> > is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
> > Document use of non-CFI compliant command '0x00'").
> >
> > I now consider the device in maintenance-only
> > mode and won't add any new features.
> >
> > I started working on a new implementation, hoping it can be a
> > drop in replacement. Laszlo still has hope that QEMU pflash
> > device will support sector locking so firmware developers could
> > test upgrading fw in VMs.
> >
> > Back to the SDcard, it might be less critical, so a migration
> > breaking change might be acceptable. I'm only aware of Paolo
> > and Kevin using this device for testing. Not sure of its
> > importance in production.
> 
> Neither am I.
> 
> Which machine types include this device by default?

To me it looks like it's some of the ARM boards.

Dave

> How can a non-default device be added, and to which machine types?
> 
> I gather the fix changes device state incompatibly.  Always, or only in
> certain states?  I'm asking because if device state remains compatible
> most of the time, we might be able use subsection trickery to keep
> migration working most of the time.  Has been done before, I think.
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Philippe Mathieu-Daudé]

On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote:
> * Markus Armbruster (armbru@redhat.com) wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>
>>> +Paolo & Kevin.
>>>
>>> On 9/21/20 10:40 AM, Markus Armbruster wrote:
>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>>
>>>>> As it is legal to WRITE/ERASE the address/block 0,
>>>>> change the value of this definition to an illegal
>>>>> address: UINT32_MAX.
>>>>>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>> ---
>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>>>
>>>>> Same problem I had with the pflash device last year...
>>>>> This break migration :(
>>>>> What is the best way to do this?
>>>>
>>>> Remind me: did we solve the problem with pflash, and if yes, how?
>>>
>>> No we can't. The best I could do is add a comment and as this
>>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
>>> Document use of non-CFI compliant command '0x00'").
>>>
>>> I now consider the device in maintenance-only
>>> mode and won't add any new features.
>>>
>>> I started working on a new implementation, hoping it can be a
>>> drop in replacement. Laszlo still has hope that QEMU pflash
>>> device will support sector locking so firmware developers could
>>> test upgrading fw in VMs.
>>>
>>> Back to the SDcard, it might be less critical, so a migration
>>> breaking change might be acceptable. I'm only aware of Paolo
>>> and Kevin using this device for testing. Not sure of its
>>> importance in production.
>>
>> Neither am I.
>>
>> Which machine types include this device by default?
> 
> To me it looks like it's some of the ARM boards.

My worry is TYPE_PCI_SDHCI ("sdhci-pci"):

    k->vendor_id = PCI_VENDOR_ID_REDHAT;
    k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
    k->class_id = PCI_CLASS_SYSTEM_SDHCI;

config SDHCI_PCI
    bool
    default y if PCI_DEVICES

> 
> Dave
> 
>> How can a non-default device be added, and to which machine types?
>>
>> I gather the fix changes device state incompatibly.  Always, or only in
>> certain states?  I'm asking because if device state remains compatible
>> most of the time, we might be able use subsection trickery to keep
>> migration working most of the time.  Has been done before, I think.

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Markus Armbruster]

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

> On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (armbru@redhat.com) wrote:
>>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>>>
>>>> +Paolo & Kevin.
>>>>
>>>> On 9/21/20 10:40 AM, Markus Armbruster wrote:
>>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
>>>>>
>>>>>> As it is legal to WRITE/ERASE the address/block 0,
>>>>>> change the value of this definition to an illegal
>>>>>> address: UINT32_MAX.
>>>>>>
>>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>>> ---
>>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
>>>>>> Cc: Markus Armbruster <armbru@redhat.com>
>>>>>>
>>>>>> Same problem I had with the pflash device last year...
>>>>>> This break migration :(
>>>>>> What is the best way to do this?
>>>>>
>>>>> Remind me: did we solve the problem with pflash, and if yes, how?
>>>>
>>>> No we can't. The best I could do is add a comment and as this
>>>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
>>>> Document use of non-CFI compliant command '0x00'").
>>>>
>>>> I now consider the device in maintenance-only
>>>> mode and won't add any new features.
>>>>
>>>> I started working on a new implementation, hoping it can be a
>>>> drop in replacement. Laszlo still has hope that QEMU pflash
>>>> device will support sector locking so firmware developers could
>>>> test upgrading fw in VMs.
>>>>
>>>> Back to the SDcard, it might be less critical, so a migration
>>>> breaking change might be acceptable. I'm only aware of Paolo
>>>> and Kevin using this device for testing. Not sure of its
>>>> importance in production.
>>>
>>> Neither am I.
>>>
>>> Which machine types include this device by default?
>> 
>> To me it looks like it's some of the ARM boards.
>
> My worry is TYPE_PCI_SDHCI ("sdhci-pci"):
>
>     k->vendor_id = PCI_VENDOR_ID_REDHAT;
>     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
>     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
>
> config SDHCI_PCI
>     bool
>     default y if PCI_DEVICES

Ah, now I remember.  Not the first time I wished it wouldn't exist...

>>> How can a non-default device be added, and to which machine types?
>>>
>>> I gather the fix changes device state incompatibly.  Always, or only in
>>> certain states?

I think we need to answer this question.

>>>                  I'm asking because if device state remains compatible
>>> most of the time, we might be able use subsection trickery to keep
>>> migration working most of the time.  Has been done before, I think.

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 3144 bytes --]

I think we can just bite the bullet and bump the version number. Just like
not all boards are created equal in terms of migration compatibility,
neither are all devices.

Unfortunately pflash is among those that need some care, but we have much
more leeway with sdhci-pci.

Paolo

Il lun 21 set 2020, 17:08 Markus Armbruster <armbru@redhat.com> ha scritto:

> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
>
> > On 9/21/20 2:24 PM, Dr. David Alan Gilbert wrote:
> >> * Markus Armbruster (armbru@redhat.com) wrote:
> >>> Philippe Mathieu-Daudé <philmd@redhat.com> writes:
> >>>
> >>>> +Paolo & Kevin.
> >>>>
> >>>> On 9/21/20 10:40 AM, Markus Armbruster wrote:
> >>>>> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> >>>>>
> >>>>>> As it is legal to WRITE/ERASE the address/block 0,
> >>>>>> change the value of this definition to an illegal
> >>>>>> address: UINT32_MAX.
> >>>>>>
> >>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>>>> ---
> >>>>>> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >>>>>> Cc: Markus Armbruster <armbru@redhat.com>
> >>>>>>
> >>>>>> Same problem I had with the pflash device last year...
> >>>>>> This break migration :(
> >>>>>> What is the best way to do this?
> >>>>>
> >>>>> Remind me: did we solve the problem with pflash, and if yes, how?
> >>>>
> >>>> No we can't. The best I could do is add a comment and as this
> >>>> is not fixable. See commit aba53a12bd5: ("hw/block/pflash_cfi01:
> >>>> Document use of non-CFI compliant command '0x00'").
> >>>>
> >>>> I now consider the device in maintenance-only
> >>>> mode and won't add any new features.
> >>>>
> >>>> I started working on a new implementation, hoping it can be a
> >>>> drop in replacement. Laszlo still has hope that QEMU pflash
> >>>> device will support sector locking so firmware developers could
> >>>> test upgrading fw in VMs.
> >>>>
> >>>> Back to the SDcard, it might be less critical, so a migration
> >>>> breaking change might be acceptable. I'm only aware of Paolo
> >>>> and Kevin using this device for testing. Not sure of its
> >>>> importance in production.
> >>>
> >>> Neither am I.
> >>>
> >>> Which machine types include this device by default?
> >>
> >> To me it looks like it's some of the ARM boards.
> >
> > My worry is TYPE_PCI_SDHCI ("sdhci-pci"):
> >
> >     k->vendor_id = PCI_VENDOR_ID_REDHAT;
> >     k->device_id = PCI_DEVICE_ID_REDHAT_SDHCI;
> >     k->class_id = PCI_CLASS_SYSTEM_SDHCI;
> >
> > config SDHCI_PCI
> >     bool
> >     default y if PCI_DEVICES
>
> Ah, now I remember.  Not the first time I wished it wouldn't exist...
>
> >>> How can a non-default device be added, and to which machine types?
> >>>
> >>> I gather the fix changes device state incompatibly.  Always, or only in
> >>> certain states?
>
> I think we need to answer this question.
>
> >>>                  I'm asking because if device state remains compatible
> >>> most of the time, we might be able use subsection trickery to keep
> >>> migration working most of the time.  Has been done before, I think.
>
>

[-- Attachment #2: Type: text/html, Size: 4946 bytes --]

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Markus Armbruster]

Paolo Bonzini <pbonzini@redhat.com> writes:

> I think we can just bite the bullet and bump the version number. Just like
> not all boards are created equal in terms of migration compatibility,
> neither are all devices.
>
> Unfortunately pflash is among those that need some care, but we have much
> more leeway with sdhci-pci.

No objection.

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Philippe Mathieu-Daudé]

On 9/22/20 4:48 PM, Markus Armbruster wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> I think we can just bite the bullet and bump the version number. Just like
>> not all boards are created equal in terms of migration compatibility,
>> neither are all devices.

Great. I'll add that to the commit description.

>>
>> Unfortunately pflash is among those that need some care, but we have much
>> more leeway with sdhci-pci.
> 
> No objection.
> 

Re: [RFC PATCH 3/6] hw/sd/sdcard: Do not use legal address '0' for INVALID_ADDRESS

[Kevin O'Connor]

On Mon, Sep 21, 2020 at 12:31:21PM +0200, Philippe Mathieu-Daudé wrote:
> Back to the SDcard, it might be less critical, so a migration
> breaking change might be acceptable. I'm only aware of Paolo
> and Kevin using this device for testing. Not sure of its
> importance in production.

FWIW, I only use the sdcard for testing (and only use sdhci-pci).  I
don't know if others use it in production, however.

Cheers,
-Kevin

[RFC PATCH 4/6] hw/sd/sdcard: Reset both start/end addresses on error

[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>
---
 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

[RFC PATCH 5/6] hw/sd/sdcard: Do not attempt to erase out of range addresses

[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/bugs/1895310
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Signed-off-by: Philippe Mathieu-Daudé <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

[RFC PATCH 6/6] hw/sd/sdcard: Assert if accessing an illegal group

[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>
---
 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