| Message ID | 20200728195706.11087-1-richard.henderson@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | target/arm: Fix AddPAC error indication | expand |
On Tue, 28 Jul 2020 at 20:57, Richard Henderson <richard.henderson@linaro.org> wrote: > > The definition of top_bit used in this function is one higher > than that used in the Arm ARM psuedo-code, which put the error > indication at top_bit - 1 at the wrong place, which meant that > it wasn't visible to Auth. > > Fixing the definition of top_bit requires more changes, because > its most common use is for the count of bits in top_bit:bot_bit, > which would then need to be computed as top_bit - bot_bit + 1. > > For now, prefer the minimal fix to the error indication alone. > > Fixes: 63ff0ca94cb > Reported-by: Derrick McKee <derrick.mckee@gmail.com> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org> This seems like it might confuse us in future so I've added a comment inside the if(): /* * Note that our top_bit is one greater than the pseudocode's * version, hence "- 2" here. */ Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> and added to target-arm.next. thanks -- PMM
diff --git a/target/arm/pauth_helper.c b/target/arm/pauth_helper.c index b909630317..d00cd97332 100644 --- a/target/arm/pauth_helper.c +++ b/target/arm/pauth_helper.c @@ -300,7 +300,7 @@ static uint64_t pauth_addpac(CPUARMState *env, uint64_t ptr, uint64_t modifier, */ test = sextract64(ptr, bot_bit, top_bit - bot_bit); if (test != 0 && test != -1) { - pac ^= MAKE_64BIT_MASK(top_bit - 1, 1); + pac ^= MAKE_64BIT_MASK(top_bit - 2, 1); } /* diff --git a/tests/tcg/aarch64/pauth-5.c b/tests/tcg/aarch64/pauth-5.c new file mode 100644 index 0000000000..67c257918b --- /dev/null +++ b/tests/tcg/aarch64/pauth-5.c @@ -0,0 +1,33 @@ +#include <assert.h> + +static int x; + +int main() +{ + int *p0 = &x, *p1, *p2, *p3; + unsigned long salt = 0; + + /* + * With TBI enabled and a 48-bit VA, there are 7 bits of auth, and so + * a 1/128 chance of auth = pac(ptr,key,salt) producing zero. + * Find a salt that creates auth != 0. + */ + do { + salt++; + asm("pacda %0, %1" : "=r"(p1) : "r"(salt), "0"(p0)); + } while (p0 == p1); + + /* + * This pac must fail, because the input pointer bears an encryption, + * and so is not properly extended within bits [55:47]. This will + * toggle bit 54 in the output... + */ + asm("pacda %0, %1" : "=r"(p2) : "r"(salt), "0"(p1)); + + /* ... so that the aut must fail, setting bit 53 in the output ... */ + asm("autda %0, %1" : "=r"(p3) : "r"(salt), "0"(p2)); + + /* ... which means this equality must not hold. */ + assert(p3 != p0); + return 0; +} diff --git a/tests/tcg/aarch64/Makefile.target b/tests/tcg/aarch64/Makefile.target index b617f2ac7e..e7249915e7 100644 --- a/tests/tcg/aarch64/Makefile.target +++ b/tests/tcg/aarch64/Makefile.target @@ -19,7 +19,7 @@ run-fcvt: fcvt # Pauth Tests ifneq ($(DOCKER_IMAGE)$(CROSS_CC_HAS_ARMV8_3),) -AARCH64_TESTS += pauth-1 pauth-2 pauth-4 +AARCH64_TESTS += pauth-1 pauth-2 pauth-4 pauth-5 pauth-%: CFLAGS += -march=armv8.3-a run-pauth-%: QEMU_OPTS += -cpu max run-plugin-pauth-%: QEMU_OPTS += -cpu max
The definition of top_bit used in this function is one higher than that used in the Arm ARM psuedo-code, which put the error indication at top_bit - 1 at the wrong place, which meant that it wasn't visible to Auth. Fixing the definition of top_bit requires more changes, because its most common use is for the count of bits in top_bit:bot_bit, which would then need to be computed as top_bit - bot_bit + 1. For now, prefer the minimal fix to the error indication alone. Fixes: 63ff0ca94cb Reported-by: Derrick McKee <derrick.mckee@gmail.com> Signed-off-by: Richard Henderson <richard.henderson@linaro.org> --- target/arm/pauth_helper.c | 2 +- tests/tcg/aarch64/pauth-5.c | 33 +++++++++++++++++++++++++++++++ tests/tcg/aarch64/Makefile.target | 2 +- 3 files changed, 35 insertions(+), 2 deletions(-) create mode 100644 tests/tcg/aarch64/pauth-5.c -- 2.25.1