On Sun, Aug 27, 2023 at 06:45:55PM +0200, Milan Broz wrote: > On 8/27/23 17:50, Alan Stern wrote: > > On Sun, Aug 27, 2023 at 11:32:05AM +0200, Milan Broz wrote: > > > Hello, > > > > > > I tried to extend USB storage for the passthrough of Opal > > > security commands, > > > > What sort of changes are needed? Where is this passthrough mechanism > > documented? > > We are currently adding support for optional OPAL hw encryption to > cryptsetup/LUKS2 (that uses kernel OPAL ioctl interface) and I tried > to make USB adapters to work with it too. > > I'll send RFC patchset (it is quite simple) where I explain it in detail. > The patch for USB storage is actually one liner, the rest is in SCSI driver :) > > Basically, USB adapters (not supporting UAS) cannot work as > required SCSI SECURITY IN/OUT SCSI commands do not work here. > > But we can use ATA12 pass-thru (as used with original sedutils > and some other tools we used in research; it is a documented feature). > It works once ATA12 wrapper is added to block layer and USB storage enables > the "security_supported" bit. > > > > > > and some adapters are clearly "not perfect". > > > > Which ones? > > Namely Realtek 9210 family (NVME to USB bridge). Everything OPAL related > works, but the adapter always set write-protected bit for the whole > drive (even if OPAL locking range is just covering part of the disk). > > I spent quite a lot time trying new firmware versions - this issue is > still there. It sounds like the sort of thing that should be reported as a bug to Realtek. I can't imagine their customers would be very happy about this behavior. > On the other side, many other USB to SATA bridges works nicely. > I think this is the exact situation where we should set a new quirks flag > to disable it. (The nasty thing is that for unbricking it you need PSID reset > - PSID is a number written on the label of the drive - followed by physical > disconnect for recovery.) > > > Anyway, I intentionally sent this 32bit flags question separately as it > is actually a generic issue - we are just out of flag space now... > > Even if the patches mentioned above are rejected, someone will need > a new flag for something else later. Certainly. We knew this was bound to come up eventually. > > > I would need to introduce a new quirks flag to turn it off. > > > > > > Seems that we are already out of quirks flags on 32bit > > > for usb storage - in usb_usual.h the last entry in mainline is > > > US_FLAG(SENSE_AFTER_SYNC, 0x80000000) > > > > > > Adding a new flag will work for 64-bit systems but not > > > for platforms with 32-bit unsigned long like i686. > > > > > > How do we allow new flag definitions? > > > > > > Struct us_data fflags can be made 64bit (defined in > > > drivers/usb/storage/usb.h), but the major problem is that these > > > are transferred through the generic driver_info field > > > defined in linux/mod_devicetable.h as unsigned long). > > > Making this 64bit is IMO an extensive API change (if even possible). > > > I guess this is not the way to go. > > > > > > Could USB maintainers please help to advise what is the correct > > > solution? I am not familiar with the USB driver model here > > > and I see no easy way how it can be solved by a trivial static > > > allocation inside the USB storage driver. > > > > > > Someone will need a new quirks flag in the future anyway... :) > > > > I can think of only one way to accomplish this on 32-bit systems: Change > > the driver_info field from a bit array to an index into a static table > > of 64-bit flags values. Each unusual_devs structure would have its own > > entry in this table. As far as I can tell, the other unusual_*.h tables > > could retain their current driver_info interpretations, since no new > > quirk bits are likely to be relevant to them. > > > > Making this change would be an awkward nuisance, but it should be > > doable. > > Hm, yes, thanks for the idea,that is a possible solution. > It will need to modify all unusual macros, though. Just I am not sure I want > to spent time patching all the drivers as I have not way how to test it. I don't think it will be necessary to change all those macros, just the ones in usual_tables.c. And to create the new table containing the actual flag values, of course. There will also have to be a new argument to usb_stor_probe1() specifying whether the id->driver_info field is standard (i.e., it contains the flags directly) or is one of the new indirect index values. And you'll have to figure out a comparable change to the dynamic device ID table mechanism. (If you want to be really fancy about it, you could design things in such a way that the indirect flags approach is used only on 32-bit systems. 64-bit systems can put the new flag bits directly into the driver_info field. However, it's probably best not to worry about this initially.) Alan Stern
