Re: [PATCH v3] s390/cpacf: Unpoison instruction results

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Unconditional KMSAN unpoisoning masks partial completions and information leaks.

Pre-existing issues:
- [Critical] Silent ignoring of PRNO and TRNG partial completions causes critical information leaks.
- [High] Silent ignoring of KMA operation failures causes cryptographic protocol violations.
--

--- Patch [1]: [PATCH v3] s390/cpacf: Unpoison instruction results ---
commit c65c87d6cf548eaef4bd0b06821cb0e68caaa4c3
Author: Ilya Leoshkevich <[email protected]>

s390/cpacf: Unpoison instruction results

This commit suppresses KMSAN uninitialized memory warnings for CPACF
outputs by adding kmsan_unpoison_memory() calls to multiple wrappers.
It intentionally avoids modifying variable-length parameter blocks and
the cpacf_kma() wrapper.

> Also do not touch cpacf_kma(): this wrapper does not handle cc 1 and
> cc2 at the moment and has to be reworked.

[Severity: High]
This is a pre-existing issue, but does silently falling through on Condition
Code 1 and 2 in cpacf_kma() cause callers (like gcm_aes_crypt) to accept
cryptographic errors?

By returning void when the KMA instruction encounters an error (such as
incorrect AAD/plaintext lengths or a hardware abort), callers have no way
of knowing the operation failed. This could result in partially written or
completely uninitialized ciphertext, plaintext, and tags being processed as
valid data.


> diff --git a/arch/s390/include/asm/cpacf.h b/arch/s390/include/asm/cpacf.h
> index a83683169d98d..6174552d856dc 100644
> --- a/arch/s390/include/asm/cpacf.h
> +++ b/arch/s390/include/asm/cpacf.h

[ ... ]

> @@ -619,6 +624,7 @@ static inline void cpacf_prno(unsigned long func, void *param,
>  		: [fc] "d" (func), [pba] "d" ((unsigned long)param),
>  		  [seed] "d" (s.pair), [opc] "i" (CPACF_PRNO)
>  		: "cc", "memory", "0", "1");
> +	kmsan_unpoison_memory(dest, dest_len);

[Severity: High]
Does unpoisoning the requested length (dest_len) instead of the written length
(dest_len - d.odd) mask partial completions?

When cpacf_prno() terminates early (e.g., returning Condition Code 1 due to
entropy exhaustion), the hardware only partially fills the destination buffer.
Unconditionally unpoisoning the requested length blinds KMSAN to the
uninitialized tail of the buffer, potentially hiding active kernel memory
information leaks. 

It appears the pre-existing cpacf_trng() wrapper suffers from the same
issue, unpoisoning ucbuf_len and cbuf_len unconditionally.


[Severity: Critical]
This is a pre-existing issue, but does the silent exit on Condition Code 1
in cpacf_trng() cause uninitialized kernel memory to be copied to userspace?

Looking at cpacf_trng() in arch/s390/include/asm/cpacf.h, the assembly only
loops on Condition Code 3:

	asm volatile (
		"	lghi	0,%[fc]\n"
		"0:	.insn	rre,%[opc] << 16,%[ucbuf],%[cbuf]\n"
		"	brc	1,0b"	  /* handle partial completion */
		: [ucbuf] "+&d" (u.pair), [cbuf] "+&d" (c.pair)
		: [fc] "K" (CPACF_PRNO_TRNG), [opc] "i" (CPACF_PRNO)
		: "cc", "memory", "0");

If hardware TRNG entropy is exhausted, it returns CC 1 and falls through,
discarding the unwritten byte counters (c.odd) and returning void.

Because the wrapper returns void, trng_read() in
drivers/char/hw_random/s390-trng.c falsely assumes the entire buffer allocated
with kmalloc(..., GFP_KERNEL) was filled:

	cpacf_trng(NULL, 0, p, n);
	atomic64_add(n, &trng_dev_counter);
	if (copy_to_user(ubuf, p, n)) {

Can this sequence leak uninitialized kernel heap memory to any unprivileged
local user reading from /dev/trng until entropy runs out?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1