1. Patchew
  2. QEMU
  3. View series

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

Thomas Huth posted 1 patch 1 year, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20221201184443.136355-1-thuth@redhat.com
Maintainers: Richard Henderson <richard.henderson@linaro.org>, David Hildenbrand <david@redhat.com>, Ilya Leoshkevich <iii@linux.ibm.com>, 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(-)
[PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction
Posted by Thomas Huth 1 year, 4 months ago 
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
Posted by Ilya Leoshkevich 1 year, 4 months ago 
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>
Re: [PATCH v2 for-8.0] target/s390x/tcg: Fix and improve the SACF instruction
Posted by David Hildenbrand 1 year, 4 months ago 
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
Posted by Richard Henderson 1 year, 4 months ago 
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
Posted by Thomas Huth 1 year, 4 months ago 
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.

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.