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

[Philippe Mathieu-Daudé]

On 10/26/20 2:40 AM, Bin Meng wrote:
> 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?

You are not missing anything, I was confused by the spec.

Patch applied.

Regards,

Phil.