Re: [PATCH 2/2] usb/msd: add usb_msd_fatal_error() and fix guest-triggerable assert

[Qiang Liu]

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
>