[PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Thomas Huth]

The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be
used from problem space, too. Just the switching to the home address space
is privileged and should still generate a privilege exception. This bug is
e.g. causing programs like Java that use the "getcpu" vdso kernel function
to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).

While we're at it, also check if DAT is not enabled. In that case the
instruction is supposed to generate a special operation exception.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 target/s390x/tcg/insn-data.h.inc | 2 +-
 target/s390x/tcg/cc_helper.c     | 7 +++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
index 7e952bdfc8..54d4250c9f 100644
--- a/target/s390x/tcg/insn-data.h.inc
+++ b/target/s390x/tcg/insn-data.h.inc
@@ -1365,7 +1365,7 @@
 /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
     F(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO)
 /* SET ADDRESS SPACE CONTROL FAST */
-    F(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0, IF_PRIV)
+    C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
 /* SET CLOCK */
     F(0xb204, SCK,     S,     Z,   0, m2_64a, 0, 0, sck, 0, IF_PRIV | IF_IO)
 /* SET CLOCK COMPARATOR */
diff --git a/target/s390x/tcg/cc_helper.c b/target/s390x/tcg/cc_helper.c
index b2e8d3d9f5..b36f8cdc8b 100644
--- a/target/s390x/tcg/cc_helper.c
+++ b/target/s390x/tcg/cc_helper.c
@@ -487,6 +487,10 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
 {
     HELPER_LOG("%s: %16" PRIx64 "\n", __func__, a1);
 
+    if (!(env->psw.mask & PSW_MASK_DAT)) {
+        tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
+    }
+
     switch (a1 & 0xf00) {
     case 0x000:
         env->psw.mask &= ~PSW_MASK_ASC;
@@ -497,6 +501,9 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
         env->psw.mask |= PSW_ASC_SECONDARY;
         break;
     case 0x300:
+        if ((env->psw.mask & PSW_MASK_PSTATE) != 0) {
+            tcg_s390_program_interrupt(env, PGM_PRIVILEGED, GETPC());
+        }
         env->psw.mask &= ~PSW_MASK_ASC;
         env->psw.mask |= PSW_ASC_HOME;
         break;
-- 
2.31.1

Re: [PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Richard Henderson]

On 12/1/22 10:44, Thomas Huth wrote:
> The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be
> used from problem space, too. Just the switching to the home address space
> is privileged and should still generate a privilege exception. This bug is
> e.g. causing programs like Java that use the "getcpu" vdso kernel function
> to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).
> 
> While we're at it, also check if DAT is not enabled. In that case the
> instruction is supposed to generate a special operation exception.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---

Looks ok, as far as it goes.  We appear to be missing the check for CR0_SECONDARY, which 
is unpredictable for SACF but mandatory for SAC.

I'll give you

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

for fixing the incorrect IF_PRIV check, which by itself should be enough to fix the Java 
issue.


r~

Re: [PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Thomas Huth]

On 01/12/2022 21.51, Richard Henderson wrote:
> On 12/1/22 10:44, Thomas Huth wrote:
>> The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be
>> used from problem space, too. Just the switching to the home address space
>> is privileged and should still generate a privilege exception. This bug is
>> e.g. causing programs like Java that use the "getcpu" vdso kernel function
>> to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).
>>
>> While we're at it, also check if DAT is not enabled. In that case the
>> instruction is supposed to generate a special operation exception.
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
> 
> Looks ok, as far as it goes.  We appear to be missing the check for 
> CR0_SECONDARY, which is unpredictable for SACF but mandatory for SAC.

Yes, but if I got our sources right, we do not implement SAC yet. Looks like 
Linux does not use it, so nobody bothered to implement it yet. Since it 
should be very similar to SACF, it should be easy to add - I can try to come 
up with an additional patch for it later.

> I'll give you
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 
> for fixing the incorrect IF_PRIV check, which by itself should be enough to 
> fix the Java issue.

