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é <f4...@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. 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); >>> >> >> >