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 <cyruscy...@gmail.com>
On Tue, Aug 30, 2022 at 2:38 PM Gerd Hoffmann <kra...@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 <cyruscy...@gmail.com>
> Signed-off-by: Gerd Hoffmann <kra...@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
>