Thanks!

  Thomas


PS: We might have a similar bug with the MVCP and MVCS instructions ... 
seems like they could be called from userspace in certain situations, too.

Re: [PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[David Hildenbrand]

On 01.12.22 19:44, Thomas Huth wrote:
> The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it can be
> used from problem space, too. Just the switching to the home address space
> is privileged and should still generate a privilege exception. This bug is
> e.g. causing programs like Java that use the "getcpu" vdso kernel function
> to crash (see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).
> 
> While we're at it, also check if DAT is not enabled. In that case the
> instruction is supposed to generate a special operation exception.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>   target/s390x/tcg/insn-data.h.inc | 2 +-
>   target/s390x/tcg/cc_helper.c     | 7 +++++++
>   2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/target/s390x/tcg/insn-data.h.inc b/target/s390x/tcg/insn-data.h.inc
> index 7e952bdfc8..54d4250c9f 100644
> --- a/target/s390x/tcg/insn-data.h.inc
> +++ b/target/s390x/tcg/insn-data.h.inc
> @@ -1365,7 +1365,7 @@
>   /* SERVICE CALL LOGICAL PROCESSOR (PV hypercall) */
>       F(0xb220, SERVC,   RRE,   Z,   r1_o, r2_o, 0, 0, servc, 0, IF_PRIV | IF_IO)
>   /* SET ADDRESS SPACE CONTROL FAST */
> -    F(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0, IF_PRIV)
> +    C(0xb279, SACF,    S,     Z,   0, a2, 0, 0, sacf, 0)
>   /* SET CLOCK */
>       F(0xb204, SCK,     S,     Z,   0, m2_64a, 0, 0, sck, 0, IF_PRIV | IF_IO)
>   /* SET CLOCK COMPARATOR */
> diff --git a/target/s390x/tcg/cc_helper.c b/target/s390x/tcg/cc_helper.c
> index b2e8d3d9f5..b36f8cdc8b 100644
> --- a/target/s390x/tcg/cc_helper.c
> +++ b/target/s390x/tcg/cc_helper.c
> @@ -487,6 +487,10 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
>   {
>       HELPER_LOG("%s: %16" PRIx64 "\n", __func__, a1);
>   
> +    if (!(env->psw.mask & PSW_MASK_DAT)) {
> +        tcg_s390_program_interrupt(env, PGM_SPECIAL_OP, GETPC());
> +    }
> +
>       switch (a1 & 0xf00) {
>       case 0x000:
>           env->psw.mask &= ~PSW_MASK_ASC;
> @@ -497,6 +501,9 @@ void HELPER(sacf)(CPUS390XState *env, uint64_t a1)
>           env->psw.mask |= PSW_ASC_SECONDARY;
>           break;
>       case 0x300:
> +        if ((env->psw.mask & PSW_MASK_PSTATE) != 0) {
> +            tcg_s390_program_interrupt(env, PGM_PRIVILEGED, GETPC());
> +        }
>           env->psw.mask &= ~PSW_MASK_ASC;
>           env->psw.mask |= PSW_ASC_HOME;
>           break;


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction

[Ilya Leoshkevich]

On Thu, 2022-12-01 at 19:44 +0100, Thomas Huth wrote:
> The SET ADDRESS SPACE CONTROL FAST instruction is not privileged, it
> can be
> used from problem space, too. Just the switching to the home address
> space
> is privileged and should still generate a privilege exception. This
> bug is
> e.g. causing programs like Java that use the "getcpu" vdso kernel
> function
> to crash (see
> https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=990417#26 ).
> 
> While we're at it, also check if DAT is not enabled. In that case the
> instruction is supposed to generate a special operation exception.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/655
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  target/s390x/tcg/insn-data.h.inc | 2 +-
>  target/s390x/tcg/cc_helper.c     | 7 +++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)

Reviewed-by: Ilya Leoshkevich <iii@linux.ibm.com>