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-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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
