Skip to contentSkip to conversation

New s390 specific protected key hmac

PATCH Superseded Harald Freudenberger 3 participants 20 messages Jan 15, 2025 – Feb 22, 2025

Subsystems (5)

v10 of 12 Superseded Latest is v12 →

lore
Share & tools
lore

Harald Freudenberger

author maintainer
#0 lore raw

Add support for protected key hmac ("phmac") for s390 arch.

With the latest machine generation there is now support for protected key (that is a key wrapped by a master key stored in firmware) hmac for sha2 (sha224, sha256, sha384 and sha512) for the s390 specific CPACF instruction kmac.

This patch adds support via 4 new hashes registered as phmac(sha224), phmac(sha256), phmac(sha384) and phmac(sha512).

Changelog:
v1: Initial version
v2: Increase HASH_MAX_DESCSIZE generic (not just for arch s390).
Fix one finding to use kmemdup instead of kmalloc/memcpy from test robot. Remove unneeded cpacf subfunctions checks. Simplify clone_tfm() function. Rebased to s390/features.
v3: Feedback from Herbert: Use GFP_ATOMIC in setkey function.
Feedback from Holger: rework tfm clone function, move convert key invocation from setkey to init function. Rebased to updated s390/features from 11/7/2024. Ready for integration if there are no complains on v3.
v4: Rewind back more or less to v2. Add code to check for non-sleeping
context. Non-sleeping context during attempt to derive the protected key from raw key material is not accepted and -EOPNOTSUPP is returned (also currently all derivation pathes would in fact never sleep). In general the phmac implementation is not to be used within non-sleeping context and the code header mentions this. Tested with (patched) dm-integrity - works fine.
v5: As suggested by Herbert now the shashes have been marked as
'internal' and wrapped by ahashes which use the cryptd if an atomic context is detected. So the visible phmac algorithms are now ahashes. Unfortunately the dm-integrity implementation currently requests and deals only with shashes and this phmac implementation is not fitting to the original goal any more...
v6: As suggested by Herbert now a pure async phmac implementation.
Tested via AF_ALG interface. Untested via dm-integrity as this layer only supports shashes. Maybe I'll develop a patch to switch the dm-integrity to ahash as it is anyway the more flexible interface.
v7: Total rework of the implementation. Now uses workqueues and triggers
asynch requests for key convert, init, update, final and digest. Tested with instrumented code and with a reworked version of dm-integrity which uses asynchronous hashes. A patch for dm-integrity is on the way but yet needs some last hone work.
v8: Added selftest. With the selftest comes some code which wraps the
clear key into a "clear key token" digestible by PKEY. The selftest also uses import() and export(), so these are now also implemented. Furthermore a finup() implementation is now also available. Tested with AF_ALG testcases and dm-integrity, also tested with some instrumented code to check that the asynch workqueue functions do their job correctly. Coding is complete!
v9: As suggested by Herbert use ahash_request_complete() and surround it
with local_bh_disable().
v10: Split the pkey selftest patch into 3 patches. Slight rework of the
setkey function as suggested by Holger: When selftest is running as much as possible of the production code should run. So now the key prep with selftest is one additional if/then block instead of an if/then/else construct.
Code is ready for integration and well tested.

Harald Freudenberger (4):
s390/crypto: New s390 specific protected key hash phmac
crypto: api - Add crypto_tfm_alg_get_flags() helper inline function s390/crypto: Add selftest support for phmac
crypto: testmgr - Enable phmac selftest

Holger Dengler (1):
s390/crypto: Add protected key hmac subfunctions for KMAC

 arch/s390/configs/debug_defconfig |   1 +
 arch/s390/configs/defconfig       |   1 +
 arch/s390/crypto/Makefile         |   1 +
 arch/s390/crypto/phmac_s390.c     | 995 ++++++++++++++++++++++++++++++
 arch/s390/include/asm/cpacf.h     |   4 +
 crypto/testmgr.c                  |  30 +
 drivers/crypto/Kconfig            |  12 +
 include/linux/crypto.h            |   5 +
 8 files changed, 1049 insertions(+)
 create mode 100644 arch/s390/crypto/phmac_s390.c

base-commit: fac04efc5c793dccbd07e2d59af9f90b7fc0dca4

Signature · 1 line
2.43.0
author maintainer
#1 lore raw

PATCH v10 1/5 s390/crypto: Add protected key hmac subfunctions for KMAC

From: Holger Dengler <[email protected]>

The CPACF KMAC instruction supports new subfunctions for protected key hmac. Add defines for these 4 new subfuctions.

