Re: [PATCH v3 3/3] s390/crypto: New s390 specific shash phmac

[Harald Freudenberger <freude@linux.ibm.com>]

On 2024-11-12 02:38, Herbert Xu wrote:
> On Thu, Nov 07, 2024 at 03:55:21PM +0100, Harald Freudenberger wrote:
>> 
>> +static int s390_phmac_sha2_init(struct shash_desc *desc)
>> +{
>> +	struct s390_phmac_ctx *tfm_ctx = crypto_shash_ctx(desc->tfm);
>> +	struct s390_kmac_sha2_ctx *ctx = shash_desc_ctx(desc);
>> +	unsigned int bs = crypto_shash_blocksize(desc->tfm);
>> +	int rc;
>> +
>> +	rc = phmac_convert_key(desc->tfm);
>> +	if (rc)
>> +		goto out;
>> +
>> +	spin_lock_bh(&tfm_ctx->pk_lock);
>> +	memcpy(ctx->param + SHA2_KEY_OFFSET(bs),
>> +	       tfm_ctx->pk.protkey, tfm_ctx->pk.len);
>> +	spin_unlock_bh(&tfm_ctx->pk_lock);
> 
> This appers to be completely broken.  Each tfm can be used by
> an unlimited number of descriptors in parallel.  So you cannot
> modify the tfm context.  I see that you have taken spinlocks
> around it, but it is still broken:
> 
> CPU1			CPU2
> lock(tfm)
> tfm->pk = pk1
> unlock(tfm)
> 			lock(tfm)
> 			tfm->pk = pk2
> 			unlock(tfm)
> lock(tfm)
> copy tfm->pk to desc
> 	pk2 is copied
> unlock(tfm)
> 
> Now this could all be harmless because pk1 and pk2 may be guaranteed
> to be the same, but if that's the case why go through all this in
> the first place? You could've just done it in setkey.
> 
> Cheers,

Well, we had a similar discussion once with paes (See
https://lore.kernel.org/linux-crypto/20191113105523.8007-1-freude@linux.ibm.com/)

The tfm holds the pkey which is a hardware wrapped version of the key 
value.
It is generated by a special invocation done via the PKEY kernel 
module(s) which
knows how to unpack the raw key material and re-wrap it so it can be 
used
with the CPACF instructions. The hardware wrapping key may change - in 
fact
it chances for example with a KVM guest relocated to another system and 
then
this unpack/rewrap cycle needs to be triggered again and thus the pkey 
may
change but the underlying "effective" or "real" key stays the same.
In that sense the tfm holding the pkey value is updated. To make the 
update
of the pkey atomic the spinlock is used as the tfm may be used by 
multiple
hash contexts.

Why not convert in the setkey() function? As of now this could be an 
option
as the invocation of convert_key() in the end within the PKEY pkey_pckmo
kernel module only calls PCKMO to generate the wrapped pkey. However, 
long
term we will have another path using a crypto card for this action and
then we are clearly in a sleeping context which must not be used from
setkey(). So make it correct now means to delay the conversion from 
setkey()
to later: the init of the hash context is the next chance to do this.

I see that calling the conversion each time a shash_init() is called
is total overkill and an avoidable action as the dm-integrity layer 
calls
this per sector. This may even get worse if we intent to go a hardware
path down to convert the key. So I am thinking of a better way which
avoids this overhead ... patch is under construction ...

Regards
Harald Freudenberger