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-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Wednesday, May 4, 2016 12:30 AM > To: edk2-devel-01 <edk2-de...@ml01.01.org> > Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini <pbonz...@redhat.com>; > Tian, Feng <feng.t...@intel.com>; Zeng, Star <star.z...@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.t...@intel.com> > Cc: Paolo Bonzini <pbonz...@redhat.com> > Cc: Ruiyu Ni <ruiyu...@intel.com> > Cc: Star Zeng <star.z...@intel.com> > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1330955 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <ler...@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
