diff mbox series

[v3,03/11] hw/sd/sdcard: Do not switch to ReceivingData if address is invalid

Message ID 20200605102230.21493-4-philmd@redhat.com (mailing list archive)
State New, archived
Headers show
Series hw/sd/sdcard: Fix CVE-2020-13253 & cleanups | expand

Commit Message

Philippe Mathieu-Daudé June 5, 2020, 10:22 a.m. UTC 
From: Philippe Mathieu-Daudé <f4bug@amsat.org>

Only move the state machine to ReceivingData if there is no
pending error.  This avoids later OOB access while processing
commands queued.

  "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"

  4.3.3 Data Read

  Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

  4.3.4 Data Write

  Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
  occurred and no data transfer is performed.

Fixes: CVE-2020-13253
Cc: Prasad J Pandit <pjp@fedoraproject.org>
Reported-by: Alexander Bulekov <alxndr@bu.edu>
Buglink: https://bugs.launchpad.net/qemu/+bug/1880822
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/sd/sd.c | 64 ++++++++++++++++++++++++++++++++----------------------
 1 file changed, 38 insertions(+), 26 deletions(-)

Comments

Peter Maydell June 15, 2020, 2:06 p.m. UTC | #1 
On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Only move the state machine to ReceivingData if there is no
> pending error.  This avoids later OOB access while processing
> commands queued.
>
>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>
>   4.3.3 Data Read
>
>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>   occurred and no data transfer is performed.
>
>   4.3.4 Data Write
>
>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>   occurred and no data transfer is performed.

It's not clear from the spec that this should also
apply to WP_VIOLATION errors. The text about WP_VIOLATION
suggests that it is handled by aborting the data transfer
(ie set the error bit, stay in receive-data state, wait for
a stop command, but ignore all further data transfer),
which is I think distinct from "rejecting" the command.

If that theory is right then moving the check for the
ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
tests should stay as they are, I think.

NB: is the buffer overrun we're trying to protect against
caused by passing sd_wp_addr() a bad address? Maybe we
should assert in sd_addr_to_wpnum() that the address is
in range, as a defence.

thanks
-- PMM
Philippe Mathieu-Daudé July 13, 2020, 4:36 p.m. UTC | #2 
On 6/15/20 4:06 PM, Peter Maydell wrote:
> On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> Only move the state machine to ReceivingData if there is no
>> pending error.  This avoids later OOB access while processing
>> commands queued.
>>
>>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>>
>>   4.3.3 Data Read
>>
>>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>   occurred and no data transfer is performed.
>>
>>   4.3.4 Data Write
>>
>>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>   occurred and no data transfer is performed.
> 
> It's not clear from the spec that this should also
> apply to WP_VIOLATION errors. The text about WP_VIOLATION
> suggests that it is handled by aborting the data transfer
> (ie set the error bit, stay in receive-data state, wait for
> a stop command, but ignore all further data transfer),
> which is I think distinct from "rejecting" the command.
> 
> If that theory is right then moving the check for the
> ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
> tests should stay as they are, I think.

I found the correct behavior in table '4.10.1 Card Status':

* OUT_OF_RANGE
  ============
  Type: E R X

  The command's argument was out of the allowed range for this card.

* ADDRESS_ERROR
  =============
  Type: E R X

  A misaligned address which did not match the block length was
  used in the command.

* WP_VIOLATION
  ============
  Type: E R X

  Set when the host attempts to write to a protected block or to
  the temporary or permanent write protected card.

With 'Type':

- E: Error bit.
- R: Detected and set for the actual command response.
- X: Detected and set during command execution. The host can get
     the status by issuing a command with R1 response.

Block Read
==========
[...]
When the last block of user area is read using CMD18, the host should
ignore OUT_OF_RANGE error that may occur even the sequence is correct.
If the host uses partial blocks whose accumulated length is not block
aligned and block misalignment is not allowed, the card shall detect
a block misalignment at the beginning of the first misaligned block,
set the ADDRESS_ERROR error bit in the status register, abort
transmission and wait in the Data State for a stop command.


