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@xxxxxxxxxxxxx/)
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
