| Message ID | 20231016070211.39502-1-gmazyland@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: use ATA-12 pass-thru for OPAL as fallback | expand |
On Mon, Oct 16, 2023 at 09:02:11AM +0200, Milan Broz wrote: > All common USB/SATA or USB/NVMe adapters I tested need this patch. > > In short, these steps are run for OPAL support check: > 1) Storage driver enables security driver flag (security_supported). > USB-attached storage drivers will enable it in a separate patchset. > SCSI and NNVMe drivers do it already. If the flag is not enabled, > no following steps are run, and OPAL remains disabled. > 2) SCSI device enumerates SECURITY IN/OUT command support. If detected, > SECURITY ON/OUT wrapper is used (as in the current code). > If not, new ATA-12 pass-thru wrapper is used instead. > 3) SED OPAL code tries OPAL discovery command for the device. > If it receives a correct reply, OPAL is enabled for the device. > If SCSI SECURITY or ATA-12 command with discovery command is rejected, > OPAL remains disabled. > > Note, USB attached storage needs an additional patchset sent separately > as requested by USB driver maintainers (it contains required changes > related to USB quirk processing). This just feels wrong. These adapters are broken if they can't translated, and we should not put ATA command submission into sd.c. > + cdb[0] = ATA_12; > + cdb[1] = (send ? 5 /* ATA_PROTOCOL_PIO_DATA_IN */ : 4 /* ATA_PROTOCOL_PIO_DATA_OUT */) << 1; > + cdb[2] = 2 /* t_length */ | (1 << 2) /* byt_blok */ | ((send ? 0 : 1) << 3) /* t_dir */; > + cdb[3] = secp; > + put_unaligned_le16(len / 512, &cdb[4]); > + put_unaligned_le16(spsp, &cdb[6]); > + cdb[9] = send ? 0x5e /* ATA_CMD_TRUSTED_SND */: 0x5c /* ATA_CMD_TRUSTED_RCV */; Also avoid all these crazy long lines, and please use the actual constants. Using a good old if/else is actually a very good way to structure the code in a somewhat readable way. > + if (sdkp->security) > + sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit); > + else > + sdkp->opal_dev = init_opal_dev(sdkp, &sd_ata12_submit); Messed up indentation here. besides the fact that the statement is fundamentally wrong and you'll start sending ATA command to random devices.
On Mon, Oct 16, 2023 at 09:24:16AM +0200, Milan Broz wrote: > I think it is blocked in USB layer as not running command enumeration, > SCSI SECURITY will be never sent to the adapter through USB. What is blocked? >> besides the fact that the statement is fundamentally wrong and you'll >> start sending ATA command to random devices. > > So what do you suggest? As I said, this exactly happen if you configure it from userspace. If the USB maintainers are fine with it add the translation to usb-storage as that's where we deal with these quirky devices usually.
On 10/16/23 15:27, Christoph Hellwig wrote: > On Mon, Oct 16, 2023 at 02:46:03PM +0200, Milan Broz wrote: >> The problem is that we (for simplicity) decided to use kernel SED-ioctl interface that >> internally wraps OPAL command to SCSI SECURITY command only. It means, that all devices > > No, it doesn't. It uses the properly specified protocol for each > layer. That is NVMe uses NVMe Security Send/Receive, SCSI uses the > SCSI protocol, and libata translats for ATA devices. > >> that can use ATA-12 just cannot work with this kernel interface (unlike userspace which >> can decide which wrapper to use). > > It supports all devices that actually speak ATA perfectly fine, take > a look at ata_scsi_security_inout_xlat. Yes, I have several of them in my test machine. The comment was about (S)ATA connected through USB bridge only. >> >> And IMO it is not correct - if it was designed only for some servers with directly connected >> devices, then it is really not generic OPAL support. It should work for any hw that supports it. > > Let's get off your crack pipe before we continue. It is designed and > implemented to support the security protocols exactly as spec'ed. > > You seem to have found devices that claim to be SCSI, but actually > require ATA passthrough for security. That's no secret cabal to lock > out non-server hardware but just proper protocol design. *grin* I just bought several NVMe to USB adapters that presents NVMe device as SCSI, this is pretty common. (And Thunderbolt adapters - that present NMVe as real NVMe is another story too. But once configured, it is doing it correctly.) But yes, you are right - except the USB hw is here (in huge quantities) and I want to use it. It is quite possible that there is not way to do it clearly - fine, that's why I sent the patch for review. > >> For USB, it actually works quite nice with the patch (ignoring usual bugs in firmware). > > So move it into usb if you can convince the usb maintainers that they > are fine with it. Yep, fair enough. My initial motivation was just understand WTF is going there. Put the support on a proper layer is step #2. >>> Note that nowhere in your patch do you test if you are talking to an ATA device. >> >> Yes, I know. I expected the command to be rejected if not supported. > > Good luck. Cheap storage hardware trips up on unknown commands all the > time. ... And my tests for TCG OPAL commands shows that it can be even worse on this layer :-) (To be fair, recent NVMe devices looks much better. Anyway, yes, I know what you mean.) > >> IMO it is quite similar to discard/TRIM support... > > Where we also don't support weird ATA commands directly from sd > for good reason. ok, I am actually quite happy that I get some response to this patch. Supporting it is a mess, but I still believe we can do it (if fw is not completely bogus). Thanks, Milan
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 83b6a3f3863b..3782556cb461 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -686,6 +686,32 @@ static int sd_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, &exec_args); return ret <= 0 ? ret : -EIO; } + +static int sd_ata12_submit(void *data, u16 spsp, u8 secp, void *buffer, + size_t len, bool send) +{ + struct scsi_disk *sdkp = data; + struct scsi_device *sdev = sdkp->device; + u8 cdb[12] = { 0, }; + const struct scsi_exec_args exec_args = { + .req_flags = BLK_MQ_REQ_PM, + }; + int ret; + + cdb[0] = ATA_12; + cdb[1] = (send ? 5 /* ATA_PROTOCOL_PIO_DATA_IN */ : 4 /* ATA_PROTOCOL_PIO_DATA_OUT */) << 1; + cdb[2] = 2 /* t_length */ | (1 << 2) /* byt_blok */ | ((send ? 0 : 1) << 3) /* t_dir */; + cdb[3] = secp; + put_unaligned_le16(len / 512, &cdb[4]); + put_unaligned_le16(spsp, &cdb[6]); + cdb[9] = send ? 0x5e /* ATA_CMD_TRUSTED_SND */: 0x5c /* ATA_CMD_TRUSTED_RCV */; + + ret = scsi_execute_cmd(sdev, cdb, send ? REQ_OP_DRV_OUT : REQ_OP_DRV_IN, + buffer, len, SD_TIMEOUT, sdkp->max_retries, + &exec_args); + return ret <= 0 ? ret : -EIO; +} + #endif /* CONFIG_BLK_SED_OPAL */ /* @@ -3699,8 +3725,11 @@ static int sd_probe(struct device *dev) goto out; } - if (sdkp->security) { - sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit); + if (sdp->security_supported) { + if (sdkp->security) + sdkp->opal_dev = init_opal_dev(sdkp, &sd_sec_submit); + else + sdkp->opal_dev = init_opal_dev(sdkp, &sd_ata12_submit); if (sdkp->opal_dev) sd_printk(KERN_NOTICE, sdkp, "supports TCG Opal\n"); }
USB attached storage (handled by UAS or usb-storage driver) does not support SCSI SECURITY IN/OUT commands. (USB adapters usually do not support it, and USB drivers also turn off SCSI command enumeration). The original sedutils (used to configure OPAL from userspace) always used the ATA-12 pass-through command. Internal kernel implementation for OPAL interface (sed-opal ioctls) currently supports only SCSI SECURITY IN/OUT command wrapper. This patch adds an optional wrapper to the SCSI layer for the ATA-12 pass-thru command as an alternative if SECURITY IN/OUT is unavailable. The wrapper has the same structure as tools that talk to OPAL drives through ioctl from userspace. All common USB/SATA or USB/NVMe adapters I tested need this patch. In short, these steps are run for OPAL support check: 1) Storage driver enables security driver flag (security_supported). USB-attached storage drivers will enable it in a separate patchset. SCSI and NNVMe drivers do it already. If the flag is not enabled, no following steps are run, and OPAL remains disabled. 2) SCSI device enumerates SECURITY IN/OUT command support. If detected, SECURITY ON/OUT wrapper is used (as in the current code). If not, new ATA-12 pass-thru wrapper is used instead. 3) SED OPAL code tries OPAL discovery command for the device. If it receives a correct reply, OPAL is enabled for the device. If SCSI SECURITY or ATA-12 command with discovery command is rejected, OPAL remains disabled. Note, USB attached storage needs an additional patchset sent separately as requested by USB driver maintainers (it contains required changes related to USB quirk processing). Signed-off-by: Milan Broz <gmazyland@gmail.com> --- drivers/scsi/sd.c | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-)