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

[Herbert Xu <herbert@gondor.apana.org.au>]

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,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt