[PATCH 0/4] target/arm: Fix ComputePAC (LP 1859713)

[PATCH 0/4] target/arm: Fix ComputePAC (LP 1859713)

[Richard Henderson]

Thanks to Vincent and Adrien for both reporting the bug and
providing the solution.  I've converted their manual testing
into executable tests.


r~


Richard Henderson (3):
  tests/tcg/aarch64: Fix compilation parameters for pauth-%
  tests/tcg/aarch64: Add pauth-3
  tests/tcg/aarch64: Add pauth-4

Vincent Dehors (1):
  target/arm: Fix PAuth sbox functions

 target/arm/pauth_helper.c                 |  4 +--
 tests/tcg/aarch64/pauth-1.c               |  2 --
 tests/tcg/aarch64/pauth-2.c               |  2 --
 tests/tcg/aarch64/pauth-4.c               | 25 ++++++++++++++
 tests/tcg/aarch64/system/pauth-3.c        | 40 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  5 ++-
 tests/tcg/aarch64/Makefile.target         |  3 +-
 7 files changed, 73 insertions(+), 8 deletions(-)
 create mode 100644 tests/tcg/aarch64/pauth-4.c
 create mode 100644 tests/tcg/aarch64/system/pauth-3.c

-- 
2.20.1

[PATCH 1/4] target/arm: Fix PAuth sbox functions

[Richard Henderson]

From: Vincent Dehors <vincent.dehors@smile.fr>

In the PAC computation, sbox was applied over wrong bits.
As this is a 4-bit sbox, bit index should be incremented by 4 instead of 16.

