Re: [RFC PATCH v2 5/8] scsi: ufs: UFS crypto API

[Eric Biggers <ebiggers@kernel.org>]

Hi Satya,

On Wed, Jun 05, 2019 at 04:28:34PM -0700, Satya Tangirala wrote:
> Introduce functions to manipulate UFS inline encryption hardware
> in line with the JEDEC UFSHCI v2.1 specification and to work with the
> block keyslot manager.
> 
> Signed-off-by: Satya Tangirala <satyat@google.com>
> ---
>  drivers/scsi/ufs/Kconfig         |  10 +
>  drivers/scsi/ufs/Makefile        |   1 +
>  drivers/scsi/ufs/ufshcd-crypto.c | 438 +++++++++++++++++++++++++++++++
>  drivers/scsi/ufs/ufshcd-crypto.h |  69 +++++
>  4 files changed, 518 insertions(+)
>  create mode 100644 drivers/scsi/ufs/ufshcd-crypto.c
>  create mode 100644 drivers/scsi/ufs/ufshcd-crypto.h
> 

There is a build error after this patch because it adds code that uses the
crypto fields in struct ufs_hba, but those aren't added until the next patch.

It needs to be possible to compile a working kernel after each patch.
Otherwise it breaks bisection.

So, perhaps add the fields in this patch instead.

> +++ b/drivers/scsi/ufs/ufshcd-crypto.c
> @@ -0,0 +1,438 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright 2019 Google LLC
> + */
> +
> +#include <crypto/algapi.h>
> +
> +#include "ufshcd.h"
> +#include "ufshcd-crypto.h"
> +
> +bool ufshcd_hba_is_crypto_supported(struct ufs_hba *hba)
> +{
> +	return hba->crypto_capabilities.reg_val != 0;
> +}
> +
> +bool ufshcd_is_crypto_enabled(struct ufs_hba *hba)
> +{
> +	return hba->caps & UFSHCD_CAP_CRYPTO;
> +}
> +
> +static bool ufshcd_cap_idx_valid(struct ufs_hba *hba, unsigned int cap_idx)
> +{
> +	return cap_idx < hba->crypto_capabilities.num_crypto_cap;
> +}
> +
> +#define NUM_KEYSLOTS(hba) (hba->crypto_capabilities.config_count + 1)
> +
> +bool ufshcd_keyslot_valid(struct ufs_hba *hba, unsigned int slot)
> +{
> +	/*
> +	 * The actual number of configurations supported is (CFGC+1), so slot
> +	 * numbers range from 0 to config_count inclusive.
> +	 */
> +	return slot < NUM_KEYSLOTS(hba);
> +}

Since ufshcd_hba_is_crypto_supported(), ufshcd_is_crypto_enabled(), and
ufshcd_keyslot_valid() are one-liners, don't access any private structures, and
are used outside this file including on the command submission path, how about
making them inline functions in ufshcd-crypto.h?

> +
> +static int ufshcd_crypto_alg_find(void *hba_p,
> +			   enum blk_crypt_mode_num crypt_mode,
> +			   unsigned int data_unit_size)
> +{

Now that the concept of "crypto alg IDs" is gone, rename this to
ufshcd_crypto_cap_find() and rename the crypto_alg_id variables to cap_idx.

This would make it consistent with using cap_idx elsewhere in the code and avoid
confusion with ufs_crypto_cap_entry::algorithm_id.

> +
> +static int ufshcd_crypto_keyslot_program(void *hba_p, const u8 *key,
> +					 enum blk_crypt_mode_num crypt_mode,
> +					 unsigned int data_unit_size,
> +					 unsigned int slot)
> +{
> +	struct ufs_hba *hba = hba_p;
> +	int err = 0;
> +	u8 data_unit_mask;
> +	union ufs_crypto_cfg_entry cfg;
> +	union ufs_crypto_cfg_entry *cfg_arr = hba->crypto_cfgs;
> +	int crypto_alg_id;
> +
> +	crypto_alg_id = ufshcd_crypto_alg_find(hba_p, crypt_mode,
> +					       data_unit_size);
> +
> +	if (!ufshcd_is_crypto_enabled(hba) ||
> +	    !ufshcd_keyslot_valid(hba, slot) ||
> +	    !ufshcd_cap_idx_valid(hba, crypto_alg_id))
> +		return -EINVAL;
> +
> +	data_unit_mask = get_data_unit_size_mask(data_unit_size);
> +
> +	if (!(data_unit_mask &
> +	      hba->crypto_cap_array[crypto_alg_id].sdus_mask))
> +		return -EINVAL;

Nit: the 'if' expression with data_unit_mask fits on one line.

> +static int ufshcd_crypto_keyslot_find(void *hba_p,
> +				      const u8 *key,
> +				      enum blk_crypt_mode_num crypt_mode,
> +				      unsigned int data_unit_size)
> +{
> +	struct ufs_hba *hba = hba_p;
> +	int err = 0;
> +	int slot;
> +	u8 data_unit_mask;
> +	union ufs_crypto_cfg_entry cfg;
> +	union ufs_crypto_cfg_entry *cfg_arr = hba->crypto_cfgs;
> +	int crypto_alg_id;
> +
> +	crypto_alg_id = ufshcd_crypto_alg_find(hba_p, crypt_mode,
> +					       data_unit_size);
> +
> +	if (!ufshcd_is_crypto_enabled(hba) ||
> +	    !ufshcd_cap_idx_valid(hba, crypto_alg_id))
> +		return -EINVAL;
> +
> +	data_unit_mask = get_data_unit_size_mask(data_unit_size);
> +
> +	if (!(data_unit_mask &
> +	      hba->crypto_cap_array[crypto_alg_id].sdus_mask))
> +		return -EINVAL;

Same here.

> +	for (slot = 0; slot < NUM_KEYSLOTS(hba); slot++) {
> +		if ((cfg_arr[slot].config_enable &
> +		     UFS_CRYPTO_CONFIGURATION_ENABLE) &&
> +		    data_unit_mask == cfg_arr[slot].data_unit_size &&
> +		    crypto_alg_id == cfg_arr[slot].crypto_cap_idx &&
> +		    crypto_memneq(&cfg.crypto_key, cfg_arr[slot].crypto_key,
> +				  UFS_CRYPTO_KEY_MAX_SIZE) == 0) {
> +			memzero_explicit(&cfg, sizeof(cfg));
> +			return slot;
> +		}
> +	}

Nit: as I've mentioned before, I think !crypto_memneq() is easier to read than
'crypto_memneq() == 0'.

> +	hba->crypto_cap_array =
> +		devm_kcalloc(hba->dev,
> +			     hba->crypto_capabilities.num_crypto_cap,
> +			     sizeof(hba->crypto_cap_array[0]),
> +			     GFP_KERNEL);
> +	if (!hba->crypto_cap_array) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +
> +	hba->crypto_cfgs =
> +		devm_kcalloc(hba->dev,
> +			     hba->crypto_capabilities.config_count + 1,
> +			     sizeof(union ufs_crypto_cfg_entry),
> +			     GFP_KERNEL);
> +	if (!hba->crypto_cfgs) {
> +		err = -ENOMEM;
> +		goto out_cfg_mem;
> +	}

Nit: use 'sizeof(hba->crypto_cfgs[0])' rather than 'sizeof(union
ufs_crypto_cfg_entry)', for consistency with the other array allocation.

Thanks,

- Eric