---
 arch/s390/include/asm/cpacf.h | 4 ++++
 1 file changed, 4 insertions(+)
arch/s390/include/asm/cpacf.h +4 -0
index 59ab1192e2d5..0468dd5c4690 100644
@@ -129,6 +129,10 @@
 #define CPACF_KMAC_HMAC_SHA_256	0x71
 #define CPACF_KMAC_HMAC_SHA_384	0x72
 #define CPACF_KMAC_HMAC_SHA_512	0x73
+#define CPACF_KMAC_PHMAC_SHA_224	0x78
+#define CPACF_KMAC_PHMAC_SHA_256	0x79
+#define CPACF_KMAC_PHMAC_SHA_384	0x7a
+#define CPACF_KMAC_PHMAC_SHA_512	0x7b
 
 /*
  * Function codes for the PCKMO (PERFORM CRYPTOGRAPHIC KEY MANAGEMENT)
Signature · 1 line
2.43.0
author maintainer
#2 lore raw

PATCH v10 2/5 s390/crypto: New s390 specific protected key hash phmac

In reply to Harald Freudenberger

Add support for protected key hmac ("phmac") for s390 arch.

With the latest machine generation there is now support for protected key (that is a key wrapped by a master key stored in firmware) hmac for sha2 (sha224, sha256, sha384 and sha512) for the s390 specific CPACF instruction kmac.

This patch adds support via 4 new ahashes registered as phmac(sha224), phmac(sha256), phmac(sha384) and phmac(sha512).

---
 arch/s390/configs/debug_defconfig |   1 +
 arch/s390/configs/defconfig       |   1 +
 arch/s390/crypto/Makefile         |   1 +
 arch/s390/crypto/phmac_s390.c     | 852 ++++++++++++++++++++++++++++++
 drivers/crypto/Kconfig            |  12 +
 5 files changed, 867 insertions(+)
 create mode 100644 arch/s390/crypto/phmac_s390.c
arch/s390/configs/debug_defconfig +1 -0
index d8d227ab82de..c857618087fe 100644
@@ -813,6 +813,7 @@ CONFIG_PKEY_EP11=m
 CONFIG_PKEY_PCKMO=m
 CONFIG_PKEY_UV=m
 CONFIG_CRYPTO_PAES_S390=m
+CONFIG_CRYPTO_PHMAC_S390=m
 CONFIG_CRYPTO_DEV_VIRTIO=m
 CONFIG_SYSTEM_BLACKLIST_KEYRING=y
 CONFIG_CORDIC=m
arch/s390/configs/defconfig +1 -0
index 6c2f2bb4fbf8..94bde39967a5 100644
@@ -800,6 +800,7 @@ CONFIG_PKEY_EP11=m
 CONFIG_PKEY_PCKMO=m
 CONFIG_PKEY_UV=m
 CONFIG_CRYPTO_PAES_S390=m
+CONFIG_CRYPTO_PHMAC_S390=m
 CONFIG_CRYPTO_DEV_VIRTIO=m
 CONFIG_SYSTEM_BLACKLIST_KEYRING=y
 CONFIG_CORDIC=m
arch/s390/crypto/Makefile +1 -0
index a0cb96937c3d..47637140b95c 100644
@@ -16,6 +16,7 @@ obj-$(CONFIG_S390_PRNG) += prng.o
 obj-$(CONFIG_CRYPTO_GHASH_S390) += ghash_s390.o
 obj-$(CONFIG_CRYPTO_CRC32_S390) += crc32-vx_s390.o
 obj-$(CONFIG_CRYPTO_HMAC_S390) += hmac_s390.o
+obj-$(CONFIG_CRYPTO_PHMAC_S390) += phmac_s390.o
 obj-y += arch_random.o
 
 crc32-vx_s390-y := crc32-vx.o crc32le-vx.o crc32be-vx.o
arch/s390/crypto/phmac_s390.c new file +852 -0

852 lines not on this page · raw message

drivers/crypto/Kconfig +12 -0
index 0a9cdd31cbd9..519305e04f18 100644
@@ -187,6 +187,18 @@ config CRYPTO_PAES_S390
 	  Select this option if you want to use the paes cipher
 	  for example to use protected key encrypted devices.
 
+config CRYPTO_PHMAC_S390
+	tristate "PHMAC cipher algorithms"
+	depends on S390
+	depends on PKEY
+	select CRYPTO_HASH
+	help
+	  This is the s390 hardware accelerated implementation of the
+	  protected key HMAC support for SHA224, SHA256, SHA384 and SHA512.
+
+	  Select this option if you want to use the phmac digests
+	  for example to use dm-integrity with secure/protected keys.
+
 config S390_PRNG
 	tristate "Pseudo random number generator device driver"
 	depends on S390
Signature · 1 line
2.43.0
maintainer
#3 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 11 lines · Harald Freudenberger
arch/s390/crypto/phmac_s390.c new file +10 -0
+static int s390_phmac_init(struct ahash_request *req)
+{
+	struct s390_phmac_req_ctx *req_ctx = ahash_request_ctx(req);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+	struct s390_kmac_sha2_ctx *ctx = &req_ctx->sha2_ctx;
+	int rc;
+
+	/*
+	 * First try synchronous. If this fails for any reason
+	 * schedule this request asynchronous via workqueue.

arch/s390/crypto/phmac_s390.c line 300

Quoted 10 lines
arch/s390/crypto/phmac_s390.c new file +10 -0
+	 */
+
+	rc = phmac_init(tfm, ctx, false);
+	if (!rc)
+		goto out;
+
+	req_ctx->req = req;
+	INIT_DELAYED_WORK(&req_ctx->work, phmac_wq_init_fn);
+	schedule_delayed_work(&req_ctx->work, 0);
+	rc = -EINPROGRESS;

