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

[Bin Meng]

Hi Philippe,

On Mon, Oct 26, 2020 at 2:56 AM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> 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().

Currently sd_normal_command() ensures that the maximum block length is
512 bytes as the response to cmd 16.

The spec also says in chapter4.3.2 2 GByte Card:

"However, the Block Length, set by CMD16, shall be up to 512 bytes to
keep consistency with 512 bytes Maximum Block Length cards (Less than
or equal 2GBytes cards).

I don't see any issue here. Am I missing anything?

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

Regards,
Bin