Test vector from QARMA paper (https://eprint.iacr.org/2016/444.pdf) was
used to verify one computation of the pauth_computepac() function which
uses sbox2.

Launchpad: https://bugs.launchpad.net/bugs/1859713
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Vincent DEHORS <vincent.dehors@smile.fr>
Signed-off-by: Adrien GRASSEIN <adrien.grassein@smile.fr>
---
 target/arm/pauth_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c
index d3194f2043..0a5f41e10c 100644
--- a/target/arm/pauth_helper.c
+++ b/target/arm/pauth_helper.c
@@ -89,7 +89,7 @@ static uint64_t pac_sub(uint64_t i)
     uint64_t o = 0;
     int b;
 
-    for (b = 0; b < 64; b += 16) {
+    for (b = 0; b < 64; b += 4) {
         o |= (uint64_t)sub[(i >> b) & 0xf] << b;
     }
     return o;
@@ -104,7 +104,7 @@ static uint64_t pac_inv_sub(uint64_t i)
     uint64_t o = 0;
     int b;
 
-    for (b = 0; b < 64; b += 16) {
+    for (b = 0; b < 64; b += 4) {
         o |= (uint64_t)inv_sub[(i >> b) & 0xf] << b;
     }
     return o;
-- 
2.20.1

[PATCH 2/4] tests/tcg/aarch64: Fix compilation parameters for pauth-%

[Richard Henderson]

Hiding the required architecture within asm() is not correct.
Add it to the cflags of the (cross-) compiler.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/aarch64/pauth-1.c       | 2 --
 tests/tcg/aarch64/pauth-2.c       | 2 --
 tests/tcg/aarch64/Makefile.target | 1 +
 3 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/tests/tcg/aarch64/pauth-1.c b/tests/tcg/aarch64/pauth-1.c
index a3c1443cd0..ea0984ea82 100644
--- a/tests/tcg/aarch64/pauth-1.c
+++ b/tests/tcg/aarch64/pauth-1.c
@@ -2,8 +2,6 @@
 #include <sys/prctl.h>
 #include <stdio.h>
 
-asm(".arch armv8.4-a");
-
 #ifndef PR_PAC_RESET_KEYS
 #define PR_PAC_RESET_KEYS  54
 #define PR_PAC_APDAKEY     (1 << 2)
diff --git a/tests/tcg/aarch64/pauth-2.c b/tests/tcg/aarch64/pauth-2.c
index 2fe030ba3d..9bba0beb63 100644
--- a/tests/tcg/aarch64/pauth-2.c
+++ b/tests/tcg/aarch64/pauth-2.c
@@ -1,8 +1,6 @@
 #include <stdint.h>
 #include <assert.h>
 
-asm(".arch armv8.4-a");
-
 void do_test(uint64_t value)
 {
     uint64_t salt1, salt2;
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index df3fe8032c..374c8d6830 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -20,6 +20,7 @@ run-fcvt: fcvt
 # Pauth Tests
 AARCH64_TESTS += pauth-1 pauth-2
 run-pauth-%: QEMU_OPTS += -cpu max
+pauth-%: CFLAGS += -march=armv8.3-a
 
 # Semihosting smoke test for linux-user
 AARCH64_TESTS += semihosting
-- 
2.20.1

Re: [PATCH 2/4] tests/tcg/aarch64: Fix compilation parameters for pauth-%

[Philippe Mathieu-Daudé]

On 1/17/20 12:08 AM, Richard Henderson wrote:
> Hiding the required architecture within asm() is not correct.
> Add it to the cflags of the (cross-) compiler.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tests/tcg/aarch64/pauth-1.c       | 2 --
>   tests/tcg/aarch64/pauth-2.c       | 2 --
>   tests/tcg/aarch64/Makefile.target | 1 +
>   3 files changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/tests/tcg/aarch64/pauth-1.c b/tests/tcg/aarch64/pauth-1.c
> index a3c1443cd0..ea0984ea82 100644
> --- a/tests/tcg/aarch64/pauth-1.c
> +++ b/tests/tcg/aarch64/pauth-1.c
> @@ -2,8 +2,6 @@
>   #include <sys/prctl.h>
>   #include <stdio.h>
>   
> -asm(".arch armv8.4-a");
> -
>   #ifndef PR_PAC_RESET_KEYS
>   #define PR_PAC_RESET_KEYS  54
>   #define PR_PAC_APDAKEY     (1 << 2)
> diff --git a/tests/tcg/aarch64/pauth-2.c b/tests/tcg/aarch64/pauth-2.c
> index 2fe030ba3d..9bba0beb63 100644
> --- a/tests/tcg/aarch64/pauth-2.c
> +++ b/tests/tcg/aarch64/pauth-2.c
> @@ -1,8 +1,6 @@
>   #include <stdint.h>
>   #include <assert.h>
>   
> -asm(".arch armv8.4-a");
> -
>   void do_test(uint64_t value)
>   {
>       uint64_t salt1, salt2;
> diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
> index df3fe8032c..374c8d6830 100644
> --- a/tests/tcg/aarch64/Makefile.target
> +++ b/tests/tcg/aarch64/Makefile.target
> @@ -20,6 +20,7 @@ run-fcvt: fcvt
>   # Pauth Tests
>   AARCH64_TESTS += pauth-1 pauth-2
>   run-pauth-%: QEMU_OPTS += -cpu max
> +pauth-%: CFLAGS += -march=armv8.3-a

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>   
>   # Semihosting smoke test for linux-user
>   AARCH64_TESTS += semihosting
> 

Re: [PATCH 2/4] tests/tcg/aarch64: Fix compilation parameters for pauth-%

[Peter Maydell]

On Thu, 16 Jan 2020 at 23:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Hiding the required architecture within asm() is not correct.
> Add it to the cflags of the (cross-) compiler.

This commit message implies that we're just moving the architecture
specification from the asm statements to the cflags, but the
patch also changes the requested value from v8.4-a to v8.3-a.

I plan to tweak the commit message to be a bit clearer:

We were incorrectly requiring ARMv8.4 support for the pauth
tests, but Pointer Authentication is an ARMv8.3 extension.
Further, hiding the required architecture within asm() is
not correct.

Correct the architecture version requested, and specify it
in the cflags of the (cross-) compiler rather than in the asm.


thanks
-- PMM

[PATCH 3/4] tests/tcg/aarch64: Add pauth-3

[Richard Henderson]

This is the test vector from the QARMA paper, run through PACGA.

Suggested-by: Vincent Dehors <vincent.dehors@smile.fr>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/aarch64/system/pauth-3.c        | 40 +++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.softmmu-target |  5 ++-
 2 files changed, 44 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/system/pauth-3.c

diff --git a/tests/tcg/aarch64/system/pauth-3.c b/tests/tcg/aarch64/system/pauth-3.c
new file mode 100644
index 0000000000..42eff4d5ea
--- /dev/null
+++ b/tests/tcg/aarch64/system/pauth-3.c
@@ -0,0 +1,40 @@
+#include <inttypes.h>
+#include <minilib.h>
+
+int main()
+{
+    /*
+     * Test vector from QARMA paper (https://eprint.iacr.org/2016/444.pdf)
+     * to verify one computation of the pauth_computepac() function,
+     * which uses sbox2.
+     *
+     * Use PACGA, because it returns the most bits from ComputePAC.
+     * We still only get the most significant 32-bits of the result.
+     */
+
+    static const uint64_t d[5] = {
+        0xfb623599da6e8127ull,
+        0x477d469dec0b8762ull,
+        0x84be85ce9804e94bull,
+        0xec2802d4e0a488e9ull,
+        0xc003b93999b33765ull & 0xffffffff00000000ull
+    };
+    uint64_t r;
+
+    asm("msr apgakeyhi_el1, %[w0]\n\t"
+        "msr apgakeylo_el1, %[k0]\n\t"
+        "pacga %[r], %[P], %[T]"
+        : [r] "=r"(r)
+        : [P] "r" (d[0]),
+          [T] "r" (d[1]),
+          [w0] "r" (d[2]),
+          [k0] "r" (d[3]));
+
+    if (r == d[4]) {
+        ml_printf("OK\n");
+        return 0;
+    } else {
+        ml_printf("FAIL: %lx != %lx\n", r, d[4]);
+        return 1;
+    }
+}
diff --git a/tests/tcg/aarch64/Makefile.softmmu-target b/tests/tcg/aarch64/Makefile.softmmu-target
index 7b4eede3f0..f6b5121f5c 100644
--- a/tests/tcg/aarch64/Makefile.softmmu-target
+++ b/tests/tcg/aarch64/Makefile.softmmu-target
@@ -61,4 +61,7 @@ run-memory-replay: memory-replay run-memory-record
 	   	  $(QEMU_OPTS) memory, \
 	  "$< on $(TARGET_NAME)")
 
-EXTRA_TESTS+=memory-record memory-replay
+run-pauth-3: pauth-3
+pauth-3: CFLAGS += -march=armv8.3-a
+
+EXTRA_TESTS+=memory-record memory-replay pauth-3
-- 
2.20.1

[PATCH 4/4] tests/tcg/aarch64: Add pauth-4

[Richard Henderson]

Perform the set of operations and test described in LP 1859713.

Suggested-by: Adrien GRASSEIN <adrien.grassein@smile.fr>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tests/tcg/aarch64/pauth-4.c       | 25 +++++++++++++++++++++++++
 tests/tcg/aarch64/Makefile.target |  2 +-
 2 files changed, 26 insertions(+), 1 deletion(-)
 create mode 100644 tests/tcg/aarch64/pauth-4.c

diff --git a/tests/tcg/aarch64/pauth-4.c b/tests/tcg/aarch64/pauth-4.c
new file mode 100644
index 0000000000..4b22e6e282
--- /dev/null
+++ b/tests/tcg/aarch64/pauth-4.c
@@ -0,0 +1,25 @@
+#include <stdint.h>
+#include <assert.h>
+
+int main()
+{
+  uintptr_t x, y;
+
+  asm("mov %0, lr\n\t"
+      "pacia %0, sp\n\t"	/* sigill if pauth not supported */
+      "eor %0, %0, #4\n\t"	/* corrupt single bit */
+      "mov %1, %0\n\t"
+      "autia %1, sp\n\t"	/* validate corrupted pointer */
+      "xpaci %0\n\t"		/* strip pac from corrupted pointer */
+      : "=r"(x), "=r"(y));
+
+  /*
+   * Once stripped, the corrupted pointer is of the form 0x0000...wxyz.
+   * We expect the autia to indicate failure, producing a pointer of the
+   * form 0x000e....wxyz.  Use xpaci and != for the test, rather than
+   * extracting explicit bits from the top, because the location of the
+   * error code "e" depends on the configuration of virtual memory.
+   */
+  assert(x != y);
+  return 0;
+}
diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target
index 374c8d6830..efa67cf1e9 100644
--- a/tests/tcg/aarch64/Makefile.target
+++ b/tests/tcg/aarch64/Makefile.target
@@ -18,7 +18,7 @@ run-fcvt: fcvt
 	$(call diff-out,$<,$(AARCH64_SRC)/fcvt.ref)
 
 # Pauth Tests
-AARCH64_TESTS += pauth-1 pauth-2
+AARCH64_TESTS += pauth-1 pauth-2 pauth-4
 run-pauth-%: QEMU_OPTS += -cpu max
 pauth-%: CFLAGS += -march=armv8.3-a
 
-- 
2.20.1

Re: [PATCH 0/4] target/arm: Fix ComputePAC (LP 1859713)

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200116230809.19078-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20200116230809.19078-1-richard.henderson@linaro.org
Type: series
Subject: [PATCH 0/4] target/arm: Fix ComputePAC (LP 1859713)

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git rev-parse base > /dev/null || exit 0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 - [tag update]      patchew/20200117111147.5006-1-cohuck@redhat.com -> patchew/20200117111147.5006-1-cohuck@redhat.com
Switched to a new branch 'test'
88edbfe tests/tcg/aarch64: Add pauth-4
f826e15 tests/tcg/aarch64: Add pauth-3
6c82a5a tests/tcg/aarch64: Fix compilation parameters for pauth-%
d01fc4a target/arm: Fix PAuth sbox functions

=== OUTPUT BEGIN ===
1/4 Checking commit d01fc4adb51f (target/arm: Fix PAuth sbox functions)
2/4 Checking commit 6c82a5aa6b0f (tests/tcg/aarch64: Fix compilation parameters for pauth-%)
3/4 Checking commit f826e1549b92 (tests/tcg/aarch64: Add pauth-3)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#27: 
new file mode 100644

total: 0 errors, 1 warnings, 48 lines checked

Patch 3/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
4/4 Checking commit 88edbfe8c3a8 (tests/tcg/aarch64: Add pauth-4)
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#27: 
new file mode 100644

ERROR: code indent should never use tabs
#40: FILE: tests/tcg/aarch64/pauth-4.c:9:
+      "pacia %0, sp\n\t"^I/* sigill if pauth not supported */$

ERROR: code indent should never use tabs
#41: FILE: tests/tcg/aarch64/pauth-4.c:10:
+      "eor %0, %0, #4\n\t"^I/* corrupt single bit */$

ERROR: code indent should never use tabs
#43: FILE: tests/tcg/aarch64/pauth-4.c:12:
+      "autia %1, sp\n\t"^I/* validate corrupted pointer */$

ERROR: code indent should never use tabs
#44: FILE: tests/tcg/aarch64/pauth-4.c:13:
+      "xpaci %0\n\t"^I^I/* strip pac from corrupted pointer */$

total: 4 errors, 1 warnings, 33 lines checked

Patch 4/4 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20200116230809.19078-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/4] target/arm: Fix ComputePAC (LP 1859713)

[Peter Maydell]

On Thu, 16 Jan 2020 at 23:08, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> Thanks to Vincent and Adrien for both reporting the bug and
> providing the solution.  I've converted their manual testing
> into executable tests.



Applied to target-arm.next, thanks (with the commit message
for patch 2 tweaked, and the hard-coded tabs in patch 4
changed to spaces).

-- PMM