Re: [PATCH v4 4/7] soc: qcom: Make the Qualcomm UFS/SDCC ICE a dedicated driver

[Bjorn Andersson <andersson@kernel.org>]

On Mon, Mar 27, 2023 at 12:09:54PM -0700, Eric Biggers wrote:
> On Mon, Mar 27, 2023 at 11:53:58AM -0700, Bjorn Andersson wrote:
> > > +int qcom_ice_program_key(struct qcom_ice *ice,
> > > +			 u8 algorithm_id, u8 key_size,
> > > +			 const u8 crypto_key[], u8 data_unit_size,
> > > +			 int slot)
> > > +{
> > > +	struct device *dev = ice->dev;
> > > +	union {
> > > +		u8 bytes[AES_256_XTS_KEY_SIZE];
> > > +		u32 words[AES_256_XTS_KEY_SIZE / sizeof(u32)];
> > > +	} key;
> > > +	int i;
> > > +	int err;
> > > +
> > > +	/* Only AES-256-XTS has been tested so far. */
> > > +	if (algorithm_id != QCOM_ICE_CRYPTO_ALG_AES_XTS ||
> > > +	    key_size != QCOM_ICE_CRYPTO_KEY_SIZE_256) {
> > > +		dev_err_ratelimited(dev,
> > > +				    "Unhandled crypto capability; algorithm_id=%d, key_size=%d\n",
> > > +				    algorithm_id, key_size);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	memcpy(key.bytes, crypto_key, AES_256_XTS_KEY_SIZE);
> > > +
> > > +	/*
> > > +	 * The SCM call byte-swaps the 32-bit words of the key.
> > > +	 * So we have to do the same, in order for the final key be correct.
> > 
> > Does it actually byte swap the words, or is the API just specified to
> > take the words in big endian format?
> 
> [Note, this is existing code I wrote that Abel is just moving to a new file.]
> 

Ah right, then I'm inclined to keep it untouched.

> It doesn't write to the input array, if that is what you are asking.  I was
> thinking of this as one byte swap cancelling out another.  But sure, the comment
> could be simplified to something like the following:
> 
> 	/* The SCM call requires that the key words be byte-swapped. */
> 

Last time I looked at a crypto driver, it was full of "switch the
endian" operations, back and forth. So my request here was simply to
make it clear which endian is actually expected.
So I'm guessing the appropriate comment is:

	/* The SCM call requires that the key words are encoded in big endian */

> > How come you memcpy + swap in place, instead of loop over the words and
> > cpu_to_be32() them into a __be words[] array?
> > 
> > > +	 */
> > > +	for (i = 0; i < ARRAY_SIZE(key.words); i++)
> > > +		__cpu_to_be32s(&key.words[i]);
> 
> With this approach there is no need to worry about unaligned memory accesses.

That's a valid reason that I was looking for. Wouldn't this be a common
problem, something other parts of the stack would like to avoid?
Or it's just a byte array until we get here?

> It could be done with unaligned memory accesses, though, if you prefer that:
> 

No need to jump through the hoops, but a comment would have saved
(robbed?) me from wondering.

Regards,
Bjorn

> 	union {
> 		[...]
> 		__be32 words[...];
> 	} key;
> 
> 	[...]
> 		key.words[i] = cpu_to_be32(get_unaligned((__u32 *)crypto_key + i));
> 
> - Eric