Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
On Mon, 6 Jul 2020 at 06:52, Markus Armbruster wrote: > This is TYPE_SD_CARD's ("sd-card"). What exactly does that device > model? It is I think supposed to be an SD card. The modelling may well be odd -- it's a qomification of a pre-existing abstraction layer that predates QOM and qbus. > If it's the sd-card, then the modelling is odd. A physical SD card gets > plugged and unplugged as a whole. This model can't. Instead, you > change "media". Isn't the SD card the medium? I suspect this is because the requirement to change SD card contents existed and was implemented via changing the block backend before the concept of a hot-pluggable QOM object even existed. So now we have something we wish to maintain back-compat for (in terms of monitor commands to change/eject doing what they've always done), some SD controllers which haven't been entirely QOMified (I see Philippe just posted a series to do that cleanup, which is great), and some parts of the code which have been updated but by people (ie me) who understand the QOM parts of things but don't have any understanding of the operations the block layer provides and so converted the device/SD code's API to the rest of QEMU but left its interaction with the block layer using the same APIs that the pre-QOM code used. > The other device models with removable media (IDE & SCSI CD drives, > floppy drives) model the receptacle for media. On media change, the > drive stays put, and only the medium changes, both in the physical world > and in our virtual world. > > Our "sd-card" seems to be an "SD card drive". But then I wonder what > the thing at the other end of TYPE_SD_BUS ("sd-bus") actually models. > Any ideas? The thing at the other end of the TYPE_SD_BUS is the SD controller (ie the bit of hardware which presents a registers-and-interrupts interface to the system on one end and has an SD card slot on the other end). The link between them has operations like "do command", "read data", "write data", "get readonly status", which are abstractions of the h/w operations on the pins between an SD controller and the SD card. It also has an operation for "tell the controller I'm actually an inserted card". So the "sd-card" device implements the logic that in real h/w really is inside the microcontroller on the SD card (and so is identical for all SD cards regardless of machine type), and the sd controller device implements the logic that's in the sd controller chip in the machine proper (which can vary a lot between machines, from very-simple "software does all the work and the controller just waves the wires up and down" to much more sophisticated setups). thanks -- PMM
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
Peter Maydell writes: > On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé 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? > > I have a feeling that also the monitor "change" and "eject" > commands can remove the backing block device from the SD card > object. Correct: static const BlockDevOps sd_block_ops = { .change_media_cb = sd_cardchange, }; This is TYPE_SD_CARD's ("sd-card"). What exactly does that device model? If it's the sd-card, then the modelling is odd. A physical SD card gets plugged and unplugged as a whole. This model can't. Instead, you change "media". Isn't the SD card the medium? The other device models with removable media (IDE & SCSI CD drives, floppy drives) model the receptacle for media. On media change, the drive stays put, and only the medium changes, both in the physical world and in our virtual world. Our "sd-card" seems to be an "SD card drive". But then I wonder what the thing at the other end of TYPE_SD_BUS ("sd-bus") actually models. Any ideas?
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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é >> 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), >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); >>> >>> >> >
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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é > 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), >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); >>> >> >> >
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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é 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 > >>> 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), >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); >> > >
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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é >>> 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"); } } >> >>> 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), >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); >
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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é 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 > >> 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), >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);
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
On 7/3/20 3:23 PM, Peter Maydell wrote: > On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé 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 > 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.
Re: [PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
On Tue, 30 Jun 2020 at 14:39, Philippe Mathieu-Daudé 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? I have a feeling that also the monitor "change" and "eject" commands can remove the backing block device from the SD card object. thanks -- PMM
[PATCH v7 09/17] hw/sd/sdcard: Special case the -ENOMEDIUM error
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. Signed-off-by: Philippe Mathieu-Daudé --- hw/sd/sd.c | 28 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/hw/sd/sd.c b/hw/sd/sd.c index e5adcc8055..548745614e 100644 --- a/hw/sd/sd.c +++ b/hw/sd/sd.c @@ -545,18 +545,30 @@ static inline uint64_t sd_addr_to_wpnum(uint64_t addr) static void sd_reset(DeviceState *dev) { SDState *sd = SD_CARD(dev); -uint64_t size; -uint64_t sect; trace_sdcard_reset(); if (sd->blk) { -blk_get_geometry(sd->blk, ); -} else { -sect = 0; -} -size = sect << 9; +int64_t size = blk_getlength(sd->blk); + +if (size == -ENOMEDIUM) { +/* + * FIXME blk should be set once per device in sd_realize(), + * and we shouldn't be checking it in sed_reset(). But this + * is how the reparent currently works. + */ +char *id = object_get_canonical_path_component(OBJECT(dev)); + +warn_report("sd-card '%s' created with no drive.", +id ? id : "unknown"); +g_free(id); +size = 0; +} +assert(size >= 0); +sd->size = size; +} else { +sd->size = 0; +} -sd->size = size; sd->state = sd_idle_state; sd->rca = 0x; sd_set_ocr(sd); -- 2.21.3