[PATCH] target/s390x/tcg: fix register update on guest memory fault in cpacf_aes_ecb

[PATCH] target/s390x/tcg: fix register update on guest memory fault in cpacf_aes_ecb

[Andrew Holden]

The KM instruction loop in cpacf_aes_ecb() updated the source pointer,
destination pointer, and length registers only after the entire loop
completed, using a 'done' accumulator. If a guest memory access fault
fires mid-loop, siglongjmp unwinds the C stack before the register
update is reached. The registers then reflect block 0 rather than the
block being processed, causing the instruction to restart from the
beginning on every fault. This results in quadratic reprocessing and
violates the PoP requirement that registers reflect partial completion
per block processed.

Fix this by updating the registers inside the loop body after each
successfully processed block, and removing the 'done' accumulator.

Signed-off-by: Andrew Holden <andy0x48@linux.ibm.com>
---
 target/s390x/tcg/cpacf_aes.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/target/s390x/tcg/cpacf_aes.c b/target/s390x/tcg/cpacf_aes.c
index 92884c4120..41ebc57087 100644
--- a/target/s390x/tcg/cpacf_aes.c
+++ b/target/s390x/tcg/cpacf_aes.c
@@ -62,7 +62,7 @@ int cpacf_aes_ecb(CPUS390XState *env, const int mmu_idx, uintptr_t ra,
 {
     enum { MAX_BLOCKS_PER_RUN = 8192 / AES_BLOCK_SIZE };
     uint8_t in[AES_BLOCK_SIZE], out[AES_BLOCK_SIZE];
-    uint64_t len = *src_len_reg, done = 0;
+    uint64_t len = *src_len_reg;
     int i, keysize, addr_reg_size;
     uint8_t key[32];
     AES_KEY exkey;
@@ -116,23 +116,21 @@ int cpacf_aes_ecb(CPUS390XState *env, const int mmu_idx, uintptr_t ra,
 
     /* process up to MAX_BLOCKS_PER_RUN aes blocks */
     for (i = 0; i < MAX_BLOCKS_PER_RUN && len >= AES_BLOCK_SIZE; i++) {
-        aes_read_block(env, mmu_idx, ra, *src_ptr_reg + done, in);
+        aes_read_block(env, mmu_idx, ra, *src_ptr_reg, in);
         if (mod) {
             AES_decrypt(in, out, &exkey);
         } else {
             AES_encrypt(in, out, &exkey);
         }
-        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg + done, out);
+        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg, out);
+        *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
+                                 *src_ptr_reg + AES_BLOCK_SIZE);
+        *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
+                                 *dst_ptr_reg + AES_BLOCK_SIZE);
+        *src_len_reg -= AES_BLOCK_SIZE;
         len -= AES_BLOCK_SIZE;
-        done += AES_BLOCK_SIZE;
     }
 
-    *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
-                             *src_ptr_reg + done);
-    *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
-                             *dst_ptr_reg + done);
-    *src_len_reg -= done;
-
     return !len ? 0 : 3;
 }
 
-- 
2.56.0

Re: [PATCH] target/s390x/tcg: fix register update on guest memory fault in cpacf_aes_ecb

[Harald Freudenberger]

On 2026-10-08 17:44, Andrew Holden wrote:
> The KM instruction loop in cpacf_aes_ecb() updated the source pointer,
> destination pointer, and length registers only after the entire loop
> completed, using a 'done' accumulator. If a guest memory access fault
> fires mid-loop, siglongjmp unwinds the C stack before the register
> update is reached. The registers then reflect block 0 rather than the
> block being processed, causing the instruction to restart from the
> beginning on every fault. This results in quadratic reprocessing and
> violates the PoP requirement that registers reflect partial completion
> per block processed.
> 
> Fix this by updating the registers inside the loop body after each
> successfully processed block, and removing the 'done' accumulator.
> 
> Signed-off-by: Andrew Holden <andy0x48@linux.ibm.com>
> ---
>  target/s390x/tcg/cpacf_aes.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/target/s390x/tcg/cpacf_aes.c 
> b/target/s390x/tcg/cpacf_aes.c
> index 92884c4120..41ebc57087 100644
> --- a/target/s390x/tcg/cpacf_aes.c
> +++ b/target/s390x/tcg/cpacf_aes.c
> @@ -62,7 +62,7 @@ int cpacf_aes_ecb(CPUS390XState *env, const int
> mmu_idx, uintptr_t ra,
>  {
>      enum { MAX_BLOCKS_PER_RUN = 8192 / AES_BLOCK_SIZE };
>      uint8_t in[AES_BLOCK_SIZE], out[AES_BLOCK_SIZE];
> -    uint64_t len = *src_len_reg, done = 0;
> +    uint64_t len = *src_len_reg;
>      int i, keysize, addr_reg_size;
>      uint8_t key[32];
>      AES_KEY exkey;

Try to hold this bunch of variable declarations as a
"reverse Christmas tree". I am not sure if this is a strong
coding guideline for qemu but there are a lot of open source
projects insisting on this.

> @@ -116,23 +116,21 @@ int cpacf_aes_ecb(CPUS390XState *env, const int
> mmu_idx, uintptr_t ra,
> 
>      /* process up to MAX_BLOCKS_PER_RUN aes blocks */
>      for (i = 0; i < MAX_BLOCKS_PER_RUN && len >= AES_BLOCK_SIZE; i++) 
> {
> -        aes_read_block(env, mmu_idx, ra, *src_ptr_reg + done, in);
> +        aes_read_block(env, mmu_idx, ra, *src_ptr_reg, in);
>          if (mod) {
>              AES_decrypt(in, out, &exkey);
>          } else {
>              AES_encrypt(in, out, &exkey);
>          }
> -        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg + done, out);
> +        aes_write_block(env, mmu_idx, ra, *dst_ptr_reg, out);

A comment here like
   /* update registers at end of each loop */
or so would be nice.

> +        *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
> +                                 *src_ptr_reg + AES_BLOCK_SIZE);
> +        *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
> +                                 *dst_ptr_reg + AES_BLOCK_SIZE);
> +        *src_len_reg -= AES_BLOCK_SIZE;
>          len -= AES_BLOCK_SIZE;
> -        done += AES_BLOCK_SIZE;
>      }
> 
> -    *src_ptr_reg = deposit64(*src_ptr_reg, 0, addr_reg_size,
> -                             *src_ptr_reg + done);
> -    *dst_ptr_reg = deposit64(*dst_ptr_reg, 0, addr_reg_size,
> -                             *dst_ptr_reg + done);
> -    *src_len_reg -= done;
> -
>      return !len ? 0 : 3;
>  }

Nice starting point.
Reviewed-by: Harald Freudenberger <freude@linux.ibm.com>

Re: [PATCH] target/s390x/tcg: fix register update on guest memory fault in cpacf_aes_ecb

[Andrew Holden]

On 09/10/2026 08:42, Harald Freudenberger wrote:

> Try to hold this bunch of variable declarations as a
> "reverse Christmas tree". I am not sure if this is a strong
> coding guideline for qemu but there are a lot of open source
> projects insisting on this. 
[...]
> A comment here like
>   /* update registers at end of each loop */
> or so would be nice.
Will do both in v2, thanks.

-- 
Le gach dea-ghuí,
Andrew H

IBM, Waterford IE