[PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Po-Wen Kao]

From: Brian Kao <powenkao@google.com>

The UFS driver utilizes block layer crypto fields, such as
rq->crypt_keyslot and rq->crypt_ctx, to configure hardware for inline
encryption. However, the SCSI error handler (EH) reuses the
Protocol Data Unit (PDU) from the original failing request when issuing
EH commands (e.g., TEST UNIT READY, START STOP UNIT).

This can lead to issues if the original request of reused PDU contains
stale cryptographic configurations, which are not applicable for
the simple EH commands. These commands should not involve data
encryption.

This patch fixes this by checking if the command was submitted by the
SCSI error handler. If so, it bypasses the cryptographic setup for
the request, ensuring UTRDs are not inadvertently
configured with potentially incorrect encryption parameters.

Signed-off-by: Brian Kao <powenkao@google.com>
---
 drivers/ufs/core/ufshcd-crypto.h | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/ufs/core/ufshcd-crypto.h b/drivers/ufs/core/ufshcd-crypto.h
index c148a5194378..26a0699c8412 100644
--- a/drivers/ufs/core/ufshcd-crypto.h
+++ b/drivers/ufs/core/ufshcd-crypto.h
@@ -16,7 +16,12 @@
 static inline void ufshcd_prepare_lrbp_crypto(struct request *rq,
 					      struct ufshcd_lrb *lrbp)
 {
-	if (!rq || !rq->crypt_keyslot) {
+	/*
+	 * Do not use the crypto settings if the SCSI error handler has replaced
+	 * the SCSI command
+	 */
+	if (!rq || !rq->crypt_keyslot ||
+	    unlikely(lrbp->cmd->submitter == SUBMITTED_BY_SCSI_ERROR_HANDLER)) {
 		lrbp->crypto_key_slot = -1;
 		return;
 	}
-- 
2.52.0.223.gf5cc29aaa4-goog

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Bart Van Assche]

On 12/7/25 6:52 PM, Po-Wen Kao wrote:
> From: Brian Kao <powenkao@google.com>
> 
> The UFS driver utilizes block layer crypto fields, such as
> rq->crypt_keyslot and rq->crypt_ctx, to configure hardware for inline
> encryption. However, the SCSI error handler (EH) reuses the
> Protocol Data Unit (PDU) from the original failing request when issuing
> EH commands (e.g., TEST UNIT READY, START STOP UNIT).
> 
> This can lead to issues if the original request of reused PDU contains
> stale cryptographic configurations, which are not applicable for
> the simple EH commands. These commands should not involve data
> encryption.
> 
> This patch fixes this by checking if the command was submitted by the
> SCSI error handler. If so, it bypasses the cryptographic setup for
> the request, ensuring UTRDs are not inadvertently
> configured with potentially incorrect encryption parameters.
> 
> Signed-off-by: Brian Kao <powenkao@google.com>
> ---
>   drivers/ufs/core/ufshcd-crypto.h | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ufs/core/ufshcd-crypto.h b/drivers/ufs/core/ufshcd-crypto.h
> index c148a5194378..26a0699c8412 100644
> --- a/drivers/ufs/core/ufshcd-crypto.h
> +++ b/drivers/ufs/core/ufshcd-crypto.h
> @@ -16,7 +16,12 @@
>   static inline void ufshcd_prepare_lrbp_crypto(struct request *rq,
>   					      struct ufshcd_lrb *lrbp)
>   {
> -	if (!rq || !rq->crypt_keyslot) {
> +	/*
> +	 * Do not use the crypto settings if the SCSI error handler has replaced
> +	 * the SCSI command
> +	 */
> +	if (!rq || !rq->crypt_keyslot ||
> +	    unlikely(lrbp->cmd->submitter == SUBMITTED_BY_SCSI_ERROR_HANDLER)) {
>   		lrbp->crypto_key_slot = -1;
>   		return;
>   	}

(+Christoph Hellwig and Eric Biggers)

Cc: stable@vger.kernel.org
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Christoph Hellwig]

On Mon, Dec 08, 2025 at 02:52:21AM +0000, Po-Wen Kao wrote:
> From: Brian Kao <powenkao@google.com>
> 
> The UFS driver utilizes block layer crypto fields, such as
> rq->crypt_keyslot and rq->crypt_ctx, to configure hardware for inline
> encryption. However, the SCSI error handler (EH) reuses the
> Protocol Data Unit (PDU) from the original failing request when issuing
> EH commands (e.g., TEST UNIT READY, START STOP UNIT).
> 
> This can lead to issues if the original request of reused PDU contains
> stale cryptographic configurations, which are not applicable for
> the simple EH commands. These commands should not involve data
> encryption.
> 
> This patch fixes this by checking if the command was submitted by the
> SCSI error handler. If so, it bypasses the cryptographic setup for
> the request, ensuring UTRDs are not inadvertently
> configured with potentially incorrect encryption parameters.

As mentioned last round, why are you even calling into the crypto
code here?  Calling that for a request without a crypt context,
which includes all of them that do not transfer any data makes no
sense to start with.

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Bart Van Assche]

On 12/9/25 10:48 PM, Christoph Hellwig wrote:
> As mentioned last round, why are you even calling into the crypto
> code here?  Calling that for a request without a crypt context,
> which includes all of them that do not transfer any data makes no
> sense to start with.

ufshcd_prepare_lrbp_crypto() only has one caller. Moving the new test
from inside ufshcd_prepare_lrbp_crypto() into its caller should be easy.

Thanks,

Bart.

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Christoph Hellwig]

