Re: [PATCH v6 6/9] scsi: ufs: Add inline encryption support to UFS

Eric Biggers
2019-12-20T05:44:46Z

On Wed, Dec 18, 2019 at 06:51:33AM -0800, Satya Tangirala wrote:
> Wire up ufshcd.c with the UFS Crypto API, the block layer inline
> encryption additions and the keyslot manager.
>
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
> drivers/scsi/ufs/ufshcd-crypto.c | 30 ++++++++++++++++++
> drivers/scsi/ufs/ufshcd-crypto.h | 21 +++++++++++++
> drivers/scsi/ufs/ufshcd.c | 54 +++++++++++++++++++++++++++++---
> drivers/scsi/ufs/ufshcd.h | 8 +++++
> 4 files changed, 108 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd-crypto.c b/drivers/scsi/ufs/ufshcd-crypto.c
> index b0aa072d9009..749c325686a7 100644
> --- a/drivers/scsi/ufs/ufshcd-crypto.c
> +++ b/drivers/scsi/ufs/ufshcd-crypto.c
> @@ -352,6 +352,36 @@ void ufshcd_crypto_setup_rq_keyslot_manager(struct ufs_hba *hba,
> }
> EXPORT_SYMBOL_GPL(ufshcd_crypto_setup_rq_keyslot_manager);
>
> +int ufshcd_prepare_lrbp_crypto(struct ufs_hba *hba,
> + struct scsi_cmnd *cmd,
> + struct ufshcd_lrb *lrbp)
> +{
> + struct bio_crypt_ctx *bc;
> +
> + if (!bio_crypt_should_process(cmd->request)) {
> + lrbp->crypto_enable = false;
> + return 0;
> + }
> + bc = cmd->request->bio->bi_crypt_context;
> +
> + if (WARN_ON(!ufshcd_is_crypto_enabled(hba))) {
> + /*
> + * Upper layer asked us to do inline encryption
> + * but that isn't enabled, so we fail this request.
> + */
> + return -EINVAL;
> + }
> + if (!ufshcd_keyslot_valid(hba, bc->bc_keyslot))
> + return -EINVAL;
> +
> + lrbp->crypto_enable = true;
> + lrbp->crypto_key_slot = bc->bc_keyslot;
> + lrbp->data_unit_num = bc->bc_dun[0];
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(ufshcd_prepare_lrbp_crypto);

The UFS driver only uses the first 64 bits of the DUN, but in this version of
the patchset the DUN in the bio_crypt_ctx can be up to the real length of the
algorithm's IV -- which for AES-256-XTS is 128 bits. So if the user were to
specify anything nonzero in bits 64-127, the crypto would be done incorrectly.

(This case isn't encountered with fscrypt. But it's still an issue with the
overall approach.)

So there needs to be a way for drivers to declare the max_dun_size they support,
and prevent them from being used with longer DUNs.

- Eric