* [PATCH 0/2] usb/msd: fix guest-triggerable assert @ 2022-08-30 6:38 Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 1/2] usb/msd: move usb_msd_packet_complete() Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert Gerd Hoffmann 0 siblings, 2 replies; 4+ messages in thread From: Gerd Hoffmann @ 2022-08-30 6:38 UTC (permalink / raw) To: qemu-devel; +Cc: Gerd Hoffmann Gerd Hoffmann (2): usb/msd: move usb_msd_packet_complete() usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert include/hw/usb/msd.h | 1 + hw/usb/dev-storage.c | 56 ++++++++++++++++++++++++++++++++++---------- hw/usb/trace-events | 1 + 3 files changed, 45 insertions(+), 13 deletions(-) -- 2.37.2 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] usb/msd: move usb_msd_packet_complete() 2022-08-30 6:38 [PATCH 0/2] usb/msd: fix guest-triggerable assert Gerd Hoffmann @ 2022-08-30 6:38 ` Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert Gerd Hoffmann 1 sibling, 0 replies; 4+ messages in thread From: Gerd Hoffmann @ 2022-08-30 6:38 UTC (permalink / raw) To: qemu-devel; +Cc: Gerd Hoffmann Change ordering to avoid adding forward declarations in following patches. Fix comment code style while being at it. No functional change. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/usb/dev-storage.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index dca62d544fe9..4485a2411797 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -177,6 +177,20 @@ static const USBDesc desc = { .str = desc_strings, }; +static void usb_msd_packet_complete(MSDState *s) +{ + USBPacket *p = s->packet; + + /* + * Set s->packet to NULL before calling usb_packet_complete + * because another request may be issued before + * usb_packet_complete returns. + */ + trace_usb_msd_packet_complete(); + s->packet = NULL; + usb_packet_complete(&s->dev, p); +} + static void usb_msd_copy_data(MSDState *s, USBPacket *p) { uint32_t len; @@ -208,18 +222,6 @@ static void usb_msd_send_status(MSDState *s, USBPacket *p) memset(&s->csw, 0, sizeof(s->csw)); } -static void usb_msd_packet_complete(MSDState *s) -{ - USBPacket *p = s->packet; - - /* Set s->packet to NULL before calling usb_packet_complete - because another request may be issued before - usb_packet_complete returns. */ - trace_usb_msd_packet_complete(); - s->packet = NULL; - usb_packet_complete(&s->dev, p); -} - void usb_msd_transfer_data(SCSIRequest *req, uint32_t len) { MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent); -- 2.37.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert 2022-08-30 6:38 [PATCH 0/2] usb/msd: fix guest-triggerable assert Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 1/2] usb/msd: move usb_msd_packet_complete() Gerd Hoffmann @ 2022-08-30 6:38 ` Gerd Hoffmann 2022-08-30 7:12 ` Qiang Liu 1 sibling, 1 reply; 4+ messages in thread From: Gerd Hoffmann @ 2022-08-30 6:38 UTC (permalink / raw) To: qemu-devel; +Cc: Gerd Hoffmann, Qiang Liu Add handler for fatal errors. Moves device into error state where it stops responding until the guest resets it. Guest can send illegal requests where scsi command and usb packet transfer directions are inconsistent. Use the new usb_msd_fatal_error() function instead of assert() in that case. Reported-by: Qiang Liu <cyruscyliu@gmail.com> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- include/hw/usb/msd.h | 1 + hw/usb/dev-storage.c | 30 +++++++++++++++++++++++++++++- hw/usb/trace-events | 1 + 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h index 54e9f38bda46..f9fd862b529a 100644 --- a/include/hw/usb/msd.h +++ b/include/hw/usb/msd.h @@ -40,6 +40,7 @@ struct MSDState { bool removable; bool commandlog; SCSIDevice *scsi_dev; + bool needs_reset; }; typedef struct MSDState MSDState; diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c index 4485a2411797..3928209b8249 100644 --- a/hw/usb/dev-storage.c +++ b/hw/usb/dev-storage.c @@ -191,6 +191,23 @@ static void usb_msd_packet_complete(MSDState *s) usb_packet_complete(&s->dev, p); } +static void usb_msd_fatal_error(MSDState *s) +{ + trace_usb_msd_fatal_error(); + + if (s->packet) { + s->packet->status = USB_RET_STALL; + usb_msd_packet_complete(s); + } + + /* + * Guest messed up up device state with illegal requests. Go + * ignore any requests until the guests resets the device (and + * brings it into a known state that way). + */ + s->needs_reset = true; +} + static void usb_msd_copy_data(MSDState *s, USBPacket *p) { uint32_t len; @@ -227,7 +244,11 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len) MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent); USBPacket *p = s->packet; - assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV)); + if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) { + usb_msd_fatal_error(s); + return; + } + s->scsi_len = len; s->scsi_off = 0; if (p) { @@ -317,6 +338,8 @@ void usb_msd_handle_reset(USBDevice *dev) memset(&s->csw, 0, sizeof(s->csw)); s->mode = USB_MSDM_CBW; + + s->needs_reset = false; } static void usb_msd_handle_control(USBDevice *dev, USBPacket *p, @@ -382,6 +405,11 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p) SCSIDevice *scsi_dev; uint32_t len; + if (s->needs_reset) { + p->status = USB_RET_STALL; + return; + } + switch (p->pid) { case USB_TOKEN_OUT: if (devep != 2) diff --git a/hw/usb/trace-events b/hw/usb/trace-events index 914ca7166829..b65269892c5e 100644 --- a/hw/usb/trace-events +++ b/hw/usb/trace-events @@ -263,6 +263,7 @@ usb_msd_packet_complete(void) "" usb_msd_cmd_submit(unsigned lun, unsigned tag, unsigned flags, unsigned len, unsigned data_len) "lun %u, tag 0x%x, flags 0x%08x, len %d, data-len %d" usb_msd_cmd_complete(unsigned status, unsigned tag) "status %d, tag 0x%x" usb_msd_cmd_cancel(unsigned tag) "tag 0x%x" +usb_msd_fatal_error(void) "" # dev-uas.c usb_uas_reset(int addr) "dev %d" -- 2.37.2 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert 2022-08-30 6:38 ` [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert Gerd Hoffmann @ 2022-08-30 7:12 ` Qiang Liu 0 siblings, 0 replies; 4+ messages in thread From: Qiang Liu @ 2022-08-30 7:12 UTC (permalink / raw) To: Gerd Hoffmann; +Cc: open list:All patches CC here I've checked out the patches and re-run my PoC. I see no crash anymore. I also fuzzed the latest code for a while (with the patches) and I saw no related crashes. Tested-by: Qiang Liu <cyruscyliu@gmail.com> On Tue, Aug 30, 2022 at 2:38 PM Gerd Hoffmann <kraxel@redhat.com> wrote: > > Add handler for fatal errors. Moves device into error state where it > stops responding until the guest resets it. > > Guest can send illegal requests where scsi command and usb packet > transfer directions are inconsistent. Use the new usb_msd_fatal_error() > function instead of assert() in that case. > > Reported-by: Qiang Liu <cyruscyliu@gmail.com> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- > include/hw/usb/msd.h | 1 + > hw/usb/dev-storage.c | 30 +++++++++++++++++++++++++++++- > hw/usb/trace-events | 1 + > 3 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/include/hw/usb/msd.h b/include/hw/usb/msd.h > index 54e9f38bda46..f9fd862b529a 100644 > --- a/include/hw/usb/msd.h > +++ b/include/hw/usb/msd.h > @@ -40,6 +40,7 @@ struct MSDState { > bool removable; > bool commandlog; > SCSIDevice *scsi_dev; > + bool needs_reset; > }; > > typedef struct MSDState MSDState; > diff --git a/hw/usb/dev-storage.c b/hw/usb/dev-storage.c > index 4485a2411797..3928209b8249 100644 > --- a/hw/usb/dev-storage.c > +++ b/hw/usb/dev-storage.c > @@ -191,6 +191,23 @@ static void usb_msd_packet_complete(MSDState *s) > usb_packet_complete(&s->dev, p); > } > > +static void usb_msd_fatal_error(MSDState *s) > +{ > + trace_usb_msd_fatal_error(); > + > + if (s->packet) { > + s->packet->status = USB_RET_STALL; > + usb_msd_packet_complete(s); > + } > + > + /* > + * Guest messed up up device state with illegal requests. Go > + * ignore any requests until the guests resets the device (and > + * brings it into a known state that way). > + */ > + s->needs_reset = true; > +} > + > static void usb_msd_copy_data(MSDState *s, USBPacket *p) > { > uint32_t len; > @@ -227,7 +244,11 @@ void usb_msd_transfer_data(SCSIRequest *req, uint32_t len) > MSDState *s = DO_UPCAST(MSDState, dev.qdev, req->bus->qbus.parent); > USBPacket *p = s->packet; > > - assert((s->mode == USB_MSDM_DATAOUT) == (req->cmd.mode == SCSI_XFER_TO_DEV)); > + if ((s->mode == USB_MSDM_DATAOUT) != (req->cmd.mode == SCSI_XFER_TO_DEV)) { > + usb_msd_fatal_error(s); > + return; > + } > + > s->scsi_len = len; > s->scsi_off = 0; > if (p) { > @@ -317,6 +338,8 @@ void usb_msd_handle_reset(USBDevice *dev) > > memset(&s->csw, 0, sizeof(s->csw)); > s->mode = USB_MSDM_CBW; > + > + s->needs_reset = false; > } > > static void usb_msd_handle_control(USBDevice *dev, USBPacket *p, > @@ -382,6 +405,11 @@ static void usb_msd_handle_data(USBDevice *dev, USBPacket *p) > SCSIDevice *scsi_dev; > uint32_t len; > > + if (s->needs_reset) { > + p->status = USB_RET_STALL; > + return; > + } > + > switch (p->pid) { > case USB_TOKEN_OUT: > if (devep != 2) > diff --git a/hw/usb/trace-events b/hw/usb/trace-events > index 914ca7166829..b65269892c5e 100644 > --- a/hw/usb/trace-events > +++ b/hw/usb/trace-events > @@ -263,6 +263,7 @@ usb_msd_packet_complete(void) "" > usb_msd_cmd_submit(unsigned lun, unsigned tag, unsigned flags, unsigned len, unsigned data_len) "lun %u, tag 0x%x, flags 0x%08x, len %d, data-len %d" > usb_msd_cmd_complete(unsigned status, unsigned tag) "status %d, tag 0x%x" > usb_msd_cmd_cancel(unsigned tag) "tag 0x%x" > +usb_msd_fatal_error(void) "" > > # dev-uas.c > usb_uas_reset(int addr) "dev %d" > -- > 2.37.2 > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-08-30 7:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-08-30 6:38 [PATCH 0/2] usb/msd: fix guest-triggerable assert Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 1/2] usb/msd: move usb_msd_packet_complete() Gerd Hoffmann 2022-08-30 6:38 ` [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert Gerd Hoffmann 2022-08-30 7:12 ` Qiang Liu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).