Re: [PATCH] hw/sd: Fix 2 GiB card CSD register values

[Philippe Mathieu-Daudé]

On 10/25/20 4:23 PM, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
>
> Per the SD spec, to indicate a 2 GiB card, BLOCK_LEN shall be 1024
> bytes, hence the READ_BL_LEN field in the CSD register shall be 10
> instead of 9.
>
> This fixes the acceptance test error for the NetBSD 9.0 test of the
> Orange Pi PC that has an expanded SD card image of 2 GiB size.
>
> Fixes: 6d2d4069c47e ("hw/sd: Correct the maximum size of a Standard Capacity SD 
> Memory Card")
> Reported-by: Niek Linnenbank <nieklinnenbank@gmail.com>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>   hw/sd/sd.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/hw/sd/sd.c b/hw/sd/sd.c
> index bd10ec8fc4..732fcb5ff0 100644
> --- a/hw/sd/sd.c
> +++ b/hw/sd/sd.c
> @@ -386,10 +386,17 @@ static const uint8_t sd_csd_rw_mask[16] = {
>   
>   static void sd_set_csd(SDState *sd, uint64_t size)
>   {
> -    uint32_t csize = (size >> (CMULT_SHIFT + HWBLOCK_SHIFT)) - 1;
> +    int hwblock_shift = HWBLOCK_SHIFT;
> +    uint32_t csize;
>       uint32_t sectsize = (1 << (SECTOR_SHIFT + 1)) - 1;
>       uint32_t wpsize = (1 << (WPGROUP_SHIFT + 1)) - 1;
>   
> +    /* To indicate 2 GiB card, BLOCK_LEN shall be 1024 bytes */
> +    if (size == SDSC_MAX_CAPACITY) {
> +        hwblock_shift += 1;

This is going in the good direction, however now we have an huge
security hole, as SDState::data[] is 512 bytes, and you announce the
guest it can use 1024 bytes. See sd_blk_read() and sd_blk_write().

Now SDState::data[] is migrated, so this isn't an easy field to
modify without breaking compatibility again :(

I've been working on a more robust approach today, doing some cleanup
first. I'll send it during the next days hopefully.

> +    }
> +    csize = (size >> (CMULT_SHIFT + hwblock_shift)) - 1;
> +
>       if (size <= SDSC_MAX_CAPACITY) { /* Standard Capacity SD */
>           sd->csd[0] = 0x00; /* CSD structure */
>           sd->csd[1] = 0x26; /* Data read access-time-1 */
> @@ -397,7 +404,7 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>           sd->csd[3] = 0x32;      /* Max. data transfer rate: 25 MHz */
>           sd->csd[4] = 0x5f; /* Card Command Classes */
>           sd->csd[5] = 0x50 |        /* Max. read data block length */
> -            HWBLOCK_SHIFT;
> +            hwblock_shift;
>           sd->csd[6] = 0xe0 |        /* Partial block for read allowed */
>               ((csize >> 10) & 0x03);
>           sd->csd[7] = 0x00 |        /* Device size */
> @@ -411,9 +418,9 @@ static void sd_set_csd(SDState *sd, uint64_t size)
>           sd->csd[11] = 0x00 |       /* Write protect group size */
>               ((sectsize << 7) & 0x80) | wpsize;
>           sd->csd[12] = 0x90 |       /* Write speed factor */
> -            (HWBLOCK_SHIFT >> 2);
> +            (hwblock_shift >> 2);
>           sd->csd[13] = 0x20 |       /* Max. write data block length */
> -            ((HWBLOCK_SHIFT << 6) & 0xc0);
> +            ((hwblock_shift << 6) & 0xc0);
>           sd->csd[14] = 0x00;        /* File format group */
>       } else {                  /* SDHC */
>           size /= 512 * KiB;