| Message ID | 20231006125445.122380-1-gmazyland@gmail.com |
|---|---|
| Headers | show |
| Series | usb-storage,uas,scsi: Support OPAL commands on USB attached devices. | expand |
On Fri, Oct 06, 2023 at 02:54:45PM +0200, Milan Broz wrote: > Realtek 9210 family (NVME to USB bridge) adapters always set > the write-protected bit for the whole drive if an OPAL locking range > is defined (even if the OPAL locking range just covers part of the disk). > > The only way to recover is PSID reset and physical reconnection of the device. > > This looks like a wrong implementation of OPAL standard (and I will try > to report it to Realtek as it happens for all firmware versions I have), > but for now, these adapters are unusable for OPAL. > > Signed-off-by: Milan Broz <gmazyland@gmail.com> > --- > drivers/usb/storage/unusual_devs.h | 11 +++++++++++ > drivers/usb/storage/unusual_uas.h | 11 +++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/drivers/usb/storage/unusual_devs.h b/drivers/usb/storage/unusual_devs.h > index 20dcbccb290b..b7c0df180e5d 100644 > --- a/drivers/usb/storage/unusual_devs.h > +++ b/drivers/usb/storage/unusual_devs.h > @@ -1476,6 +1476,17 @@ UNUSUAL_DEV( 0x0bc2, 0x3332, 0x0000, 0x9999, > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_NO_WP_DETECT ), > > +/* > + * Realtek 9210 family set global write-protection flag > + * for any OPAL locking range making device unusable > + * Reported-by: Milan Broz <gmazyland@gmail.com> > + */ > +UNUSUAL_DEV( 0x0bda, 0x9210, 0x0000, 0xffff, > + "Realtek", > + "", Doesn't Realtek have some sort of product name you can put here? > + USB_SC_DEVICE, USB_PR_DEVICE, NULL, > + US_FL_IGNORE_OPAL), > + > UNUSUAL_DEV( 0x0d49, 0x7310, 0x0000, 0x9999, > "Maxtor", > "USB to SATA", > diff --git a/drivers/usb/storage/unusual_uas.h b/drivers/usb/storage/unusual_uas.h > index 1f8c9b16a0fb..71ab824bfb32 100644 > --- a/drivers/usb/storage/unusual_uas.h > +++ b/drivers/usb/storage/unusual_uas.h > @@ -185,3 +185,14 @@ UNUSUAL_DEV(0x4971, 0x8024, 0x0000, 0x9999, > "External HDD", > USB_SC_DEVICE, USB_PR_DEVICE, NULL, > US_FL_ALWAYS_SYNC), > + > +/* > + * Realtek 9210 family set global write-protection flag > + * for any OPAL locking range making device unusable > + * Reported-by: Milan Broz <gmazyland@gmail.com> > + */ > +UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0xffff, > + "Realtek", > + "", > + USB_SC_DEVICE, USB_PR_DEVICE, NULL, > + US_FL_IGNORE_OPAL), This entry is not in the right position. The file is supposed to be sorted by vendor ID, then product ID. Alan Stern
On 10/6/23 19:16, Alan Stern wrote: > On Fri, Oct 06, 2023 at 02:54:40PM +0200, Milan Broz wrote: >> This patch removes macro that was used only >> by commit that was reverted in >> commit ab4b71644a26d1ab92b987b2fd30e17c25e89f85 >> USB: storage: fix Huawei mode switching regression > > The standard format for referring to commits in patch descriptions is > like this: > > commit ab4b71644a26 ("USB: storage: fix Huawei mode switching regression") Sure, I just forgot to put it here. Thanks for review for this usb part! I'll send a new version with fixed issues and your review line. Milan > > That is, the commit hash is abbreviated to its first 12 hex digits and > is followed by the commit title enclosed in parentheses and quotation > marks. > > Apart from that minor issue, > > Reviewed-by: Alan Stern <stern@rowland.harvard.edu> > > Alan Stern > >> >> Signed-off-by: Milan Broz <gmazyland@gmail.com> >> --- >> drivers/usb/storage/usb.c | 12 ------------ >> drivers/usb/storage/usual-tables.c | 15 --------------- >> 2 files changed, 27 deletions(-) >> >> diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c >> index 7b36a3334fb3..bb1fbeddc5aa 100644 >> --- a/drivers/usb/storage/usb.c >> +++ b/drivers/usb/storage/usb.c >> @@ -110,17 +110,6 @@ MODULE_PARM_DESC(quirks, "supplemental list of device IDs and their quirks"); >> .useTransport = use_transport, \ >> } >> >> -#define UNUSUAL_VENDOR_INTF(idVendor, cl, sc, pr, \ >> - vendor_name, product_name, use_protocol, use_transport, \ >> - init_function, Flags) \ >> -{ \ >> - .vendorName = vendor_name, \ >> - .productName = product_name, \ >> - .useProtocol = use_protocol, \ >> - .useTransport = use_transport, \ >> - .initFunction = init_function, \ >> -} >> - >> static const struct us_unusual_dev us_unusual_dev_list[] = { >> # include "unusual_devs.h" >> { } /* Terminating entry */ >> @@ -132,7 +121,6 @@ static const struct us_unusual_dev for_dynamic_ids = >> #undef UNUSUAL_DEV >> #undef COMPLIANT_DEV >> #undef USUAL_DEV >> -#undef UNUSUAL_VENDOR_INTF >> >> #ifdef CONFIG_LOCKDEP >> >> diff --git a/drivers/usb/storage/usual-tables.c b/drivers/usb/storage/usual-tables.c >> index 529512827d8f..b3c3ea04c11c 100644 >> --- a/drivers/usb/storage/usual-tables.c >> +++ b/drivers/usb/storage/usual-tables.c >> @@ -26,20 +26,6 @@ >> #define USUAL_DEV(useProto, useTrans) \ >> { USB_INTERFACE_INFO(USB_CLASS_MASS_STORAGE, useProto, useTrans) } >> >> -/* Define the device is matched with Vendor ID and interface descriptors */ >> -#define UNUSUAL_VENDOR_INTF(id_vendor, cl, sc, pr, \ >> - vendorName, productName, useProtocol, useTransport, \ >> - initFunction, flags) \ >> -{ \ >> - .match_flags = USB_DEVICE_ID_MATCH_INT_INFO \ >> - | USB_DEVICE_ID_MATCH_VENDOR, \ >> - .idVendor = (id_vendor), \ >> - .bInterfaceClass = (cl), \ >> - .bInterfaceSubClass = (sc), \ >> - .bInterfaceProtocol = (pr), \ >> - .driver_info = (flags) \ >> -} >> - >> const struct usb_device_id usb_storage_usb_ids[] = { >> # include "unusual_devs.h" >> { } /* Terminating entry */ >> @@ -49,7 +35,6 @@ MODULE_DEVICE_TABLE(usb, usb_storage_usb_ids); >> #undef UNUSUAL_DEV >> #undef COMPLIANT_DEV >> #undef USUAL_DEV >> -#undef UNUSUAL_VENDOR_INTF >> >> /* >> * The table of devices to ignore >> -- >> 2.42.0 >>
On 10/6/23 20:57, Alan Stern wrote: > On Fri, Oct 06, 2023 at 02:54:45PM +0200, Milan Broz wrote: >> Realtek 9210 family (NVME to USB bridge) adapters always set >> the write-protected bit for the whole drive if an OPAL locking range >> is defined (even if the OPAL locking range just covers part of the disk). ... >> +/* >> + * Realtek 9210 family set global write-protection flag >> + * for any OPAL locking range making device unusable >> + * Reported-by: Milan Broz <gmazyland@gmail.com> >> + */ >> +UNUSUAL_DEV( 0x0bda, 0x9210, 0x0000, 0xffff, >> + "Realtek", >> + "", > > Doesn't Realtek have some sort of product name you can put here? These adapters comes under many names, the only common thing is that it uses Realtek controller... "USB to NVMe/SATA bridge" could work though, I guess. ... >> + >> +/* >> + * Realtek 9210 family set global write-protection flag >> + * for any OPAL locking range making device unusable >> + * Reported-by: Milan Broz <gmazyland@gmail.com> >> + */ >> +UNUSUAL_DEV(0x0bda, 0x9210, 0x0000, 0xffff, >> + "Realtek", >> + "", >> + USB_SC_DEVICE, USB_PR_DEVICE, NULL, >> + US_FL_IGNORE_OPAL), > > This entry is not in the right position. The file is supposed to be > sorted by vendor ID, then product ID. Yes, despite I checked it at least three times and I did not spot it :-))) Thanks, Milan
This patchset adds support for OPAL commands (self-encrypted drives)
through USB-attached storage (usb-storage and UAS drivers).
The related SCSI change was sent in a separate patch
https://lore.kernel.org/linux-scsi/20231016070211.39502-1-gmazyland@gmail.com/
The first part (64-bit quirks) is generic and will be needed later anyway
once new flags appear.
1) Patches 1-5 only add support for 64-bit quirks for USB storage
(unfortunately, USB device info can be 32-bit on 32-bit platforms,
and we are out of space for flags now).
2) Patches 6-7 enable OPAL commands on USB devices and adds
IGNORE_OPAL quirk. The last patch uses the flag for Realtek 9210
devices that do not behave correctly.
More info
1) 64bit USB storage quirk flags
The quirks are transferred through the device info value, which
is unsigned long (and as a part of USB infrastructure, it cannot
be changed).
After discussion on the USB list, I used high bit as an indicator
that the values need to be translated/unpacked to 64bit
(while lower values are used directly).
This is implemented through a host-compiled program that
generates device tables and uses a translation function.
As both usb-storage and UAS drivers share a lot of headers and
definitions, we need to generate separate files for usb-storage
and UAS. Note that due to the linking of both UAS and mass-storage
together, it must use separate names for translation tables.
(I also tried to use a statically generated array for flags,
but this increased the size of drivers significantly and
the code was quite ugly...)
2) Support for OPAL on USB attached storage.
The main support for OPAL on USB-attached storage is
straightforward (it depends on ATA-12 pass-thru support
for security commands).
Patch 6 enables the SCSI security flag for USB mass storage
and UAS device by default.
During device detection, the USB driver (mass-storage, UAS) enables
the security driver flag to allow SED OPAL code to run OPAL discovery
command for the device. If it receives a correct reply, OPAL is enabled
for the device. If not (or if SCSI command is rejected), OPAL
remains disabled.
Enabling OPAL support may uncover many issues, as OPAL-locked devices
often tend to generate errors on the locked range.
Anyway, cryptsetup will soon support OPAL devices, and I think support
for USB devices is a nice feature that enables users to unlock drives
even if they are attached through USB adapters.
There are also bugs in firmware implementations, so I added a quirk
flag that can disable security commands for particular devices.
The last patch uses this quirk for Realtek 9210, which seems to support
OPAL commands, but after configuring OPAL locking range, it also sets
the write-protected flag for the whole device.
This is perhaps a bug in firmware (all versions I tried), and I will
report that later to Realtek.
Milan Broz (7):
usb-storage: remove UNUSUAL_VENDOR_INTF macro
usb-storage,uas: make internal quirks flags 64bit
usb-storage: use fflags index only in usb-storage driver
usb-storage,uas: use host helper to generate driver info
usb-storage,uas: do not convert device_info for 64-bit platforms
usb-storage,uas: enable security commands for USB-attached storage
usb-storage,uas: disable security commands (OPAL) for RT9210 chip
family
.../admin-guide/kernel-parameters.txt | 2 +
drivers/usb/storage/Makefile | 28 +++
drivers/usb/storage/alauda.c | 2 +-
drivers/usb/storage/cypress_atacb.c | 2 +-
drivers/usb/storage/datafab.c | 2 +-
drivers/usb/storage/ene_ub6250.c | 2 +-
drivers/usb/storage/freecom.c | 2 +-
drivers/usb/storage/isd200.c | 2 +-
drivers/usb/storage/jumpshot.c | 2 +-
drivers/usb/storage/karma.c | 2 +-
drivers/usb/storage/mkflags.c | 235 ++++++++++++++++++
drivers/usb/storage/onetouch.c | 2 +-
drivers/usb/storage/realtek_cr.c | 2 +-
drivers/usb/storage/scsiglue.c | 4 +
drivers/usb/storage/sddr09.c | 2 +-
drivers/usb/storage/sddr55.c | 2 +-
drivers/usb/storage/shuttle_usbat.c | 2 +-
drivers/usb/storage/uas-detect.h | 6 +-
drivers/usb/storage/uas.c | 29 +--
drivers/usb/storage/unusual_devs.h | 11 +
drivers/usb/storage/unusual_uas.h | 11 +
drivers/usb/storage/usb-ids.h | 37 +++
drivers/usb/storage/usb.c | 44 ++--
drivers/usb/storage/usb.h | 7 +-
drivers/usb/storage/usual-tables.c | 38 +--
include/linux/usb_usual.h | 2 +
26 files changed, 385 insertions(+), 95 deletions(-)
create mode 100644 drivers/usb/storage/mkflags.c
create mode 100644 drivers/usb/storage/usb-ids.h
On Mon, Oct 16, 2023 at 09:25:57AM +0200, Milan Broz wrote: > This patchset adds support for OPAL commands (self-encrypted drives) > through USB-attached storage (usb-storage and UAS drivers). This is version 2 of the proposed patch set, but you didn't include the version number in the email Subject: lines and you didn't include the summary of differences from v1 below the "---" lines of the various patches. Patches 5, 6, and 7 look okay. You can add my Reviewed-by: to each of them. I've got some additional comments on patch 4 (in a separate email). Alan Stern
On 10/16/23 19:33, Alan Stern wrote: > On Mon, Oct 16, 2023 at 09:25:57AM +0200, Milan Broz wrote: >> This patchset adds support for OPAL commands (self-encrypted drives) >> through USB-attached storage (usb-storage and UAS drivers). > > This is version 2 of the proposed patch set, but you didn't include the > version number in the email Subject: lines and you didn't include the > summary of differences from v1 below the "---" lines of the various > patches. Hi, well, the first patchset was RFC, so I sent is as "the first real version". Perhaps not the correct way, sorry for that. Anyway, if you see the discussion about OPAL change on SCSI list, another solution (inside USB storage driver) is needed. So, please ignore patch 6/7, these will be needed, but I have to rewrite SCSI logic to USB glue/UAS driver. But for the generic 64-bit flags (patch 1-5), if you see this useful, please review it. Common requirement is that kernel patch need an user for merge (and my flag is currently no going to be used without rewrite). But that time will come one day, and if I can save people time to reinvent the 64-bit quirks logic, it would be nice to merge it. Thanks, Milan > > Patches 5, 6, and 7 look okay. You can add my Reviewed-by: to each of > them. > > I've got some additional comments on patch 4 (in a separate email). > > Alan Stern
This patchset adds support for OPAL commands (self-encrypted drives) through USB-attached storage (usb-storage and UAS drivers). 1) Patches 1-4 only add support for 64-bit quirks for USB storage (unfortunately, USB device info can be 32-bit on 32-bit platforms, and we are out of space for flags now). 2) Patches 5-6 enable OPAL commands on USB device and also adds an ATA-12 pass-thru wrapper to support OPAL even on devices that do not support SCSI SECURITY IN/OUT commands. ATA-12 is already used by sedutils directly; this patch makes internal kernel OPAL ioctl work with ATA-12 too. As patch 6 introduced a new USB quirk that overflows 32-bit, I posted all patches together - but logically, these solve two separate issues. More info 1) 64bit USB storage quirk flags The quirks are transferred through the device info value, which is unsigned long (and as a part of USB infrastructure, it cannot be changed). After discussion on USB list, I used high bit as an indicator that the values need to be translated/unpacked to 64bit (while lower values are used directly). This is implemented through a host-compiled program that generates device tables and translation function. As both usb-storage and UAS drivers share a lot of headers and definitions, we need to generate separate files for usb-storage, UAS and flags translation function. (I also tried to use a statically generated array for flags, but this increased the size of drivers significantly, and the code was quite ugly...) 2) Support for OPAL on USB attached storage. The main support for OPAL on USB-attached storage is straightforward. The patch 6 - enables SCSI security flag for USB mass storage and UAS device by default. - adds an optional wrapper to the SCSI layer for the ATA-12 pass-thru command as an alternative if SECURITY IN/OUT is unavailable. During device detection, these steps are then done: 1) USB driver (mass-storage, UAS) enables security driver flag by default if not disabled by quirk 2) SCSI device enumerates SECURITY IN/OUT support. If detected, SECURITY ON/OUT wrapper is used (as in the current code). If not, the new ATA12 pass-thru wrapper is used instead. 3) SED OPAL code tries OPAL discovery command for the device. If it receives correct reply, OPAL is enabled for the device. Enabling support may uncover many issues, as OPAL-locked devices often tend to generate errors on the locked range. Anyway, cryptsetup will soon support OPAL devices, and I think support for USB devices is a nice feature that enables users to unlock drives even if they are attached through USB adapters. But also, there are bugs in firmware, so I added a quirk flag that can disable security commands for particular devices. The last patch uses this quirk for Realtek 9210, which seems to support OPAL commands, but after configuring OPAL locking range, it also sets the write-protected flag for the whole device. This is perhaps a bug in firmware (all versions I tried), and I will report that later to Realtek. Milan Broz (6): usb-storage: remove UNUSUAL_VENDOR_INTF macro usb-storage: make internal quirks flags 64bit usb-storage: use fflags index only in usb-storage driver usb-storage,uas: use host helper to generate driver info usb-storage,uas,scsi: allow to pass through security commands (OPAL) usb-storage,uas: Disable security commands (OPAL) for RT9210 chip family drivers/scsi/sd.c | 33 ++++- drivers/usb/storage/Makefile | 25 ++++ drivers/usb/storage/alauda.c | 2 +- drivers/usb/storage/cypress_atacb.c | 2 +- drivers/usb/storage/datafab.c | 2 +- drivers/usb/storage/ene_ub6250.c | 2 +- drivers/usb/storage/freecom.c | 2 +- drivers/usb/storage/isd200.c | 2 +- drivers/usb/storage/jumpshot.c | 2 +- drivers/usb/storage/karma.c | 2 +- drivers/usb/storage/mkflags.c | 212 ++++++++++++++++++++++++++++ drivers/usb/storage/onetouch.c | 2 +- drivers/usb/storage/realtek_cr.c | 2 +- drivers/usb/storage/scsiglue.c | 4 + drivers/usb/storage/sddr09.c | 2 +- drivers/usb/storage/sddr55.c | 2 +- drivers/usb/storage/shuttle_usbat.c | 2 +- drivers/usb/storage/uas-detect.h | 4 +- drivers/usb/storage/uas.c | 26 ++-- drivers/usb/storage/unusual_devs.h | 11 ++ drivers/usb/storage/unusual_uas.h | 11 ++ drivers/usb/storage/usb.c | 42 +++--- drivers/usb/storage/usb.h | 7 +- drivers/usb/storage/usual-tables.c | 38 +---- include/linux/usb_usual.h | 2 + 25 files changed, 346 insertions(+), 95 deletions(-) create mode 100644 drivers/usb/storage/mkflags.c