So I understand we want OUT_OF_RANGE (returned via R1).
Philippe Mathieu-Daudé July 13, 2020, 4:49 p.m. UTC | #3 
On 7/13/20 6:36 PM, Philippe Mathieu-Daudé wrote:
> On 6/15/20 4:06 PM, Peter Maydell wrote:
>> On Fri, 5 Jun 2020 at 11:25, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>>
>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>
>>> Only move the state machine to ReceivingData if there is no
>>> pending error.  This avoids later OOB access while processing
>>> commands queued.
>>>
>>>   "SD Specifications Part 1 Physical Layer Simplified Spec. v3.01"
>>>
>>>   4.3.3 Data Read
>>>
>>>   Read command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>>   occurred and no data transfer is performed.
>>>
>>>   4.3.4 Data Write
>>>
>>>   Write command is rejected if BLOCK_LEN_ERROR or ADDRESS_ERROR
>>>   occurred and no data transfer is performed.
>>
>> It's not clear from the spec that this should also
>> apply to WP_VIOLATION errors. The text about WP_VIOLATION
>> suggests that it is handled by aborting the data transfer
>> (ie set the error bit, stay in receive-data state, wait for
>> a stop command, but ignore all further data transfer),
>> which is I think distinct from "rejecting" the command.
>>
>> If that theory is right then moving the check for the
>> ADDRESS_ERROR in this patch is correct but the WP_VIOLATION
>> tests should stay as they are, I think.
> 
> I found the correct behavior in table '4.10.1 Card Status':
> 
> * OUT_OF_RANGE
>   ============
>   Type: E R X
> 
>   The command's argument was out of the allowed range for this card.
> 
> * ADDRESS_ERROR
>   =============
>   Type: E R X
> 
>   A misaligned address which did not match the block length was
>   used in the command.
> 
> * WP_VIOLATION
>   ============
>   Type: E R X
> 
>   Set when the host attempts to write to a protected block or to
>   the temporary or permanent write protected card.
> 
> With 'Type':
> 
> - E: Error bit.
> - R: Detected and set for the actual command response.
> - X: Detected and set during command execution. The host can get
>      the status by issuing a command with R1 response.
> 
> Block Read
> ==========
> [...]
> When the last block of user area is read using CMD18, the host should
> ignore OUT_OF_RANGE error that may occur even the sequence is correct.
> If the host uses partial blocks whose accumulated length is not block
> aligned and block misalignment is not allowed, the card shall detect
> a block misalignment at the beginning of the first misaligned block,
> set the ADDRESS_ERROR error bit in the status register, abort
> transmission and wait in the Data State for a stop command.
> 
> 
> So I understand we want OUT_OF_RANGE (returned via R1).

We always returned ADDRESS_ERROR and guests never complained, so I don't
plan to modify this bit for 5.1. What matters is "command is rejected
if error occurred and no data transfer is performed".
diff mbox series

Patch

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index f1b12b49db..90d5ff6209 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -1150,13 +1150,15 @@  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 17:	/* CMD17:  READ_SINGLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->state = sd_sendingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
 
             if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
             }
+
+            sd->state = sd_sendingdata_state;
+            sd->data_start = addr;
+            sd->data_offset = 0;
             return sd_r1;
 
         default:
@@ -1167,13 +1169,15 @@  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
         switch (sd->state) {
         case sd_transfer_state:
-            sd->state = sd_sendingdata_state;
-            sd->data_start = addr;
-            sd->data_offset = 0;
 
             if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
             }
+
+            sd->state = sd_sendingdata_state;
+            sd->data_start = addr;
+            sd->data_offset = 0;
             return sd_r1;
 
         default:
@@ -1213,20 +1217,24 @@  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (sd->data_start + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+            if (sd->csd[14] & 0x30) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            if (sd->csd[14] & 0x30) {
-                sd->card_status |= WP_VIOLATION;
-            }
             return sd_r1;
 
         default:
@@ -1240,20 +1248,24 @@  static sd_rsp_type_t sd_normal_command(SDState *sd, SDRequest req)
             /* Writing in SPI mode not implemented.  */
             if (sd->spi)
                 break;
+
+            if (sd->data_start + sd->blk_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                return sd_r1;
+            }
+            if (sd_wp_addr(sd, sd->data_start)) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+            if (sd->csd[14] & 0x30) {
+                sd->card_status |= WP_VIOLATION;
+                return sd_r1;
+            }
+
             sd->state = sd_receivingdata_state;
             sd->data_start = addr;
             sd->data_offset = 0;
             sd->blk_written = 0;
-
-            if (sd->data_start + sd->blk_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-            }
-            if (sd_wp_addr(sd, sd->data_start)) {
-                sd->card_status |= WP_VIOLATION;
-            }
-            if (sd->csd[14] & 0x30) {
-                sd->card_status |= WP_VIOLATION;
-            }
             return sd_r1;
 
         default: