Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error

[Philippe Mathieu-Daudé]

On 7/5/20 12:26 AM, Philippe Mathieu-Daudé wrote:
> On 7/5/20 12:18 AM, Philippe Mathieu-Daudé wrote:
>> On 7/5/20 12:10 AM, Philippe Mathieu-Daudé wrote:
>>> On 7/4/20 1:42 AM, Philippe Mathieu-Daudé wrote:
>>>> On 7/3/20 5:16 PM, Philippe Mathieu-Daudé wrote:
>>>>> On 7/3/20 3:23 PM, Peter Maydell wrote:
>>>>>> On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé <f4bug@amsat.org> 
>>>>>> wrote:
>>>>>>>
>>>>>>> As we have no interest in the underlying block geometry,
>>>>>>> directly call blk_getlength(). We have to care about machines
>>>>>>> creating SD card with not drive attached (probably incorrect
>>>>>>> API use). Simply emit a warning when such Frankenstein cards
>>>>>>> of zero size are reset.
>>>>>>
>>>>>> Which machines create SD cards without a backing block device?
>>>>>
>>>>> The Aspeed machines:
>>>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg718116.html
>>>
>>> Also all boards using:
>>>
>>> hw/sd/milkymist-memcard.c:278:    /* FIXME use a qdev drive property
>>> instead of drive_get_next() */
>>> hw/sd/pl181.c:506:    /* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>> hw/sd/ssi-sd.c:253:    /* FIXME use a qdev drive property instead of
>>> drive_get_next() */
>>>
>>> I.e.:
>>>
>>> static void pl181_realize(DeviceState *dev, Error **errp)
>>> {
>>>     PL181State *s = PL181(dev);
>>>     DriveInfo *dinfo;
>>>
>>>     /* FIXME use a qdev drive property instead of drive_get_next() */
>>>     dinfo = drive_get_next(IF_SD);
>>>     s->card = sd_init(dinfo ? blk_by_legacy_dinfo(dinfo) : NULL, false);
>>>     if (s->card == NULL) {
>>>         error_setg(errp, "sd_init failed");
>>>     }
>>> }
>>
>> Doh I was pretty sure this series was merged:
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg514645.html
>>
>> Time to respin I guess, addressing your comment...
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg515866.html
> 
> Not straight forward at first glance, so probably too late for soft
> freeze.

I looked in backups of my previous computer and found the branch with
your comment addressed... Not sure why I never send the series, but
this explains why I was sure it was already fixed.

> 
> Meanwhile patches 1-8 are reviewed and sufficient to fix
> CVE-2020-13253. The problematic patch is #9, your "Check address is
> in range" suggestion. Patches 11-14 are also reviewed and can go in.
> 
> Peter, can you consider taking them via your ARM queue? If you prefer
> I can prepare a pull request.
> 
> I'll keep working on this during the next dev window.
> 
> Thanks,
> 
> Phil.
> 
>>
>>>
>>>>>
>>>>>> I have a feeling that also the monitor "change" and "eject"
>>>>>> commands can remove the backing block device from the SD card
>>>>>> object.
>>>>>
>>>>> This is what I wanted to talk about on IRC. This seems wrong to me,
>>>>> we should eject the card and destroy it, and recreate a new card
>>>>> when plugging in another backing block device.
>>>>>
>>>>> Keep the reparenting on the bus layer, not on the card.
>>>>
>>>> I was wrong, the current code is correct:
>>>>
>>>> void sdbus_reparent_card(SDBus *from, SDBus *to)
>>>> {
>>>>     SDState *card = get_card(from);
>>>>     SDCardClass *sc;
>>>>     bool readonly;
>>>>
>>>>     /* We directly reparent the card object rather than implementing this
>>>>      * as a hotpluggable connection because we don't want to expose SD 
>>>> cards
>>>>      * to users as being hotpluggable, and we can get away with it in this
>>>>      * limited use case. This could perhaps be implemented more cleanly in
>>>>      * future by adding support to the hotplug infrastructure for "device
>>>>      * can be hotplugged only via code, not by user".
>>>>      */
>>>>
>>>>     if (!card) {
>>>>         return;
>>>>     }
>>>>
>>>>     sc = SD_CARD_GET_CLASS(card);
>>>>     readonly = sc->get_readonly(card);
>>>>
>>>>     sdbus_set_inserted(from, false);
>>>>     qdev_set_parent_bus(DEVICE(card), &to->qbus);
>>>>     sdbus_set_inserted(to, true);
>>>>     sdbus_set_readonly(to, readonly);
>>>> }
>>>>
>>>> What I don't understand is why create a sdcard with no block backend.
>>>>
>>>> Maybe this is old code before the null-co block backend existed? I
>>>> haven't checked the git history yet.
>>>>
>>>> I'll try to restrict sdcard with only block backend and see if
>>>> something break (I doubt) at least it simplifies the code.
>>>> But I need to update the Aspeed machines first.
>>>>
>>>> The problem when not using block backend, is the size is 0,
>>>> so the next patch abort in sd_reset() due to:
>>>>
>>>>   static uint64_t sd_addr_to_wpnum(SDState *sd, uint64_t addr)
>>>>   {
>>>>       assert(addr < sd->size);
>>>>
>>>
>>>
>>
>