Patchwork [2/7] sd: fix card size checking on R/W accesses

login
register
mail settings
Submitter Vincent Palatin
Date July 25, 2011, 11:19 p.m.
Message ID <1311635951-11047-3-git-send-email-vpalatin@chromium.org>
Download mbox | patch
Permalink /patch/106762/
State New
Headers show

Comments

Vincent Palatin - July 25, 2011, 11:19 p.m.
We need to check that we are not crossing the boundaries of the card for
the current access not for the next one which might not happen.

Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
---
 hw/sd.c |   22 ++++++++++++----------
 1 files changed, 12 insertions(+), 10 deletions(-)
andrzej zaborowski - July 30, 2011, 5:50 a.m.
Hi Vincent,

On 26 July 2011 01:19, Vincent Palatin <vpalatin@chromium.org> wrote:
> We need to check that we are not crossing the boundaries of the card for
> the current access not for the next one which might not happen.
>
> Signed-off-by: Vincent Palatin <vpalatin@chromium.org>
> ---
>  hw/sd.c |   22 ++++++++++++----------
>  1 files changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/hw/sd.c b/hw/sd.c
> index f48d589..de477fe 100644
> --- a/hw/sd.c
> +++ b/hw/sd.c
> @@ -1451,11 +1451,6 @@ void sd_write_data(SDState *sd, uint8_t value)
>         sd->data[sd->data_offset ++] = value;
>         if (sd->data_offset >= sd->blk_len) {
>             /* TODO: Check CRC before committing */
> -            sd->state = sd_programming_state;
> -            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
> -            sd->blk_written ++;
> -            sd->data_start += sd->blk_len;
> -            sd->data_offset = 0;
>             if (sd->data_start + sd->blk_len > sd->size) {
>                 sd->card_status |= ADDRESS_ERROR;
>                 break;
> @@ -1464,6 +1459,11 @@ void sd_write_data(SDState *sd, uint8_t value)
>                 sd->card_status |= WP_VIOLATION;
>                 break;
>             }
> +            sd->state = sd_programming_state;
> +            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
> +            sd->blk_written ++;
> +            sd->data_start += sd->blk_len;
> +            sd->data_offset = 0;
>             sd->csd[14] |= 0x40;
>
>             /* Bzzzzzzztt .... Operation complete.  */
> @@ -1606,17 +1606,19 @@ uint8_t sd_read_data(SDState *sd)
>         break;
>
>     case 18:   /* CMD18:  READ_MULTIPLE_BLOCK */
> -        if (sd->data_offset == 0)
> +        if (sd->data_offset == 0) {
> +            if (sd->data_start + io_len > sd->size) {
> +                sd->card_status |= ADDRESS_ERROR;
> +                ret = 0;
> +                break;
> +            }
>             BLK_READ_BLOCK(sd->data_start, io_len);
> +        }
>         ret = sd->data[sd->data_offset ++];
>
>         if (sd->data_offset >= io_len) {
>             sd->data_start += io_len;
>             sd->data_offset = 0;
> -            if (sd->data_start + io_len > sd->size) {
> -                sd->card_status |= ADDRESS_ERROR;
> -                break;
> -            }
>         }
>         break;

I have to look more closely at the spec but it looks like
ADDRESS_ERROR might not even be the right error code for when we
read/write outside of the memory, because the spec says it's for
incorrect alignment.  Also the first chunk of this patch is
unnecessary now because I committed a patch from Peter Maydell that
did the same thing.

I pushed your patch 01 in the series, thanks.

Cheers

Patch

diff --git a/hw/sd.c b/hw/sd.c
index f48d589..de477fe 100644
--- a/hw/sd.c
+++ b/hw/sd.c
@@ -1451,11 +1451,6 @@  void sd_write_data(SDState *sd, uint8_t value)
         sd->data[sd->data_offset ++] = value;
         if (sd->data_offset >= sd->blk_len) {
             /* TODO: Check CRC before committing */
-            sd->state = sd_programming_state;
-            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
-            sd->blk_written ++;
-            sd->data_start += sd->blk_len;
-            sd->data_offset = 0;
             if (sd->data_start + sd->blk_len > sd->size) {
                 sd->card_status |= ADDRESS_ERROR;
                 break;
@@ -1464,6 +1459,11 @@  void sd_write_data(SDState *sd, uint8_t value)
                 sd->card_status |= WP_VIOLATION;
                 break;
             }
+            sd->state = sd_programming_state;
+            BLK_WRITE_BLOCK(sd->data_start, sd->data_offset);
+            sd->blk_written ++;
+            sd->data_start += sd->blk_len;
+            sd->data_offset = 0;
             sd->csd[14] |= 0x40;
 
             /* Bzzzzzzztt .... Operation complete.  */
@@ -1606,17 +1606,19 @@  uint8_t sd_read_data(SDState *sd)
         break;
 
     case 18:	/* CMD18:  READ_MULTIPLE_BLOCK */
-        if (sd->data_offset == 0)
+        if (sd->data_offset == 0) {
+            if (sd->data_start + io_len > sd->size) {
+                sd->card_status |= ADDRESS_ERROR;
+                ret = 0;
+                break;
+            }
             BLK_READ_BLOCK(sd->data_start, io_len);
+        }
         ret = sd->data[sd->data_offset ++];
 
         if (sd->data_offset >= io_len) {
             sd->data_start += io_len;
             sd->data_offset = 0;
-            if (sd->data_start + io_len > sd->size) {
-                sd->card_status |= ADDRESS_ERROR;
-                break;
-            }
         }
         break;