This creates a resource problem because there is no limit on how many requests that can be delayed in this manner for a given tfm.

When we hit this case, I presume this is a system-wide issue and all requests would go pending? If that is the case, I suggest allocating a system-wide queue through crypto_engine and using that to limit how many requests that can become EINPROGRESS.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#4 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 26 lines · Herbert Xu
Quoted 11 lines · Harald Freudenberger
arch/s390/crypto/phmac_s390.c new file +10 -0
+static int s390_phmac_init(struct ahash_request *req)
+{
+	struct s390_phmac_req_ctx *req_ctx = ahash_request_ctx(req);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+	struct s390_kmac_sha2_ctx *ctx = &req_ctx->sha2_ctx;
+	int rc;
+
+	/*
+	 * First try synchronous. If this fails for any reason
+	 * schedule this request asynchronous via workqueue.

arch/s390/crypto/phmac_s390.c line 300

Quoted 10 lines
arch/s390/crypto/phmac_s390.c new file +10 -0
+	 */
+
+	rc = phmac_init(tfm, ctx, false);
+	if (!rc)
+		goto out;
+
+	req_ctx->req = req;
+	INIT_DELAYED_WORK(&req_ctx->work, phmac_wq_init_fn);
+	schedule_delayed_work(&req_ctx->work, 0);
+	rc = -EINPROGRESS;

This creates a resource problem because there is no limit on how many requests that can be delayed in this manner for a given tfm.

Quoted 4 lines

When we hit this case, I presume this is a system-wide issue and all requests would go pending? If that is the case, I suggest allocating a system-wide queue through crypto_engine and using that to limit how many requests that can become EINPROGRESS.

Or just make it synchronous which would be way easier, and the calling code uses it synchronously anyway.

- Eric

maintainer
#5 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 3 lines · Eric Biggers

Or just make it synchronous which would be way easier, and the calling code uses it synchronously anyway.

Note that synchronous in general does not make the problem go away. The important thing here is to give congestion feedback in the form of EBUSY which tells the user to stop generating more data until a callback is made.

While synchronous can be a form of congestion control by requiring an extra thread for each waiting request, it doesn't really give that feedback to the upper level.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
#6 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 5 lines · Herbert Xu
Quoted 3 lines · Eric Biggers

Or just make it synchronous which would be way easier, and the calling code uses it synchronously anyway.

Quoted 9 lines

Note that synchronous in general does not make the problem go away. The important thing here is to give congestion feedback in the form of EBUSY which tells the user to stop generating more data until a callback is made.

While synchronous can be a form of congestion control by requiring an extra thread for each waiting request, it doesn't really give that feedback to the upper level.

Which is of course entirely theoretical, given that the proposed user waits synchronously for each request to complete anyway. And hardly anyone wants to do otherwise since it is way too much of a pain.

- Eric

maintainer
#7 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 4 lines · Eric Biggers

Which is of course entirely theoretical, given that the proposed user waits synchronously for each request to complete anyway. And hardly anyone wants to do otherwise since it is way too much of a pain.

We don't design things for a single user. When you write an algorithm through the Crypto API, it becomes available to *all* users.

Just because a single user might be naturally resistant to congestion, it doesn't make this a non-issue or theoretical.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author maintainer
#8 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

In reply to Eric Biggers
Quoted 32 lines · Eric Biggers
Quoted 26 lines · Herbert Xu
Quoted 11 lines · Harald Freudenberger
arch/s390/crypto/phmac_s390.c new file +10 -0
+static int s390_phmac_init(struct ahash_request *req)
+{
+	struct s390_phmac_req_ctx *req_ctx = ahash_request_ctx(req);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+	struct s390_kmac_sha2_ctx *ctx = &req_ctx->sha2_ctx;
+	int rc;
+
+	/*
+	 * First try synchronous. If this fails for any reason
+	 * schedule this request asynchronous via workqueue.

arch/s390/crypto/phmac_s390.c line 300

Quoted 10 lines
arch/s390/crypto/phmac_s390.c new file +10 -0
+	 */
+
+	rc = phmac_init(tfm, ctx, false);
+	if (!rc)
+		goto out;
+
+	req_ctx->req = req;
+	INIT_DELAYED_WORK(&req_ctx->work, phmac_wq_init_fn);
+	schedule_delayed_work(&req_ctx->work, 0);
+	rc = -EINPROGRESS;

This creates a resource problem because there is no limit on how many requests that can be delayed in this manner for a given tfm.

Quoted 4 lines

When we hit this case, I presume this is a system-wide issue and all requests would go pending? If that is the case, I suggest allocating a system-wide queue through crypto_engine and using that to limit how many requests that can become EINPROGRESS.

Quoted 4 lines

Or just make it synchronous which would be way easier, and the calling code uses it synchronously anyway.

- Eric

A word about synchronous vs asynchronous...

As a synchronous hash (or chipher or whatever) MUST NOT sleep I can't really implement the pkey stuff in a synchronous way:

The issue with pkey (We call it "protected key") is that it is some kind of hardware based key. As such it needs some special preparation action to be done upfront in the hardware/firmware to use such a pkey. Now think about KVM live guest migration where a guest suddenly awakes (Well the guest is not even aware of this) on a new machine with another hardware. So out of the sudden a hardware based crypto operation fails with an indication that the hardware/firmware can't deal with this key object and needs re-preparation. Usually this preparation step is some kind of asynchronous operation (write some pci registers or run some DMA sequences or refresh the working key material via an HSM communication...) and as such may take some time and involve even sleeping on a mutex or completion until another kernel thread is done. Please note this is not unique to pkey on system z but may apply to all kinds of hardware/firmware based keys in situations like KVM live guest migration or suspend/resume.

So for the pkey algorithms I can't guarantee that all the crypto operations do never sleep and thus an asynchronous implementation is the only way and makes sense to me.

There are other downsides with the asynch implementations:
They are much more complex and thus expensive and - how the hell can a test cover all the code branches?
Next is the thing with the CRYPTO_ALG_ALLOCATES_MEMORY flag. If I want to have a hash implementation usable for dm-integrity the alg implementation must NOT set this flag and must not allocate any memory during crypto operations (includung setkey()). As of now I am still not through with the phmac code for this. And I only have a faint idea on how to implement this on the pkey and maybe zcrypt code...

kind regards
Harald Freudenberger

#9 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 37 lines · Harald Freudenberger
Quoted 36 lines · Eric Biggers
Quoted 26 lines · Herbert Xu
Quoted 11 lines · Harald Freudenberger
arch/s390/crypto/phmac_s390.c new file +10 -0
+static int s390_phmac_init(struct ahash_request *req)
+{
+	struct s390_phmac_req_ctx *req_ctx = ahash_request_ctx(req);
+	struct crypto_ahash *tfm = crypto_ahash_reqtfm(req);
+	struct s390_kmac_sha2_ctx *ctx = &req_ctx->sha2_ctx;
+	int rc;
+
+	/*
+	 * First try synchronous. If this fails for any reason
+	 * schedule this request asynchronous via workqueue.

arch/s390/crypto/phmac_s390.c line 300

Quoted 10 lines
arch/s390/crypto/phmac_s390.c new file +10 -0
+	 */
+
+	rc = phmac_init(tfm, ctx, false);
+	if (!rc)
+		goto out;
+
+	req_ctx->req = req;
+	INIT_DELAYED_WORK(&req_ctx->work, phmac_wq_init_fn);
+	schedule_delayed_work(&req_ctx->work, 0);
+	rc = -EINPROGRESS;

This creates a resource problem because there is no limit on how many requests that can be delayed in this manner for a given tfm.

Quoted 4 lines

When we hit this case, I presume this is a system-wide issue and all requests would go pending? If that is the case, I suggest allocating a system-wide queue through crypto_engine and using that to limit how many requests that can become EINPROGRESS.

Or just make it synchronous which would be way easier, and the calling code uses
it synchronously anyway.

Quoted 6 lines
Quoted 1 line

- Eric

A word about synchronous vs asynchronous...

As a synchronous hash (or chipher or whatever) MUST NOT sleep I can't really implement the pkey stuff in a synchronous way:

As I said at
https://lore.kernel.org/dm-devel/[email protected]/, shash could fairly easily be fixed to support sleepable algorithms (e.g. CRYPTO_ALG_SLEEPABLE).

This would be *much* simpler than doing it with ahash.

You even had it as a shash already in the first version of your patchset, just missing the bits that add the support for sleepable algorithms.

I am trying to help you by suggesting an approach that would be much easier. There is no need to shoehorn CPU-based crypto into ahash, which is designed for off-CPU offload.

Quoted 7 lines

The issue with pkey (We call it "protected key") is that it is some kind of hardware based key. As such it needs some special preparation action to be done upfront in the hardware/firmware to use such a pkey. Now think about KVM live guest migration where a guest suddenly awakes (Well the guest is not even aware of this) on a new machine with another hardware. So out of the sudden a hardware based crypto operation fails with an indication that the hardware/firmware can't deal with this

Quoted 8 lines

key object and needs re-preparation. Usually this preparation step is some kind of asynchronous operation (write some pci registers or run some DMA sequences or refresh the working key material via an HSM communication...) and as such may take some time and involve even sleeping on a mutex or completion until another kernel thread is done. Please note this is not unique to pkey on system z but may apply to all kinds of hardware/firmware based keys in situations like KVM live guest migration or suspend/resume.

I.e. it already uses a kernel thread that does the operation synchronously (as opposed to being actual hardware offload that does not consume a thread and signals completion to the CPU via an interrupt), in addition to the caller's thread which also waits synchronously via crypto_wait_req(). There is really no need to make it async, nor to use scatterlists.

- Eric

maintainer
#10 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

In reply to Harald Freudenberger
Quoted 4 lines · Harald Freudenberger

There are other downsides with the asynch implementations:
They are much more complex and thus expensive and - how the hell can a test cover all the code branches?

I think rather than writing an ahash algorithm from scratch, this could be done in a way similar to the simd wrapper.

In fact, forget about phmac for the moment, and please fix the one that's already in the kernel: paes.

Under the simd model, you turn paes into an internal algorithm that is always synchronous. In the exceptional case, you return an error so that the wrapper can then enter the async path.

The wrapper handles all the complexities of the async path and hides it from the actual paes code.

Quoted 6 lines

Next is the thing with the CRYPTO_ALG_ALLOCATES_MEMORY flag. If I want to have a hash implementation usable for dm-integrity the alg implementation must NOT set this flag and must not allocate any memory during crypto operations (includung setkey()). As of now I am still not through with the phmac code for this. And I only have a faint idea on how to implement this on the pkey and maybe zcrypt code...

Why do you need to allocate memory? If anything ahash makes this slightly easier since you can allocate your own memory beforehand through the request object.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author maintainer
#11 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

In reply to Harald Freudenberger
Quoted 14 lines · Harald Freudenberger

Add support for protected key hmac ("phmac") for s390 arch.

With the latest machine generation there is now support for protected key (that is a key wrapped by a master key stored in firmware) hmac for sha2 (sha224, sha256, sha384 and sha512) for the s390 specific CPACF instruction kmac.

This patch adds support via 4 new ahashes registered as phmac(sha224), phmac(sha256), phmac(sha384) and phmac(sha512).

---

...

drivers/crypto/Kconfig config CRYPTO_PAES_S390 · line 204

Quoted 7 lines
drivers/crypto/Kconfig +4 -0
+
+	  Select this option if you want to use the phmac digests
+	  for example to use dm-integrity with secure/protected keys.
+
 config S390_PRNG
 	tristate "Pseudo random number generator device driver"
 	depends on S390

Hello Eric

with your commit
7fa481734016 crypto: ahash - make hash walk functions private to
ahash.c
you move the crypto_hash_walk_* functions to private context into ahash.c. So for example my new phmac does not compile any more.
What is the replacement for these functions then? Should I really do the scatter walk by my own now or is there a set of new functions to come?

#12 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

[+dm-devel]

Quoted 28 lines · Harald Freudenberger
Quoted 14 lines · Harald Freudenberger

Add support for protected key hmac ("phmac") for s390 arch.

With the latest machine generation there is now support for protected key (that is a key wrapped by a master key stored in firmware) hmac for sha2 (sha224, sha256, sha384 and sha512) for the s390 specific CPACF instruction kmac.

This patch adds support via 4 new ahashes registered as phmac(sha224), phmac(sha256), phmac(sha384) and phmac(sha512).

---

...

drivers/crypto/Kconfig config CRYPTO_PAES_S390 · line 204

Quoted 7 lines
drivers/crypto/Kconfig +4 -0
+
+	  Select this option if you want to use the phmac digests
+	  for example to use dm-integrity with secure/protected keys.
+
 config S390_PRNG
 	tristate "Pseudo random number generator device driver"
 	depends on S390

Hello Eric

Quoted 6 lines

with your commit
7fa481734016 crypto: ahash - make hash walk functions private to ahash.c
you move the crypto_hash_walk_* functions to private context into ahash.c. So for example my new phmac does not compile any more.
What is the replacement for these functions then? Should I really do the scatter walk by my own now or is there a set of new functions to come?

This is just another ridiculous problem caused by trying to shoehorn CPU-based crypto into ahash which is an interface designed for off-CPU offload. I've already given the solution here:
https://lore.kernel.org/dm-devel/[email protected]/

- Eric

maintainer
#13 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

In reply to Harald Freudenberger
Quoted 7 lines · Harald Freudenberger

with your commit
7fa481734016 crypto: ahash - make hash walk functions private to ahash.c
you move the crypto_hash_walk_* functions to private context into ahash.c. So for example my new phmac does not compile any more.
What is the replacement for these functions then? Should I really do the scatter walk by my own now or is there a set of new functions to come?

Don't worry about this. It can easily be reverted once a patch using it is submitted.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
maintainer
#14 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 3 lines · Herbert Xu

Don't worry about this. It can easily be reverted once a patch using it is submitted.

I just reverted it in cryptodev.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Nacked-by NAK
#15 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 7 lines · Herbert Xu
Quoted 3 lines · Herbert Xu

Don't worry about this. It can easily be reverted once a patch using it is submitted.

I just reverted it in cryptodev.

- Eric

maintainer
#16 lore raw

Re: [PATCH v10 2/5] s390/crypto: New s390 specific protected key hash phmac

Quoted 10 lines · Eric Biggers
Quoted 7 lines · Herbert Xu
Quoted 3 lines · Herbert Xu

Don't worry about this. It can easily be reverted once a patch using it is submitted.

I just reverted it in cryptodev.

I actually pulled the revert out again, not so much because of your nack, but because I'm going to redo the walking helpers to incorporate direct support for multibuffer.

So it's more convenient if the old walk interface is moved out of the way first.

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author maintainer
#17 lore raw

PATCH v10 3/5 crypto: api - Add crypto_tfm_alg_get_flags() helper inline function

In reply to Harald Freudenberger

Add a little helper inline function
crypto_tfm_alg_get_flags()
to the crypto.h header file to retrieve the alg flags.

---
 include/linux/crypto.h | 5 +++++
 1 file changed, 5 insertions(+)
include/linux/crypto.h +5 -0
index b164da5e129e..8b37d381cd97 100644
@@ -472,6 +472,11 @@ static inline unsigned int crypto_tfm_alg_alignmask(struct crypto_tfm *tfm)
 	return tfm->__crt_alg->cra_alignmask;
 }
 
+static inline u32 crypto_tfm_alg_get_flags(struct crypto_tfm *tfm)
+{
+	return tfm->__crt_alg->cra_flags;
+}
+
 static inline u32 crypto_tfm_get_flags(struct crypto_tfm *tfm)
 {
 	return tfm->crt_flags;
Signature · 1 line
2.43.0
author maintainer
#18 lore raw

PATCH v10 4/5 s390/crypto: Add selftest support for phmac

In reply to Harald Freudenberger

Add key preparation code in case of selftest running to the phmac setkey function:

As long as the CRYPTO_ALG_TESTED flag is not set, all setkey() invocations are assumed to carry sheer hmac clear key values and thus need some preparation to work with the phmac implementation. Thus it is possible to use the already available hmac test vectors implemented in the testmanager to test the phmac code.

When the CRYPTO_ALG_TESTED flag is set (after larval state) the phmac code assumes the key material is a blob digestible by the pkey kernel module which converts the blob into a working key for the phmac code.

---
 arch/s390/crypto/phmac_s390.c | 151 +++++++++++++++++++++++++++++++++-
 1 file changed, 147 insertions(+), 4 deletions(-)
arch/s390/crypto/phmac_s390.c +147 -4
index b2b389e94a37..5123664e4d2e 100644
@@ -112,6 +112,19 @@ struct s390_phmac_req_ctx {
 	struct s390_kmac_sha2_ctx sha2_ctx;
 };
 
+/*
+ * Pkey 'token' struct used to derive a protected key value from a clear key.
+ */
+struct hmac_clrkey_token {
+	u8  type;
+	u8  res0[3];
+	u8  version;
+	u8  res1[3];
+	u32 keytype;
+	u32 len;
+	u8 key[];
+} __packed;
+
 /*
  * kmac_sha2_set_imbl - sets the input message bit-length based on the blocksize
  */
@@ -132,6 +145,101 @@ static inline void kmac_sha2_set_imbl(u8 *param, unsigned int buflen,
 	}
 }
 
+static int hash_key(const u8 *in, unsigned int inlen,
+		    u8 *digest, unsigned int digestsize)
+{
+	unsigned long func;
+	union {
+		struct sha256_paramblock {
+			u32 h[8];
+			u64 mbl;
+		} sha256;
+		struct sha512_paramblock {
+			u64 h[8];
+			u128 mbl;
+		} sha512;
+	} __packed param;
+
+#define PARAM_INIT(x, y, z)		   \
+	param.sha##x.h[0] = SHA##y ## _H0; \
+	param.sha##x.h[1] = SHA##y ## _H1; \
+	param.sha##x.h[2] = SHA##y ## _H2; \
+	param.sha##x.h[3] = SHA##y ## _H3; \
+	param.sha##x.h[4] = SHA##y ## _H4; \
+	param.sha##x.h[5] = SHA##y ## _H5; \
+	param.sha##x.h[6] = SHA##y ## _H6; \
+	param.sha##x.h[7] = SHA##y ## _H7; \
+	param.sha##x.mbl = (z)
+
+	switch (digestsize) {
+	case SHA224_DIGEST_SIZE:
+		func = CPACF_KLMD_SHA_256;
+		PARAM_INIT(256, 224, inlen * 8);
+		break;
+	case SHA256_DIGEST_SIZE:
+		func = CPACF_KLMD_SHA_256;
+		PARAM_INIT(256, 256, inlen * 8);
+		break;
+	case SHA384_DIGEST_SIZE:
+		func = CPACF_KLMD_SHA_512;
+		PARAM_INIT(512, 384, inlen * 8);
+		break;
+	case SHA512_DIGEST_SIZE:
+		func = CPACF_KLMD_SHA_512;
+		PARAM_INIT(512, 512, inlen * 8);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+#undef PARAM_INIT
+
+	cpacf_klmd(func, &param, in, inlen);
+
+	memcpy(digest, &param, digestsize);
+
+	return 0;
+}
+
+/*
+ * make_clrkey_token() - wrap the clear key into a pkey clearkey token.
+ */
+static inline int make_clrkey_token(const u8 *clrkey, size_t clrkeylen,
+				    unsigned int digestsize, u8 *dest)
+{
+	struct hmac_clrkey_token *token = (struct hmac_clrkey_token *)dest;
+	unsigned int blocksize;
+	int rc;
+
+	token->type = 0x00;
+	token->version = 0x02;
+	switch (digestsize) {
+	case SHA224_DIGEST_SIZE:
+	case SHA256_DIGEST_SIZE:
+		token->keytype = PKEY_KEYTYPE_HMAC_512;
+		blocksize = 64;
+		break;
+	case SHA384_DIGEST_SIZE:
+	case SHA512_DIGEST_SIZE:
+		token->keytype = PKEY_KEYTYPE_HMAC_1024;
+		blocksize = 128;
+		break;
+	default:
+		return -EINVAL;
+	}
+	token->len = blocksize;
+
+	if (clrkeylen > blocksize) {
+		rc = hash_key(clrkey, clrkeylen, token->key, digestsize);
+		if (rc)
+			return rc;
+	} else {
+		memcpy(token->key, clrkey, clrkeylen);
+	}
+
+	return 0;
+}
+
 /*
  * Convert the raw key material into a protected key via PKEY api.
  * This function may sleep - don't call in non-sleeping context.
@@ -680,6 +788,12 @@ static int s390_phmac_setkey(struct crypto_ahash *tfm,
 			     const u8 *key, unsigned int keylen)
 {
 	struct s390_phmac_ctx *tfm_ctx = crypto_ahash_ctx(tfm);
+	struct crypto_tfm *tfm_base = crypto_ahash_tfm(tfm);
+	unsigned int ds = crypto_ahash_digestsize(tfm);
+	unsigned int bs = crypto_ahash_blocksize(tfm);
+	unsigned int tmpkeylen;
+	u8 *tmpkey = NULL;
+	int rc = 0;
 
 	if (tfm_ctx->keylen) {
 		kfree_sensitive(tfm_ctx->key);
@@ -687,9 +801,31 @@ static int s390_phmac_setkey(struct crypto_ahash *tfm,
 		tfm_ctx->keylen = 0;
 	}
 
+	if (!(crypto_tfm_alg_get_flags(tfm_base) & CRYPTO_ALG_TESTED)) {
+		/*
+		 * selftest running: key is a raw hmac clear key and needs
+		 * to get embedded into a 'clear key token' in order to have
+		 * it correctly processed by the pkey module.
+		 */
+		tmpkeylen = sizeof(struct hmac_clrkey_token) + bs;
+		tmpkey = kzalloc(tmpkeylen, GFP_ATOMIC);
+		if (!tmpkey) {
+			rc = -ENOMEM;
+			goto out;
+		}
+		rc = make_clrkey_token(key, keylen, ds, tmpkey);
+		if (rc)
+			goto out;
+		keylen = tmpkeylen;
+		key = tmpkey;
+	}
+
+	/* key is always a key token digestable by PKEY */
 	tfm_ctx->key = kmemdup(key, keylen, GFP_ATOMIC);
-	if (!tfm_ctx->key)
-		return -ENOMEM;
+	if (!tfm_ctx->key) {
+		rc = -ENOMEM;
+		goto out;
+	}
 	tfm_ctx->keylen = keylen;
 
 	/* Always trigger an asynch key convert */
@@ -698,8 +834,10 @@ static int s390_phmac_setkey(struct crypto_ahash *tfm,
 	spin_unlock_bh(&tfm_ctx->pk_lock);
 	schedule_delayed_work(&tfm_ctx->work, 0);
 
-	pr_debug("rc=0\n");
-	return 0;
+out:
+	kfree(tmpkey);
+	pr_debug("rc=%d\n", rc);
+	return rc;
 }
 
 static int s390_phmac_import(struct ahash_request *req, const void *in)
@@ -816,6 +954,11 @@ static int __init phmac_s390_init(void)
 	struct s390_hmac_alg *hmac;
 	int i, rc = -ENODEV;
 
+	if (!cpacf_query_func(CPACF_KLMD, CPACF_KLMD_SHA_256))
+		return -ENODEV;
+	if (!cpacf_query_func(CPACF_KLMD, CPACF_KLMD_SHA_512))
+		return -ENODEV;
+
 	for (i = 0; i < ARRAY_SIZE(s390_hmac_algs); i++) {
 		hmac = &s390_hmac_algs[i];
 		if (!cpacf_query_func(CPACF_KMAC, hmac->fc))
Signature · 1 line
2.43.0
author maintainer
#19 lore raw

PATCH v10 5/5 crypto: testmgr - Enable phmac selftest

In reply to Harald Freudenberger

Add phmac selftest invocation to the crypto testmanager.

---
 crypto/testmgr.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
crypto/testmgr.c +30 -0
index 1f5f48ab18c7..e753a68be861 100644
@@ -5539,6 +5539,36 @@ static const struct alg_test_desc alg_test_descs[] = {
 			.cipher = __VECS(fcrypt_pcbc_tv_template)
 		}
 	}, {
+#if IS_ENABLED(CONFIG_CRYPTO_PHMAC_S390)
+		.alg = "phmac(sha224)",
+		.test = alg_test_hash,
+		.fips_allowed = 1,
+		.suite = {
+			.hash = __VECS(hmac_sha224_tv_template)
+		}
+	}, {
+		.alg = "phmac(sha256)",
+		.test = alg_test_hash,
+		.fips_allowed = 1,
+		.suite = {
+			.hash = __VECS(hmac_sha256_tv_template)
+		}
+	}, {
+		.alg = "phmac(sha384)",
+		.test = alg_test_hash,
+		.fips_allowed = 1,
+		.suite = {
+			.hash = __VECS(hmac_sha384_tv_template)
+		}
+	}, {
+		.alg = "phmac(sha512)",
+		.test = alg_test_hash,
+		.fips_allowed = 1,
+		.suite = {
+			.hash = __VECS(hmac_sha512_tv_template)
+		}
+	}, {
+#endif
 		.alg = "pkcs1(rsa,none)",
 		.test = alg_test_sig,
 		.suite = {
Signature · 1 line
2.43.0