linux-kernel - Re: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20231016070531.GA28537@lst.de>
Date:   Mon, 16 Oct 2023 09:05:31 +0200
From:   Christoph Hellwig <hch@....de>
To:     Milan Broz <gmazyland@...il.com>
Cc:     linux-scsi@...r.kernel.org, jejb@...ux.ibm.com,
        martin.petersen@...cle.com, hch@....de,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] scsi: use ATA-12 pass-thru for OPAL as fallback

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.