On Wed, Dec 10, 2025 at 09:44:52AM -0800, Bart Van Assche wrote:
> On 12/9/25 10:48 PM, Christoph Hellwig wrote:
> > As mentioned last round, why are you even calling into the crypto
> > code here?  Calling that for a request without a crypt context,
> > which includes all of them that do not transfer any data makes no
> > sense to start with.
> 
> ufshcd_prepare_lrbp_crypto() only has one caller. Moving the new test
> from inside ufshcd_prepare_lrbp_crypto() into its caller should be easy.

I don't think callers vs calle is the important part here.  It is to
check if any actual data is tranferred instead of special casing EH
commands.

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Bart Van Assche]

On 12/14/25 9:58 PM, Christoph Hellwig wrote:
> On Wed, Dec 10, 2025 at 09:44:52AM -0800, Bart Van Assche wrote:
>> On 12/9/25 10:48 PM, Christoph Hellwig wrote:
>>> As mentioned last round, why are you even calling into the crypto
>>> code here?  Calling that for a request without a crypt context,
>>> which includes all of them that do not transfer any data makes no
>>> sense to start with.
>>
>> ufshcd_prepare_lrbp_crypto() only has one caller. Moving the new test
>> from inside ufshcd_prepare_lrbp_crypto() into its caller should be easy.
> 
> I don't think callers vs calle is the important part here.  It is to
> check if any actual data is tranferred instead of special casing EH
> commands.

Hi Christoph,

Do you agree with the following?

(a) There is code in the SCSI error handler that submits SCSI commands
     with a data buffer. Hence, disabling encryption if and only if the
     data buffer length is zero can't fix the reported problem. From
     scsi_eh_prep_cmnd() in drivers/scsi/scsi_error.c:

		scmd->sdb.length = min_t(unsigned, SCSI_SENSE_BUFFERSIZE,
					 sense_bytes);
		sg_init_one(&ses->sense_sgl, scmd->sense_buffer,
			    scmd->sdb.length);
		scmd->sdb.table.sgl = &ses->sense_sgl;

(b) In general in the Linux kernel it is strongly preferred to fix the
     root cause of a bug rather than to implement a workaround. This is
     preferred because it makes kernel code easier to maintain and
     reduces the chance that new bugs are introduced.

(c) Disabling encryption in the UFS driver if a command has been
     submitted by the SCSI error handler is a workaround. Patch [1] fixes
     the root cause of the problem, namely the SCSI error handler not
     setting the encryption fields in struct request correctly. We prefer
     [1] because UFS devices that support one million IOPS are expected
     to arrive soon (early 2026). Hence the importance of keeping the hot
     path in the UFS driver fast.

Thanks,

Bart.

[1] [PATCH v2 1/1] scsi: core: Fix error handler encryption support
  
(https://lore.kernel.org/linux-scsi/20251203073310.2248956-1-powenkao@google.com/)

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Christoph Hellwig]

On Mon, Dec 15, 2025 at 08:47:03AM -0800, Bart Van Assche wrote:
> > I don't think callers vs calle is the important part here.  It is to
> > check if any actual data is tranferred instead of special casing EH
> > commands.
> 
> Hi Christoph,
> 
> Do you agree with the following?
> 
> (a) There is code in the SCSI error handler that submits SCSI commands
>     with a data buffer. Hence, disabling encryption if and only if the
>     data buffer length is zero can't fix the reported problem. From
>     scsi_eh_prep_cmnd() in drivers/scsi/scsi_error.c:

This still does not actually transfer data to the media, and thus
is not affected by inline encryption.

Re: [PATCH 1/1] scsi: ufs: core: Fix error handler encryption support

[Bart Van Assche]

On 12/16/25 2:44 AM, Christoph Hellwig wrote:
> On Mon, Dec 15, 2025 at 08:47:03AM -0800, Bart Van Assche wrote:
>> (a) There is code in the SCSI error handler that submits SCSI commands
>>      with a data buffer. Hence, disabling encryption if and only if the
>>      data buffer length is zero can't fix the reported problem. From
>>      scsi_eh_prep_cmnd() in drivers/scsi/scsi_error.c:
> 
> This still does not actually transfer data to the media, and thus
> is not affected by inline encryption.

Hi Christoph,

Agreed that neither REQUEST SENSE nor any of the other SCSI commands
submitted by the SCSI error handler access the storage medium. However,
even if Po-Wen's patch would be modified such that it checks the SCSI
CDB to verify whether or not the SCSI command accesses the medium, the
other concerns that I mentioned still apply. So if nobody objects I
propose to repost patch "[PATCH v2 1/1] scsi: core: Fix error handler
encryption support" with v2 bumped to v3.

Thanks,

Bart.