| Message ID | 1462292979-12314-1-git-send-email-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | ce1647fc608e8193b416a08da633019de611199c |
| Headers | show |
On 05/04/16 02:58, Tian, Feng wrote: > Laszlo, > > Could you explain more? > > Usb Flash drive is managed by UsbMassStorage driver, which is irrelevant with UefiScsiLib lib or ScsiDisk driver. Yeah, sorry about missing that part of the context. So, the environment is the following: - The USB flash drive with VendorId/ProductId 0x1516/0x6221 is plugged into a Linux host. - The usb-storage driver in Linux presents the USB flash drive to the rest of the system as a SCSI disk. The device node that is generated for it looks like /dev/sd*, for example, /dev/sdi. - This device node can be used like any other SCSI device. - Using the virtio-scsi HBA, QEMU can provide a virtual machine with various types of virtual SCSI devices. One of those is "scsi-block", which is also known as "SCSI passthrough". It means that most of the SCSI commands are forwarded transparently from guest device to physical host device (/dev/sdi, for example), using the SG_IO ioctl() call. Only a few SCSI commands are captured and emulated by QEMU. This feature is useful e.g. for burning physical optical media, or writing physical tapes, from a virtual machine. - In this scenario, the USB stick that is plugged in the host is exposed to the guest with this feature. So, when the ScsiDiskInquiryDevice() function runs in the guest (as part of OVMF), the INQUIRY command (asking for the Supported VPD Pages" VPD page) is forwarded to the USB stick on the host. And that device returns garbage. - It is important to note that the "garbage" is not inserted by either the host kernel's usb-storage driver, or by QEMU. Even without virtualization, the host kernel consciously avoids asking SCSI disks that are actually backed by USB mass-storage devices for VPD pages, because the kernel developers have realized that most (if not all) of USB mass-storage devices fail to respond correctly. Please see this host kernel commit: http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09b6b51b0b6c - In <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c14>, I demonstrate what happens when such an INQUIRY is sent manually to the SCSI disk that is backed by the USB flash drive. For a genuine SCSI disk, the command completes: # sg_vpd -v /dev/sda Supported VPD pages VPD page: inquiry cdb: 12 01 00 00 fc 00 [PQual=0 Peripheral device type: disk] Supported VPD pages [sv] Unit serial number [sn] Device identification [di] ATA information (SAT) [ai] Block limits (SBC) [bl] Block device characteristics (SBC) [bdc] Logical block provisioning (SBC) [lbpv] Buth the USB flash drive responds with garbage: # sg_vpd -v /dev/sdi Supported VPD pages VPD page: inquiry cdb: 12 01 00 00 fc 00 invalid VPD response; probably a STANDARD INQUIRY response First 32 bytes of bad response 00 00 80 06 02 1f 00 00 00 00 00 00 00 00 00 00 00 ................ 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ fetching VPD page failed - So this patch is motivated by the case when a USB mass-storage device is presented as a SCSI disk, directly on the (virtual) hardware level, and it provides a bogus 0x00 VPD page. Thanks Laszlo > Thanks > Feng > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Wednesday, May 4, 2016 12:30 AM > To: edk2-devel-01 <edk2-devel@ml01.01.org> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com> > Subject: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: cope with broken "Supported VPD Pages" VPD page > > The USB flash drive with Vendor ID 0x1516 (CompUSA) and Product ID 0x6221 returns a broken "Supported VPD Pages" VPD page. In particular, the PageLength field has the invalid value 0x0602 (decimal 1538). > > This prevents the loop from terminating that scans for the Block Limits VPD page code in ScsiDiskInquiryDevice(): > > for (Index = 0; Index < PageLength; Index++) { > > because the Index variable has type UINT8, and it wraps from 255 to 0, without ever reaching PageLength (1538), and because EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD does not occur at offsets 0 through 255. > > * The fix is not to change the type of Index to UINT16 or a wider type. > Namely, section > > 7.8.14 Supported VPD Pages VPD page > > in the "SCSI Primary Commands - 4" (SPC-4) specification names the > following requirement: > > The supported VPD page list shall contain a list of all VPD page codes > (see 7.8) implemented by the logical unit in ascending order beginning > with page code 00h. > > Since page codes are 8-bit unsigned quantities, it follows that the > maximum size for the Supported VPD Pages VPD page is 0x100 bytes, in > which every possible page code (0x00 through 0xFF) will be found, before > the UINT8 offset wraps around. > > (EFI_SCSI_SUPPORTED_VPD_PAGES_VPD_PAGE.SupportedVpdPageList is correctly > sized as well, in "MdePkg/Include/IndustryStandard/Scsi.h".) > > * Instead, add sanity checks that enforce the above requirement. If the > device breaks the spec, simply fall back to the "Block Limits page > absent" case. > > Cc: Feng Tian <feng.tian@intel.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Star Zeng <star.zeng@intel.com> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1330955 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 37 ++++++++++++++++++++ > 1 file changed, 37 insertions(+) > > diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > index dfa5fa32e635..1b75d55231a6 100644 > --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c > @@ -1493,7 +1493,44 @@ ScsiDiskInquiryDevice ( > if (!EFI_ERROR (Status)) { > PageLength = (SupportedVpdPages->PageLength2 << 8) > | SupportedVpdPages->PageLength1; > + > + // > + // Sanity checks for coping with broken devices > + // > + if (PageLength > sizeof SupportedVpdPages->SupportedVpdPageList) { > + DEBUG ((EFI_D_WARN, > + "%a: invalid PageLength (%u) in Supported VPD Pages page\n", > + __FUNCTION__, (UINT32)PageLength)); > + PageLength = 0; > + } > + > + if ((PageLength > 0) && > + (SupportedVpdPages->SupportedVpdPageList[0] != > + EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)) { > + DEBUG ((EFI_D_WARN, > + "%a: Supported VPD Pages page doesn't start with code 0x%02x\n", > + __FUNCTION__, EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)); > + PageLength = 0; > + } > + > + // > + // Locate the code for the Block Limits VPD page > + // > for (Index = 0; Index < PageLength; Index++) { > + // > + // Sanity check > + // > + if ((Index > 0) && > + (SupportedVpdPages->SupportedVpdPageList[Index] <= > + SupportedVpdPages->SupportedVpdPageList[Index - 1])) { > + DEBUG ((EFI_D_WARN, > + "%a: non-ascending code in Supported VPD Pages page @ %u\n", > + __FUNCTION__, Index)); > + Index = 0; > + PageLength = 0; > + break; > + } > + > if (SupportedVpdPages->SupportedVpdPageList[Index] == EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD) { > break; > } > -- > 1.8.3.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 05/04/16 13:11, Laszlo Ersek wrote: > On 05/04/16 02:58, Tian, Feng wrote: >> Laszlo, >> >> Could you explain more? >> >> Usb Flash drive is managed by UsbMassStorage driver, which is irrelevant with UefiScsiLib lib or ScsiDisk driver. > > Yeah, sorry about missing that part of the context. > > So, the environment is the following: > > - The USB flash drive with VendorId/ProductId 0x1516/0x6221 is plugged > into a Linux host. > > - The usb-storage driver in Linux presents the USB flash drive to the > rest of the system as a SCSI disk. The device node that is generated for > it looks like /dev/sd*, for example, /dev/sdi. > > - This device node can be used like any other SCSI device. > > - Using the virtio-scsi HBA, QEMU can provide a virtual machine with > various types of virtual SCSI devices. One of those is "scsi-block", > which is also known as "SCSI passthrough". It means that most of the > SCSI commands are forwarded transparently from guest device to physical > host device (/dev/sdi, for example), using the SG_IO ioctl() call. Only > a few SCSI commands are captured and emulated by QEMU. > > This feature is useful e.g. for burning physical optical media, or > writing physical tapes, from a virtual machine. > > - In this scenario, the USB stick that is plugged in the host is exposed > to the guest with this feature. So, when the ScsiDiskInquiryDevice() > function runs in the guest (as part of OVMF), the INQUIRY command > (asking for the Supported VPD Pages" VPD page) is forwarded to the USB > stick on the host. And that device returns garbage. > > - It is important to note that the "garbage" is not inserted by either > the host kernel's usb-storage driver, or by QEMU. Even without > virtualization, the host kernel consciously avoids asking SCSI disks > that are actually backed by USB mass-storage devices for VPD pages, > because the kernel developers have realized that most (if not all) of > USB mass-storage devices fail to respond correctly. Please see this host > kernel commit: > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09b6b51b0b6c > > - In <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c14>, I > demonstrate what happens when such an INQUIRY is sent manually to the > SCSI disk that is backed by the USB flash drive. For a genuine SCSI > disk, the command completes: > > # sg_vpd -v /dev/sda > Supported VPD pages VPD page: > inquiry cdb: 12 01 00 00 fc 00 > [PQual=0 Peripheral device type: disk] > Supported VPD pages [sv] > Unit serial number [sn] > Device identification [di] > ATA information (SAT) [ai] > Block limits (SBC) [bl] > Block device characteristics (SBC) [bdc] > Logical block provisioning (SBC) [lbpv] > > Buth the USB flash drive responds with garbage: > > # sg_vpd -v /dev/sdi > Supported VPD pages VPD page: > inquiry cdb: 12 01 00 00 fc 00 > invalid VPD response; probably a STANDARD INQUIRY response > First 32 bytes of bad response > 00 00 80 06 02 1f 00 00 00 00 00 00 00 00 00 00 00 ................ > 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ > fetching VPD page failed > > - So this patch is motivated by the case when a USB mass-storage device > is presented as a SCSI disk, directly on the (virtual) hardware level, > and it provides a bogus 0x00 VPD page. I'm no longer certain that edk2's ScsiDiskDxe is the right place to fix this problem. It seems that the usb-storage driver in Linux could be the culprit -- it should be watching for INQUIRY commands with EVPD set, and reject them before forwarding them to the device. That is the method that complies with both SPC-4 and the UFI command spec. (And, edk2 handles this case correctly, already.) Please see <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c17>. So, for the time being, I'm withdrawing this patch. Thanks! Laszlo > Thanks > Laszlo > >> Thanks >> Feng >> >> -----Original Message----- >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek >> Sent: Wednesday, May 4, 2016 12:30 AM >> To: edk2-devel-01 <edk2-devel@ml01.01.org> >> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com> >> Subject: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: cope with broken "Supported VPD Pages" VPD page >> >> The USB flash drive with Vendor ID 0x1516 (CompUSA) and Product ID 0x6221 returns a broken "Supported VPD Pages" VPD page. In particular, the PageLength field has the invalid value 0x0602 (decimal 1538). >> >> This prevents the loop from terminating that scans for the Block Limits VPD page code in ScsiDiskInquiryDevice(): >> >> for (Index = 0; Index < PageLength; Index++) { >> >> because the Index variable has type UINT8, and it wraps from 255 to 0, without ever reaching PageLength (1538), and because EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD does not occur at offsets 0 through 255. >> >> * The fix is not to change the type of Index to UINT16 or a wider type. >> Namely, section >> >> 7.8.14 Supported VPD Pages VPD page >> >> in the "SCSI Primary Commands - 4" (SPC-4) specification names the >> following requirement: >> >> The supported VPD page list shall contain a list of all VPD page codes >> (see 7.8) implemented by the logical unit in ascending order beginning >> with page code 00h. >> >> Since page codes are 8-bit unsigned quantities, it follows that the >> maximum size for the Supported VPD Pages VPD page is 0x100 bytes, in >> which every possible page code (0x00 through 0xFF) will be found, before >> the UINT8 offset wraps around. >> >> (EFI_SCSI_SUPPORTED_VPD_PAGES_VPD_PAGE.SupportedVpdPageList is correctly >> sized as well, in "MdePkg/Include/IndustryStandard/Scsi.h".) >> >> * Instead, add sanity checks that enforce the above requirement. If the >> device breaks the spec, simply fall back to the "Block Limits page >> absent" case. >> >> Cc: Feng Tian <feng.tian@intel.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >> Cc: Star Zeng <star.zeng@intel.com> >> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1330955 >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <lersek@redhat.com> >> --- >> MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 37 ++++++++++++++++++++ >> 1 file changed, 37 insertions(+) >> >> diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c >> index dfa5fa32e635..1b75d55231a6 100644 >> --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c >> +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c >> @@ -1493,7 +1493,44 @@ ScsiDiskInquiryDevice ( >> if (!EFI_ERROR (Status)) { >> PageLength = (SupportedVpdPages->PageLength2 << 8) >> | SupportedVpdPages->PageLength1; >> + >> + // >> + // Sanity checks for coping with broken devices >> + // >> + if (PageLength > sizeof SupportedVpdPages->SupportedVpdPageList) { >> + DEBUG ((EFI_D_WARN, >> + "%a: invalid PageLength (%u) in Supported VPD Pages page\n", >> + __FUNCTION__, (UINT32)PageLength)); >> + PageLength = 0; >> + } >> + >> + if ((PageLength > 0) && >> + (SupportedVpdPages->SupportedVpdPageList[0] != >> + EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)) { >> + DEBUG ((EFI_D_WARN, >> + "%a: Supported VPD Pages page doesn't start with code 0x%02x\n", >> + __FUNCTION__, EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)); >> + PageLength = 0; >> + } >> + >> + // >> + // Locate the code for the Block Limits VPD page >> + // >> for (Index = 0; Index < PageLength; Index++) { >> + // >> + // Sanity check >> + // >> + if ((Index > 0) && >> + (SupportedVpdPages->SupportedVpdPageList[Index] <= >> + SupportedVpdPages->SupportedVpdPageList[Index - 1])) { >> + DEBUG ((EFI_D_WARN, >> + "%a: non-ascending code in Supported VPD Pages page @ %u\n", >> + __FUNCTION__, Index)); >> + Index = 0; >> + PageLength = 0; >> + break; >> + } >> + >> if (SupportedVpdPages->SupportedVpdPageList[Index] == EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD) { >> break; >> } >> -- >> 1.8.3.1 >> >> _______________________________________________ >> edk2-devel mailing list >> edk2-devel@lists.01.org >> https://lists.01.org/mailman/listinfo/edk2-devel >> > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 05/04/16 18:03, Laszlo Ersek wrote: > On 05/04/16 13:11, Laszlo Ersek wrote: >> On 05/04/16 02:58, Tian, Feng wrote: >>> Laszlo, >>> >>> Could you explain more? >>> >>> Usb Flash drive is managed by UsbMassStorage driver, which is irrelevant with UefiScsiLib lib or ScsiDisk driver. >> >> Yeah, sorry about missing that part of the context. >> >> So, the environment is the following: >> >> - The USB flash drive with VendorId/ProductId 0x1516/0x6221 is plugged >> into a Linux host. >> >> - The usb-storage driver in Linux presents the USB flash drive to the >> rest of the system as a SCSI disk. The device node that is generated for >> it looks like /dev/sd*, for example, /dev/sdi. >> >> - This device node can be used like any other SCSI device. >> >> - Using the virtio-scsi HBA, QEMU can provide a virtual machine with >> various types of virtual SCSI devices. One of those is "scsi-block", >> which is also known as "SCSI passthrough". It means that most of the >> SCSI commands are forwarded transparently from guest device to physical >> host device (/dev/sdi, for example), using the SG_IO ioctl() call. Only >> a few SCSI commands are captured and emulated by QEMU. >> >> This feature is useful e.g. for burning physical optical media, or >> writing physical tapes, from a virtual machine. >> >> - In this scenario, the USB stick that is plugged in the host is exposed >> to the guest with this feature. So, when the ScsiDiskInquiryDevice() >> function runs in the guest (as part of OVMF), the INQUIRY command >> (asking for the Supported VPD Pages" VPD page) is forwarded to the USB >> stick on the host. And that device returns garbage. >> >> - It is important to note that the "garbage" is not inserted by either >> the host kernel's usb-storage driver, or by QEMU. Even without >> virtualization, the host kernel consciously avoids asking SCSI disks >> that are actually backed by USB mass-storage devices for VPD pages, >> because the kernel developers have realized that most (if not all) of >> USB mass-storage devices fail to respond correctly. Please see this host >> kernel commit: >> >> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=09b6b51b0b6c >> >> - In <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c14>, I >> demonstrate what happens when such an INQUIRY is sent manually to the >> SCSI disk that is backed by the USB flash drive. For a genuine SCSI >> disk, the command completes: >> >> # sg_vpd -v /dev/sda >> Supported VPD pages VPD page: >> inquiry cdb: 12 01 00 00 fc 00 >> [PQual=0 Peripheral device type: disk] >> Supported VPD pages [sv] >> Unit serial number [sn] >> Device identification [di] >> ATA information (SAT) [ai] >> Block limits (SBC) [bl] >> Block device characteristics (SBC) [bdc] >> Logical block provisioning (SBC) [lbpv] >> >> Buth the USB flash drive responds with garbage: >> >> # sg_vpd -v /dev/sdi >> Supported VPD pages VPD page: >> inquiry cdb: 12 01 00 00 fc 00 >> invalid VPD response; probably a STANDARD INQUIRY response >> First 32 bytes of bad response >> 00 00 80 06 02 1f 00 00 00 00 00 00 00 00 00 00 00 ................ >> 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >> fetching VPD page failed >> >> - So this patch is motivated by the case when a USB mass-storage device >> is presented as a SCSI disk, directly on the (virtual) hardware level, >> and it provides a bogus 0x00 VPD page. > > I'm no longer certain that edk2's ScsiDiskDxe is the right place to fix > this problem. It seems that the usb-storage driver in Linux could be the > culprit -- it should be watching for INQUIRY commands with EVPD set, and > reject them before forwarding them to the device. That is the method > that complies with both SPC-4 and the UFI command spec. (And, edk2 > handles this case correctly, already.) Please see > <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c17>. > > So, for the time being, I'm withdrawing this patch. Sigh, scratch that please (and please excuse the noise). It turns out that the USB device in question has: - one configuration - for that configuration, one interface - for that interface, a /bInterfaceSubClass/ value of 6 (implying "SCSI transparent command set") This implies that the USB device in question is supposed to parse and handle *all* valid SCSI commands that are sent to it. Meaning, if the device gets an INQUIRY+EVPD command from ScsiDiskDxe -- transparently forwarded by QEMU and the host Linux kernel --, then the device is responsible for rejecting the command with CHECK CONDITION (if the device doesn't support INQUIRY+EVPD). Instead of this, however, the device silently returns a standard INQUIRY response (rather than a CHECK CONDITION, or even the requested VPD page). Hence, this device is *genuinely* broken. And, I would like to get this patch into ScsiDiskDxe after all. Thank you Laszlo _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On 05/05/16 09:07, Tian, Feng wrote: > Laszlo, > > Thanks for your details. It looks make sense. > > Reviewed-by: Feng Tian <feng.tian@intel.com> Many thanks! Commit ce1647fc608e. Cheers! Laszlo > > Thanks > Feng > > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek > Sent: Thursday, May 5, 2016 12:47 AM > To: Tian, Feng <feng.tian@intel.com>; edk2-devel-01 <edk2-devel@ml01.01.org> > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Paolo Bonzini <pbonzini@redhat.com>; Zeng, Star <star.zeng@intel.com> > Subject: Re: [edk2] [PATCH] MdeModulePkg: ScsiDiskDxe: cope with broken "Supported VPD Pages" VPD page > > On 05/04/16 18:03, Laszlo Ersek wrote: >> On 05/04/16 13:11, Laszlo Ersek wrote: >>> On 05/04/16 02:58, Tian, Feng wrote: >>>> Laszlo, >>>> >>>> Could you explain more? >>>> >>>> Usb Flash drive is managed by UsbMassStorage driver, which is irrelevant with UefiScsiLib lib or ScsiDisk driver. >>> >>> Yeah, sorry about missing that part of the context. >>> >>> So, the environment is the following: >>> >>> - The USB flash drive with VendorId/ProductId 0x1516/0x6221 is >>> plugged into a Linux host. >>> >>> - The usb-storage driver in Linux presents the USB flash drive to the >>> rest of the system as a SCSI disk. The device node that is generated >>> for it looks like /dev/sd*, for example, /dev/sdi. >>> >>> - This device node can be used like any other SCSI device. >>> >>> - Using the virtio-scsi HBA, QEMU can provide a virtual machine with >>> various types of virtual SCSI devices. One of those is "scsi-block", >>> which is also known as "SCSI passthrough". It means that most of the >>> SCSI commands are forwarded transparently from guest device to >>> physical host device (/dev/sdi, for example), using the SG_IO ioctl() >>> call. Only a few SCSI commands are captured and emulated by QEMU. >>> >>> This feature is useful e.g. for burning physical optical media, or >>> writing physical tapes, from a virtual machine. >>> >>> - In this scenario, the USB stick that is plugged in the host is >>> exposed to the guest with this feature. So, when the >>> ScsiDiskInquiryDevice() function runs in the guest (as part of OVMF), >>> the INQUIRY command (asking for the Supported VPD Pages" VPD page) is >>> forwarded to the USB stick on the host. And that device returns garbage. >>> >>> - It is important to note that the "garbage" is not inserted by >>> either the host kernel's usb-storage driver, or by QEMU. Even without >>> virtualization, the host kernel consciously avoids asking SCSI disks >>> that are actually backed by USB mass-storage devices for VPD pages, >>> because the kernel developers have realized that most (if not all) of >>> USB mass-storage devices fail to respond correctly. Please see this >>> host kernel commit: >>> >>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit >>> /?id=09b6b51b0b6c >>> >>> - In <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c14>, I >>> demonstrate what happens when such an INQUIRY is sent manually to the >>> SCSI disk that is backed by the USB flash drive. For a genuine SCSI >>> disk, the command completes: >>> >>> # sg_vpd -v /dev/sda >>> Supported VPD pages VPD page: >>> inquiry cdb: 12 01 00 00 fc 00 >>> [PQual=0 Peripheral device type: disk] >>> Supported VPD pages [sv] >>> Unit serial number [sn] >>> Device identification [di] >>> ATA information (SAT) [ai] >>> Block limits (SBC) [bl] >>> Block device characteristics (SBC) [bdc] >>> Logical block provisioning (SBC) [lbpv] >>> >>> Buth the USB flash drive responds with garbage: >>> >>> # sg_vpd -v /dev/sdi >>> Supported VPD pages VPD page: >>> inquiry cdb: 12 01 00 00 fc 00 >>> invalid VPD response; probably a STANDARD INQUIRY response First 32 >>> bytes of bad response >>> 00 00 80 06 02 1f 00 00 00 00 00 00 00 00 00 00 00 ................ >>> 10 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................ >>> fetching VPD page failed >>> >>> - So this patch is motivated by the case when a USB mass-storage >>> device is presented as a SCSI disk, directly on the (virtual) >>> hardware level, and it provides a bogus 0x00 VPD page. >> >> I'm no longer certain that edk2's ScsiDiskDxe is the right place to >> fix this problem. It seems that the usb-storage driver in Linux could >> be the culprit -- it should be watching for INQUIRY commands with EVPD >> set, and reject them before forwarding them to the device. That is the >> method that complies with both SPC-4 and the UFI command spec. (And, >> edk2 handles this case correctly, already.) Please see >> <https://bugzilla.redhat.com/show_bug.cgi?id=1330955#c17>. >> >> So, for the time being, I'm withdrawing this patch. > > Sigh, scratch that please (and please excuse the noise). > > It turns out that the USB device in question has: > - one configuration > - for that configuration, one interface > - for that interface, a /bInterfaceSubClass/ value of 6 > (implying "SCSI transparent command set") > > This implies that the USB device in question is supposed to parse and handle *all* valid SCSI commands that are sent to it. > > Meaning, if the device gets an INQUIRY+EVPD command from ScsiDiskDxe -- transparently forwarded by QEMU and the host Linux kernel --, then the device is responsible for rejecting the command with CHECK CONDITION (if the device doesn't support INQUIRY+EVPD). > > Instead of this, however, the device silently returns a standard INQUIRY response (rather than a CHECK CONDITION, or even the requested VPD page). > > Hence, this device is *genuinely* broken. And, I would like to get this patch into ScsiDiskDxe after all. > > Thank you > Laszlo > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
diff --git a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c index dfa5fa32e635..1b75d55231a6 100644 --- a/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c +++ b/MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c @@ -1493,7 +1493,44 @@ ScsiDiskInquiryDevice ( if (!EFI_ERROR (Status)) { PageLength = (SupportedVpdPages->PageLength2 << 8) | SupportedVpdPages->PageLength1; + + // + // Sanity checks for coping with broken devices + // + if (PageLength > sizeof SupportedVpdPages->SupportedVpdPageList) { + DEBUG ((EFI_D_WARN, + "%a: invalid PageLength (%u) in Supported VPD Pages page\n", + __FUNCTION__, (UINT32)PageLength)); + PageLength = 0; + } + + if ((PageLength > 0) && + (SupportedVpdPages->SupportedVpdPageList[0] != + EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)) { + DEBUG ((EFI_D_WARN, + "%a: Supported VPD Pages page doesn't start with code 0x%02x\n", + __FUNCTION__, EFI_SCSI_PAGE_CODE_SUPPORTED_VPD)); + PageLength = 0; + } + + // + // Locate the code for the Block Limits VPD page + // for (Index = 0; Index < PageLength; Index++) { + // + // Sanity check + // + if ((Index > 0) && + (SupportedVpdPages->SupportedVpdPageList[Index] <= + SupportedVpdPages->SupportedVpdPageList[Index - 1])) { + DEBUG ((EFI_D_WARN, + "%a: non-ascending code in Supported VPD Pages page @ %u\n", + __FUNCTION__, Index)); + Index = 0; + PageLength = 0; + break; + } + if (SupportedVpdPages->SupportedVpdPageList[Index] == EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD) { break; }
The USB flash drive with Vendor ID 0x1516 (CompUSA) and Product ID 0x6221 returns a broken "Supported VPD Pages" VPD page. In particular, the PageLength field has the invalid value 0x0602 (decimal 1538). This prevents the loop from terminating that scans for the Block Limits VPD page code in ScsiDiskInquiryDevice(): for (Index = 0; Index < PageLength; Index++) { because the Index variable has type UINT8, and it wraps from 255 to 0, without ever reaching PageLength (1538), and because EFI_SCSI_PAGE_CODE_BLOCK_LIMITS_VPD does not occur at offsets 0 through 255. * The fix is not to change the type of Index to UINT16 or a wider type. Namely, section 7.8.14 Supported VPD Pages VPD page in the "SCSI Primary Commands - 4" (SPC-4) specification names the following requirement: The supported VPD page list shall contain a list of all VPD page codes (see 7.8) implemented by the logical unit in ascending order beginning with page code 00h. Since page codes are 8-bit unsigned quantities, it follows that the maximum size for the Supported VPD Pages VPD page is 0x100 bytes, in which every possible page code (0x00 through 0xFF) will be found, before the UINT8 offset wraps around. (EFI_SCSI_SUPPORTED_VPD_PAGES_VPD_PAGE.SupportedVpdPageList is correctly sized as well, in "MdePkg/Include/IndustryStandard/Scsi.h".) * Instead, add sanity checks that enforce the above requirement. If the device breaks the spec, simply fall back to the "Block Limits page absent" case. Cc: Feng Tian <feng.tian@intel.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Star Zeng <star.zeng@intel.com> Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1330955 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- MdeModulePkg/Bus/Scsi/ScsiDiskDxe/ScsiDisk.c | 37 ++++++++++++++++++++ 1 file changed, 37 insertions(+) -- 1.8.3.1 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel