Hi Amit

On 03/09/2019 10:26, Amit Kachhap wrote:

Hi Cristian,

On 9/2/19 4:59 PM, Cristian Marussi wrote:

...

Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build framework; keep these new arm64 KSFT testcases separated into distinct subdirs inside tools/testing/selftests/arm64/ depending on the specific subsystem targeted.

Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Merge with

commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

while moving such KSFT tags tests inside their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile

This approach breaks the compilation of individual test cases which need to export includes individually.

make -C tools/testing/selftests/arm64/signal

../../lib.mk:25: ../../../../scripts/subarch.include: No such file or directory Makefile:25: warning: overriding recipe for target 'clean' ../../lib.mk:123: warning: ignoring old recipe for target 'clean' make: *** No rule to make target '../../../../scripts/subarch.include'. Stop.

Having removed standalone mode in signal I was not expecting to be able to run distinct arm64 subsystems as before with:

$ make -C tools/testing/selftests/arm64/signal (which was not a standard KSFT thing)

but only using standard targets way like:

$ make -C tools/testing/selftests TARGETS=arm64

or

$ make -C tools/testing/selftests TARGETS=arm64 \ INSTALL_PATH=<your-installation-path> install

or

$ make TARGETS=arm64 kselftest

which runs all tests inside arm64 as a whole. (I should have changed arm64 README accordingly)

However tags test works well, make -C tools/testing/selftests/arm64/tags

aarch64-none-linux-gnu-gcc tags_test.c -o /home/amikac01/work/MTE_WORK/linux-server/linux/tools/testing/selftests/arm64/tags/tags_test

But I'll have a look why tags expose different behavior.

Thanks, Amit Daniel

Thanks

Cristian

...

• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 70 +++++++++++++++++-- tools/testing/selftests/arm64/README | 20 ++++++ tools/testing/selftests/arm64/tags/Makefile | 10 +++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..5dbb0ffdfc9a 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,69 @@ # SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 ARM Limited -# ARCH can be overridden by the user for cross compiling -ARCH ?= $(shell uname -m 2>/dev/null || echo not) +# When ARCH not overridden for crosscompiling, lookup machine +ARCH ?= $(shell uname -m) +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/) -ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +ifeq ("x$(ARCH)", "xarm64") +SUBDIRS := tags +else +SUBDIRS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = ../../../../..

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

+CFLAGS += -I$(khdr_dir)

+export CC +export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBDIRS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• mkdir -p $$BUILD_TARGET;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+install: all

• @for DIR in $(SUBDIRS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+run_tests: all

• @for DIR in $(SUBDIRS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+# Avoid any output on non arm64 on emit_tests +emit_tests: all

• @for DIR in $(SUBDIRS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+clean:

• @for DIR in $(SUBDIRS); do \

• BUILD_TARGET=$(OUTPUT)/$$DIR;			\
  

• make OUTPUT=$$BUILD_TARGET -C $$DIR $@;		\
  

• done

+.PHONY: all clean install run_tests emit_tests diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README new file mode 100644 index 000000000000..aca892e62a6c --- /dev/null +++ b/tools/testing/selftests/arm64/README @@ -0,0 +1,20 @@ +KSelfTest ARM64 +===============

+- These tests are arm64 specific and so not built or run but just skipped

• completely when env-variable ARCH is found to be different than 'arm64'
• and `uname -m` reports other than 'aarch64'.

+- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest

• framework using standard Linux top-level-makefile targets:

•  $ make TARGETS=arm64 kselftest-clean
  

•  $ make TARGETS=arm64 kselftest
  

•  or
  

•  $ make -C tools/testing/selftests TARGETS=arm64 \
  

• INSTALL_PATH=<your-installation-path> install
  

• Further details on building and running KFST can be found in:

• Documentation/dev-tools/kselftest.rst
  

diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile new file mode 100644 index 000000000000..76205533135b --- /dev/null +++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +# ARCH can be overridden by the user for cross compiling +ARCH ?= $(shell uname -m 2>/dev/null || echo not)

+ifneq (,$(filter $(ARCH),aarch64 arm64)) +TEST_GEN_PROGS := tags_test +TEST_PROGS := run_tags_test.sh +endif

+include ../../lib.mk diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/tags/run_tags_test.sh similarity index 100% rename from tools/testing/selftests/arm64/run_tags_test.sh rename to tools/testing/selftests/arm64/tags/run_tags_test.sh diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags/tags_test.c similarity index 100% rename from tools/testing/selftests/arm64/tags_test.c rename to tools/testing/selftests/arm64/tags/tags_test.c

Hi,

On 9/5/19 11:27 PM, Cristian Marussi wrote:

Hi Amit

On 03/09/2019 10:26, Amit Kachhap wrote:

...

Hi Cristian,

On 9/2/19 4:59 PM, Cristian Marussi wrote:

...

Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build framework; keep these new arm64 KSFT testcases separated into distinct subdirs inside tools/testing/selftests/arm64/ depending on the specific subsystem targeted.

Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

Merge with

commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

while moving such KSFT tags tests inside their own subdirectory (arm64/tags).

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile

This approach breaks the compilation of individual test cases which need to export includes individually.

make -C tools/testing/selftests/arm64/signal

../../lib.mk:25: ../../../../scripts/subarch.include: No such file or directory Makefile:25: warning: overriding recipe for target 'clean' ../../lib.mk:123: warning: ignoring old recipe for target 'clean' make: *** No rule to make target '../../../../scripts/subarch.include'. Stop.

However tags test works well, make -C tools/testing/selftests/arm64/tags

aarch64-none-linux-gnu-gcc tags_test.c -o /home/amikac01/work/MTE_WORK/linux-server/linux/tools/testing/selftests/arm64/tags/tags_test

Thanks, Amit Daniel

So at the end I think I'll opt for the following in V6 regarding the issue of being able to build specific KSFT arm64 subsystems while properly searching kernel headers (and keeping compatible with the KSFT framework completely):

• only arm64 toplevel KSFT Makefile searches for the kernel headers location for all and propagates down the info

• you can also now optionally specify which arm64 subsystem to build (to avoid have to build, say, all of signal/ if you are not interested into....a sort of standalone mode without all the burden of the old standalone mode)

ok.

So you can issue:

$ make TARGETS=arm64 kselftest

or similarly:

$ make -C tools/testing/selftests TARGETS=arm64 \ INSTALL_PATH=<your-installation-path> install

or select subsystems:

$ make -C tools/testing/selftests TARGETS=arm64 SUBTARGETS="tags signal" \ INSTALL_PATH=<your-installation-path> install

This option will be useful as it is better to compile just one subtarget in development phase.

Thanks, Amit

with all of the above looking for the K headers in the proper place and without duplicating the search code in multiple places. (bugs apart :D)

Thanks

Cristian

...

...

• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

tools/testing/selftests/Makefile | 1 + tools/testing/selftests/arm64/Makefile | 70 +++++++++++++++++-- tools/testing/selftests/arm64/README | 20 ++++++ tools/testing/selftests/arm64/tags/Makefile | 10 +++ .../arm64/{ => tags}/run_tags_test.sh | 0 .../selftests/arm64/{ => tags}/tags_test.c | 0 6 files changed, 95 insertions(+), 6 deletions(-) create mode 100644 tools/testing/selftests/arm64/README create mode 100644 tools/testing/selftests/arm64/tags/Makefile rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%) rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)

diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile index 25b43a8c2b15..1722dae9381a 100644 --- a/tools/testing/selftests/Makefile +++ b/tools/testing/selftests/Makefile @@ -1,5 +1,6 @@ # SPDX-License-Identifier: GPL-2.0 TARGETS = android +TARGETS += arm64 TARGETS += bpf TARGETS += breakpoints TARGETS += capabilities diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..5dbb0ffdfc9a 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,69 @@ # SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 ARM Limited

-# ARCH can be overridden by the user for cross compiling -ARCH ?= $(shell uname -m 2>/dev/null || echo not) +# When ARCH not overridden for crosscompiling, lookup machine +ARCH ?= $(shell uname -m) +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/)

-ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +ifeq ("x$(ARCH)", "xarm64") +SUBDIRS := tags +else +SUBDIRS := endif

-include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = ../../../../..

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

+CFLAGS += -I$(khdr_dir)

+export CC +export CFLAGS +export top_srcdir

+all:

• @for DIR in $(SUBDIRS); do \

•       BUILD_TARGET=$(OUTPUT)/$$DIR;                   \
  

•       mkdir -p $$BUILD_TARGET;                        \
  

•       make OUTPUT=$$BUILD_TARGET -C $$DIR $@;         \
  

• done

+install: all

• @for DIR in $(SUBDIRS); do \

•       BUILD_TARGET=$(OUTPUT)/$$DIR;                   \
  

•       make OUTPUT=$$BUILD_TARGET -C $$DIR $@;         \
  

• done

+run_tests: all

• @for DIR in $(SUBDIRS); do \

•       BUILD_TARGET=$(OUTPUT)/$$DIR;                   \
  

•       make OUTPUT=$$BUILD_TARGET -C $$DIR $@;         \
  

• done

+# Avoid any output on non arm64 on emit_tests +emit_tests: all

• @for DIR in $(SUBDIRS); do \

•       BUILD_TARGET=$(OUTPUT)/$$DIR;                   \
  

•       make OUTPUT=$$BUILD_TARGET -C $$DIR $@;         \
  

• done

+clean:

• @for DIR in $(SUBDIRS); do \

•       BUILD_TARGET=$(OUTPUT)/$$DIR;                   \
  

•       make OUTPUT=$$BUILD_TARGET -C $$DIR $@;         \
  

• done

+.PHONY: all clean install run_tests emit_tests diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README new file mode 100644 index 000000000000..aca892e62a6c --- /dev/null +++ b/tools/testing/selftests/arm64/README @@ -0,0 +1,20 @@ +KSelfTest ARM64 +===============

+- These tests are arm64 specific and so not built or run but just skipped

• completely when env-variable ARCH is found to be different than 'arm64'
• and `uname -m` reports other than 'aarch64'.

+- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest

• framework using standard Linux top-level-makefile targets:

•  $ make TARGETS=arm64 kselftest-clean
  

•  $ make TARGETS=arm64 kselftest
  

•  or
  

•  $ make -C tools/testing/selftests TARGETS=arm64 \
  

•       INSTALL_PATH=<your-installation-path> install
  

• Further details on building and running KFST can be found in:

• Documentation/dev-tools/kselftest.rst
  

diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile new file mode 100644 index 000000000000..76205533135b --- /dev/null +++ b/tools/testing/selftests/arm64/tags/Makefile @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0 +# ARCH can be overridden by the user for cross compiling +ARCH ?= $(shell uname -m 2>/dev/null || echo not)

+ifneq (,$(filter $(ARCH),aarch64 arm64)) +TEST_GEN_PROGS := tags_test +TEST_PROGS := run_tags_test.sh +endif

+include ../../lib.mk diff --git a/tools/testing/selftests/arm64/run_tags_test.sh b/tools/testing/selftests/arm64/tags/run_tags_test.sh similarity index 100% rename from tools/testing/selftests/arm64/run_tags_test.sh rename to tools/testing/selftests/arm64/tags/run_tags_test.sh diff --git a/tools/testing/selftests/arm64/tags_test.c b/tools/testing/selftests/arm64/tags/tags_test.c similarity index 100% rename from tools/testing/selftests/arm64/tags_test.c rename to tools/testing/selftests/arm64/tags/tags_test.c

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

On Mon, Sep 02, 2019 at 12:29:22pm +0100, Cristian Marussi wrote:

Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build framework; keep these new arm64 KSFT testcases separated into distinct

Nit: this isn't true any more, since the tags tests already added the arm64 subsystem here.

subdirs inside tools/testing/selftests/arm64/ depending on the specific subsystem targeted.

Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

This:

Merge with

commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

while moving such KSFT tags tests inside their own subdirectory (arm64/tags).

...could be put under the tearoff, but it doesn't really belong in the commit message IMHO.

I suggest rewriting the commit message to reflect the current situation (but it can be kept brief).

Basically, what this patch now seems to do is to prepare for adding more arm64 tests, by moving the tags tests into their own subdirectory and extending the existing skeleton Makefile as appropriate.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

[...]

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..5dbb0ffdfc9a 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,69 @@ # SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 ARM Limited

Change of copyright? This isn't pure Arm IP upstream IIUC.

Maybe just drop it: Makefiles don't usually contain significant IP, so many have no copyright message anyway.

-# ARCH can be overridden by the user for cross compiling -ARCH ?= $(shell uname -m 2>/dev/null || echo not) +# When ARCH not overridden for crosscompiling, lookup machine +ARCH ?= $(shell uname -m) +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/) -ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +ifeq ("x$(ARCH)", "xarm64") +SUBDIRS := tags +else +SUBDIRS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = ../../../../..

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

Looking at this, can we just pass the directory in from the toplevel "all" rule instead of guessing?

Maybe don't churn this for now though. It's something that could be looked at later.

[...]

Apart from the comments above, the patch looks reasonable to me.

Cheers ---Dave

On Thu, Sep 05, 2019 at 02:45:39PM +0100, Cristian Marussi wrote:

On 04/09/2019 12:47, Dave Martin wrote:

...

On Mon, Sep 02, 2019 at 12:29:22pm +0100, Cristian Marussi wrote:

...

Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build framework; keep these new arm64 KSFT testcases separated into distinct

Nit: this isn't true any more, since the tags tests already added the arm64 subsystem here.

Ok

...

...

subdirs inside tools/testing/selftests/arm64/ depending on the specific subsystem targeted.

Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective location of Kernel headers as installed by KSFT framework.

This:

...

Merge with

commit 9ce1263033cd ("selftests, arm64: add a selftest for passing tagged pointers to kernel")

while moving such KSFT tags tests inside their own subdirectory (arm64/tags).

...could be put under the tearoff, but it doesn't really belong in the commit message IMHO.

I suggest rewriting the commit message to reflect the current situation (but it can be kept brief).

Basically, what this patch now seems to do is to prepare for adding more arm64 tests, by moving the tags tests into their own subdirectory and extending the existing skeleton Makefile as appropriate.

Ok

...

...

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• rebased on arm64/for-next/core
• merged this patch with KSFT arm64 tags patch, while moving the latter into its own subdir
• moved kernel header includes search mechanism from KSFT arm64 SIGNAL Makefile
• export proper top_srcdir ENV for lib.mk

v3 --> v4

• comment reword
• simplified documentation in README
• dropped README about standalone

---

[...]

...

diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile index a61b2e743e99..5dbb0ffdfc9a 100644 --- a/tools/testing/selftests/arm64/Makefile +++ b/tools/testing/selftests/arm64/Makefile @@ -1,11 +1,69 @@ # SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 ARM Limited

Change of copyright? This isn't pure Arm IP upstream IIUC.

Maybe just drop it: Makefiles don't usually contain significant IP, so many have no copyright message anyway.

Right. I'll drop.

...

...

-# ARCH can be overridden by the user for cross compiling -ARCH ?= $(shell uname -m 2>/dev/null || echo not) +# When ARCH not overridden for crosscompiling, lookup machine +ARCH ?= $(shell uname -m) +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/) -ifneq (,$(filter $(ARCH),aarch64 arm64)) -TEST_GEN_PROGS := tags_test -TEST_PROGS := run_tags_test.sh +ifeq ("x$(ARCH)", "xarm64") +SUBDIRS := tags +else +SUBDIRS := endif -include ../lib.mk +CFLAGS := -Wall -O2 -g

+# A proper top_srcdir is needed by KSFT(lib.mk) +top_srcdir = ../../../../..

+# Additional include paths needed by kselftest.h and local headers +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/

+# Guessing where the Kernel headers could have been installed +# depending on ENV config +ifeq ($(KBUILD_OUTPUT),) +khdr_dir = $(top_srcdir)/usr/include +else +# the KSFT preferred location when KBUILD_OUTPUT is set +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include +endif

Looking at this, can we just pass the directory in from the toplevel "all" rule instead of guessing?

Do you mean toplevel in KSFT ? I think it's how should be done at the end, but I was trying to keep this series on arm64/ lands only. (also maybe I'm missing something obvious in KSFT handling of this situation....even though many other KSFT use built CFLAGS like: -I../../../usr/include or similar)

...

Maybe don't churn this for now though. It's something that could be looked at later.

Ok. I'll leave here and fix it to avoid relative paths...which could be problematic when exported to lower level Makefiles.

Cheers

Cristian

Sounds reasonable

Cheers ---Dave

^Nit: "add one testcase" doesn't really describe what is being added here.

Maybe the following would work as the subject line:

--8<-- kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils -->8--

The remainder of the commit message looks fine.

On Mon, Sep 02, 2019 at 12:29:23pm +0100, Cristian Marussi wrote:

Add some arm64/signal specific boilerplate and utility code to help further testcases' development.

Introduce also one simple testcase mangle_pstate_invalid_compat_toggle and some related helpers: it is a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to toggle PSTATE state bits to switch the system between 32bit/64bit execution state. Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• moved kernel headers include search to top level KSFT arm64 Makefile
• removed warning about kernel headers not found
• moved testcases/.gitignore up one level

v3 --> v4

• removed standalone mode
• fixed arm64/signal/README
• add file level comments: test layout / test description
• reduced verbosity
• removed spurious headers includes
• reviewed ID_AA64MMFR[1,2]_EL1 macros
• removed unused feats_ok
• simplified CPU features gathering
• reviewed included headers
• fixed/refactored get_header() and validation routines
• added test description

---

[...]

diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c new file mode 100644 index 000000000000..f05c6dbf8659 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/test_signals.c @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Generic test wrapper for arm64 signal tests.
• • Each test provides its own tde struct tddescr descriptor to link with

Typo? tdescr

[...]

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c new file mode 100644 index 000000000000..e2a5f37e6ad3 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2019 ARM Limited */

+#include <stdio.h> +#include <stdlib.h> +#include <signal.h> +#include <string.h> +#include <unistd.h> +#include <assert.h> +#include <sys/auxv.h> +#include <linux/auxvec.h> +#include <ucontext.h>

+#include "test_signals.h" +#include "test_signals_utils.h" +#include "testcases/testcases.h"

+extern struct tdescr *current;

+static char *feats_store[FMAX_END] = {

Nit: can we call this feat_names[]?

"store" makes me think of loads and stores...

Also, nit: can this be static const char *const []?

String literals are immutable anyway, and I guess we don't intend too modify the pointers to the strings either...

• " SSBS ",
• " PAN ",
• " UAO ",

+};

+#define MAX_FEATS_SZ 128 +static char feats_string[MAX_FEATS_SZ];

+static inline char *feats_to_string(unsigned long feats) +{

• size_t flen = MAX_FEATS_SZ - 1;
• for (int i = 0; i < FMAX_END; i++) {

• if (feats & 1UL << i) {
  

Nit: maybe have () around (1UL << i), though I think it makes no difference.

• 	size_t tlen = strlen(feats_store[i]);
  

• 	assert(flen > tlen);
  

• 	flen -= tlen;
  

• 	strncat(feats_string, feats_store[i], flen);
  

• }
  

• }
• return feats_string;

+}

+static void unblock_signal(int signum) +{

• sigset_t sset;
• sigemptyset(&sset);
• sigaddset(&sset, signum);
• sigprocmask(SIG_UNBLOCK, &sset, NULL);

+}

+static void default_result(struct tdescr *td, bool force_exit) +{

• if (td->pass)

• fprintf(stderr, "==>> completed. PASS(1)\n");
  

• else

• fprintf(stdout, "==>> completed. FAIL(0)\n");
  

• if (force_exit)

• exit(td->pass ? EXIT_SUCCESS : EXIT_FAILURE);
  

+}

+static inline bool are_feats_ok(struct tdescr *td) +{

• return (td->feats_required & td->feats_supported) == td->feats_required;

+}

+static void default_handler(int signum, siginfo_t *si, void *uc) +{

• if (current->sig_trig && signum == current->sig_trig) {

(Thinking about it, signum is never 0 because there is no signal 0. So we could write if (signum == current->sig_trig). But I think your code makes the intention clearer -- so no need to change it.)

• fprintf(stderr, "Handling SIG_TRIG\n");
  

• current->triggered = 1;
  

• /* ->run was asserted NON-NULL in test_setup() already */
  

• current->run(current, si, uc);
  

• } else if (signum == SIGILL && !current->initialized) {

• /*
  

•  * A SIGILL here while still not initialized means we failed
  

•  * even to asses the existence of features during init
  

•  */
  

• fprintf(stdout,
  

• 	"Got SIGILL test_init. Marking ALL features UNSUPPORTED.\n");
  

• current->feats_supported = 0;
  

• } else if (current->sig_ok && signum == current->sig_ok) {

• /*
  

•  * it's a bug in the test code when this assert fail:
  

•  * if a sig_trig was defined, it must have been used before
  

•  * arriving here.
  

•  */
  

• assert(!current->sig_trig || current->triggered);
  

• fprintf(stderr,
  

• 	"SIG_OK -- SP:0x%llX  si_addr@:%p  si_code:%d  token@:%p  offset:%ld\n",
  

• 	((ucontext_t *)uc)->uc_mcontext.sp,
  

• 	si->si_addr, si->si_code, current->token,
  

• 	current->token - si->si_addr);
  

• /*
  

•  * fake_sigreturn tests, which have sanity_enabled=1, set, at
  

•  * the very last time, the token field to the SP address used
  

•  * to place the fake sigframe: so token==0 means we never made
  

•  * it to the end, segfaulting well-before, and the test is
  

•  * possibly broken.
  

•  */
  

• if (!current->sanity_disabled && !current->token) {
  

• 	fprintf(stdout,
  

• 		"current->token ZEROED...test is probably broken!\n");
  

• 	abort();
  

• }
  

• /*
  

•  * Trying to narrow down the SEGV to the ones generated by
  

•  * Kernel itself via arm64_notify_segfault().
  

•  * This is a best-effort check anyway, and the si_code check may
  

•  * need to change if this aspect of the kernel ABI changes.
  

•  */
  

• if (current->sig_ok == SIGSEGV && si->si_code != SEGV_ACCERR) {
  

• 	fprintf(stdout,
  

• 		"si_code != SEGV_ACCERR...test is probably broken!\n");
  

• 	abort();
  

• }
  

• fprintf(stderr, "Handling SIG_OK\n");
  

• current->pass = 1;
  

• /*
  

•  * Some tests can lead to SEGV loops: in such a case we want
  

•  * to terminate immediately exiting straight away
  

•  */
  

• default_result(current, 1);
  

• } else {

• if (signum == current->sig_unsupp && !are_feats_ok(current)) {
  

• 	fprintf(stderr,
  

• 		"-- RX SIG_UNSUPP on unsupported feat...OK\n");
  

• 	current->pass = 1;
  

• } else if (signum == SIGALRM && current->timeout) {
  

• 	fprintf(stderr, "-- Timeout !\n");
  

• } else {
  

• 	fprintf(stderr,
  

• 		"-- RX UNEXPECTED SIGNAL: %d\n", signum);
  

• }
  

• default_result(current, 1);
  

• }

+}

+static int default_setup(struct tdescr *td) +{

• struct sigaction sa;
• sa.sa_sigaction = default_handler;
• sa.sa_flags = SA_SIGINFO | SA_RESTART;
• sa.sa_flags |= td->sa_flags;
• sigemptyset(&sa.sa_mask);
• /* uncatchable signals naturally skipped ... */
• for (int sig = 1; sig < 32; sig++)

• sigaction(sig, &sa, NULL);
  

• /*

• * RT Signals default disposition is Term but they cannot be
  

• * generated by the Kernel in response to our tests; so just catch
  

• * them all and report them as UNEXPECTED signals.
  

• */
  

• for (int sig = SIGRTMIN; sig <= SIGRTMAX; sig++)

• sigaction(sig, &sa, NULL);
  

• /* just in case...unblock explicitly all we need */
• if (td->sig_trig)

• unblock_signal(td->sig_trig);
  

• if (td->sig_ok)

• unblock_signal(td->sig_ok);
  

• if (td->sig_unsupp)

• unblock_signal(td->sig_unsupp);
  

• if (td->timeout) {

• unblock_signal(SIGALRM);
  

• alarm(td->timeout);
  

• }
• fprintf(stderr, "Registered handlers for all signals.\n");
• return 1;

+}

+static inline int default_trigger(struct tdescr *td) +{

• return !raise(td->sig_trig);

+}

+static int test_init(struct tdescr *td) +{

• td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
• if (!td->minsigstksz)

• td->minsigstksz = MINSIGSTKSZ;
  

• fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
• if (td->feats_required) {

• bool feats_ok = false;
  

• td->feats_supported = 0;
  

• /*
  

•  * Checking for CPU required features using both the
  

•  * auxval and the arm64 MRS Emulation to read sysregs.
  

•  */
  

• if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
  

• 	uint64_t val = 0;
  

• 	/* Uses HWCAP to check capability */
  

• 	if (getauxval(AT_HWCAP) & HWCAP_SSBS)
  

• 		td->feats_supported |= FEAT_SSBS;
  

Should this be outside the HWCAP_CPUID check?

It's only the get_regval(SYS_ID_foo) based checks that depend on HWCAP_CPUID.

• 	/* Uses MRS emulation to check capability */
  

• 	get_regval(SYS_ID_AA64MMFR1_EL1, val);
  

• 	if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
  

• 		td->feats_supported |= FEAT_PAN;
  

• 	/* Uses MRS emulation to check capability */
  

• 	get_regval(SYS_ID_AA64MMFR2_EL1, val);
  

• 	if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
  

• 		td->feats_supported |= FEAT_UAO;
  

• } else {
  

• 	fprintf(stderr,
  

• 		"HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
  

• }
  

• feats_ok = are_feats_ok(td);
  

• fprintf(stderr,
  

• 	"Required Features: [%s] %ssupported\n",
  

• 	feats_ok ? feats_to_string(td->feats_supported) :
  

• 	feats_to_string(td->feats_required ^
  

• 			td->feats_supported),
  

Should this be something like: td->feats_required & ~td->feats_supported ?

Otherwise we'll include features that are supported but not required, when printing the features that are NOT supported.

Alternatively, we could just print out the required and supported sets separately and leave it up to the user to obverse how they are different.

(Watch out for calling feats_to_string() twice in the same printf() call though.)

• 	!feats_ok ? "NOT " : "");
  

• }
• td->initialized = 1;
• return 1;

+}

+int test_setup(struct tdescr *td) +{

• /* assert core invariants symptom of a rotten testcase */
• assert(current);
• assert(td);
• assert(td->name);
• assert(td->run);
• if (!test_init(td))

• return 0;
  

• if (td->setup)

• return td->setup(td);
  

• else

• return default_setup(td);
  

+}

+int test_run(struct tdescr *td) +{

• if (td->sig_trig) {

• if (td->trigger)
  

• 	return td->trigger(td);
  

• else
  

• 	return default_trigger(td);
  

• } else {

• return td->run(td, NULL, NULL);
  

• }

+}

+void test_result(struct tdescr *td) +{

• if (td->check_result)

• td->check_result(td);
  

• default_result(td, 0);

+}

+void test_cleanup(struct tdescr *td) +{

• if (td->cleanup)

• td->cleanup(td);
  

+} diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h new file mode 100644 index 000000000000..8658d1a7d4b9 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h @@ -0,0 +1,13 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2019 ARM Limited */

+#ifndef __TEST_SIGNALS_UTILS_H__ +#define __TEST_SIGNALS_UTILS_H__

+#include "test_signals.h"

+int test_setup(struct tdescr *td); +void test_cleanup(struct tdescr *td); +int test_run(struct tdescr *td); +void test_result(struct tdescr *td); +#endif diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c new file mode 100644 index 000000000000..2cb118b0ba05 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_compat_toggle.c @@ -0,0 +1,31 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the execution state bit: this attempt must be spotted by Kernel and
• • the test case is expected to be terminated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,

• 		     ucontext_t *uc)
  

+{

• ASSERT_GOOD_CONTEXT(uc);
• /* This config should trigger a SIGSEGV by Kernel */
• uc->uc_mcontext.pstate ^= PSR_MODE32_BIT;
• return 1;

+}

+struct tdescr tde = {

• .sanity_disabled = true,
  

• .name = "MANGLE_PSTATE_INVALID_STATE_TOGGLE",
  

• .descr = "Mangling uc_mcontext with INVALID STATE_TOGGLE",
  

• .sig_trig = SIGUSR1,
  

• .sig_ok = SIGSEGV,
  

• .run = mangle_invalid_pstate_run,
  

+}; diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c new file mode 100644 index 000000000000..72e3f482b177 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2019 ARM Limited */ +#include "testcases.h"

+struct _aarch64_ctx *get_header(struct _aarch64_ctx *head, uint32_t magic,

• 		size_t resv_sz, size_t *offset)
  

+{

• size_t offs = 0;
• struct _aarch64_ctx *found = NULL;
• if (!head)

• return found;
  

I suggest you also check for resv_sz < HDR_SZ, since the while() condition assumes that resv_sz - HDR_SZ doesn't underflow.

For now, I think resv_sz is already sizeof(__reserved) so this is never true, but I suspect we will want to reuse this code eventually to looko at the contents of extra_context. Then, resv_sz would be the extra_context size rather than a fixed constant.

• while (offs <= resv_sz - HDR_SZ &&

•       head->magic != magic && head->magic) {
  

• offs += head->size;
  

• head = GET_RESV_NEXT_HEAD(head);
  

• }
• if (head->magic == magic) {

• found = head;
  

• if (offset)
  

• 	*offset = offs;
  

• }

Although there appears to be some code duplication here, I guess you need things this way to do the right thing if called with magic==0.

So I guess this is fine.

Ultimately it would be good to have GET_RESV_NEXT_HEAD() work more like an iterator, doing integrity bounds/alignment checks and updating offs as it goes, but for now I think the code is sufficient. We can always beef it up later to catch more kinds of error from the kernel.

• return found;

+}

+bool validate_extra_context(struct extra_context *extra, char **err) +{

• struct _aarch64_ctx *term;
• if (!extra || !err)

• return false;
  

• fprintf(stderr, "Validating EXTRA...\n");
• term = GET_RESV_NEXT_HEAD(extra);
• if (!term || term->magic || term->size) {

• *err = "Missing terminator after EXTRA context";
  

• return false;
  

• }
• if (extra->datap & 0x0fUL)

• *err = "Extra DATAP misaligned";
  

• else if (extra->size & 0x0fUL)

• *err = "Extra SIZE misaligned";
  

• else if (extra->datap != (uint64_t)term + sizeof(*term))

• *err = "Extra DATAP misplaced (not contiguos)";
  

• if (*err)

• return false;
  

• return true;

+}

+bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err) +{

• bool terminated = false;
• size_t offs = 0;
• int flags = 0;
• struct extra_context *extra = NULL;
• struct _aarch64_ctx *head =

• (struct _aarch64_ctx *)uc->uc_mcontext.__reserved;
  

• if (!err)

• return false;
  

• /* Walk till the end terminator verifying __reserved contents */
• while (head && !terminated && offs < resv_sz) {

• if ((uint64_t)head & 0x0fUL) {
  

• 	*err = "Misaligned HEAD";
  

• 	return false;
  

• }
  

• switch (head->magic) {
  

• case 0:
  

• 	if (head->size)
  

• 		*err = "Bad size for terminator";
  

• 	else
  

• 		terminated = true;
  

• 	break;
  

• case FPSIMD_MAGIC:
  

• 	if (flags & FPSIMD_CTX)
  

• 		*err = "Multiple FPSIMD_MAGIC";
  

• 	else if (head->size !=
  

• 		 sizeof(struct fpsimd_context))
  

• 		*err = "Bad size for fpsimd_context";
  

• 	flags |= FPSIMD_CTX;
  

• 	break;
  

• case ESR_MAGIC:
  

• 	if (head->size != sizeof(struct esr_context))
  

• 		fprintf(stderr,
  

• 			"Bad size for esr_context is not an error...just ignore.\n");
  

• 	break;
  

Although it's not essential, I'd prefer that we enforce the correct size here. All records, including esr_context are intended to be fixed-size.

In the kernel we check a bit more loosely -- this allows userspace to delete a record using head->size += next_head->size. This way no memmove() is needed to shuffle subsequent records down. I don't know whether any userspace code makes use of this -- prior to SVE there were no optional records except for esr_context, and sigreturn ignores that in any case so deleting it is pointless.

The kernel should never insert extra padding between records though, so I think it makes sense to have strict size checks in this test.

[...]

Cheers ---Dave

On Fri, Sep 06, 2019 at 11:26:29AM +0100, Cristian Marussi wrote:

Hi

On 04/09/2019 12:47, Dave Martin wrote:

...

^Nit: "add one testcase" doesn't really describe what is being added here.

Yep I know...I was trying to stay under first commit line length limitations

...

Maybe the following would work as the subject line:

--8<-- kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils -->8--

I'll grab it

...

The remainder of the commit message looks fine.

On Mon, Sep 02, 2019 at 12:29:23pm +0100, Cristian Marussi wrote:

...

Add some arm64/signal specific boilerplate and utility code to help further testcases' development.

Introduce also one simple testcase mangle_pstate_invalid_compat_toggle and some related helpers: it is a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to toggle PSTATE state bits to switch the system between 32bit/64bit execution state. Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v4 --> v5

• moved kernel headers include search to top level KSFT arm64 Makefile
• removed warning about kernel headers not found
• moved testcases/.gitignore up one level

v3 --> v4

• removed standalone mode
• fixed arm64/signal/README
• add file level comments: test layout / test description
• reduced verbosity
• removed spurious headers includes
• reviewed ID_AA64MMFR[1,2]_EL1 macros
• removed unused feats_ok
• simplified CPU features gathering
• reviewed included headers
• fixed/refactored get_header() and validation routines
• added test description

---

[...]

...

diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c new file mode 100644 index 000000000000..f05c6dbf8659 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/test_signals.c @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Generic test wrapper for arm64 signal tests.
• • Each test provides its own tde struct tddescr descriptor to link with

Typo? tdescr

Yes

...

[...]

...

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c new file mode 100644 index 000000000000..e2a5f37e6ad3 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c @@ -0,0 +1,269 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2019 ARM Limited */

+#include <stdio.h> +#include <stdlib.h> +#include <signal.h> +#include <string.h> +#include <unistd.h> +#include <assert.h> +#include <sys/auxv.h> +#include <linux/auxvec.h> +#include <ucontext.h>

+#include "test_signals.h" +#include "test_signals_utils.h" +#include "testcases/testcases.h"

+extern struct tdescr *current;

+static char *feats_store[FMAX_END] = {

Nit: can we call this feat_names[]?ok

"store" makes me think of loads and stores...

Also, nit: can this be static const char *const []?

String literals are immutable anyway, and I guess we don't intend too modify the pointers to the strings either...

Yes of course.

...

...

• " SSBS ",
• " PAN ",
• " UAO ",

+};

+#define MAX_FEATS_SZ 128 +static char feats_string[MAX_FEATS_SZ];

+static inline char *feats_to_string(unsigned long feats) +{

• size_t flen = MAX_FEATS_SZ - 1;
• for (int i = 0; i < FMAX_END; i++) {

• if (feats & 1UL << i) {
  

Nit: maybe have () around (1UL << i), though I think it makes no difference.

Yes it's better, I feared that, being not required, was frown upon.

...

...

• 	size_t tlen = strlen(feats_store[i]);
  

• 	assert(flen > tlen);
  

• 	flen -= tlen;
  

• 	strncat(feats_string, feats_store[i], flen);
  

• }
  

• }
• return feats_string;

+}

+static void unblock_signal(int signum) +{

• sigset_t sset;
• sigemptyset(&sset);
• sigaddset(&sset, signum);
• sigprocmask(SIG_UNBLOCK, &sset, NULL);

+}

+static void default_result(struct tdescr *td, bool force_exit) +{

• if (td->pass)

• fprintf(stderr, "==>> completed. PASS(1)\n");
  

• else

• fprintf(stdout, "==>> completed. FAIL(0)\n");
  

• if (force_exit)

• exit(td->pass ? EXIT_SUCCESS : EXIT_FAILURE);
  

+}

+static inline bool are_feats_ok(struct tdescr *td) +{

• return (td->feats_required & td->feats_supported) == td->feats_required;

+}

+static void default_handler(int signum, siginfo_t *si, void *uc) +{

• if (current->sig_trig && signum == current->sig_trig) {

(Thinking about it, signum is never 0 because there is no signal 0. So we could write if (signum == current->sig_trig). But I think your code makes the intention clearer -- so no need to change it.)

Yes, in fact that's the reason I left it even if unneeded.

Ok, sounds fine.

...

...

• fprintf(stderr, "Handling SIG_TRIG\n");
  

• current->triggered = 1;
  

• /* ->run was asserted NON-NULL in test_setup() already */
  

• current->run(current, si, uc);
  

• } else if (signum == SIGILL && !current->initialized) {

[...]

...

...

+static int test_init(struct tdescr *td) +{

• td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
• if (!td->minsigstksz)

• td->minsigstksz = MINSIGSTKSZ;
  

• fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
• if (td->feats_required) {

• bool feats_ok = false;
  

• td->feats_supported = 0;
  

• /*
  

•  * Checking for CPU required features using both the
  

•  * auxval and the arm64 MRS Emulation to read sysregs.
  

•  */
  

• if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
  

• 	uint64_t val = 0;
  

• 	/* Uses HWCAP to check capability */
  

• 	if (getauxval(AT_HWCAP) & HWCAP_SSBS)
  

• 		td->feats_supported |= FEAT_SSBS;
  

Should this be outside the HWCAP_CPUID check?

Right.

...

It's only the get_regval(SYS_ID_foo) based checks that depend on HWCAP_CPUID.

...

• 	/* Uses MRS emulation to check capability */
  

• 	get_regval(SYS_ID_AA64MMFR1_EL1, val);
  

• 	if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
  

• 		td->feats_supported |= FEAT_PAN;
  

• 	/* Uses MRS emulation to check capability */
  

• 	get_regval(SYS_ID_AA64MMFR2_EL1, val);
  

• 	if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
  

• 		td->feats_supported |= FEAT_UAO;
  

• } else {
  

• 	fprintf(stderr,
  

• 		"HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
  

• }
  

• feats_ok = are_feats_ok(td);
  

• fprintf(stderr,
  

• 	"Required Features: [%s] %ssupported\n",
  

• 	feats_ok ? feats_to_string(td->feats_supported) :
  

• 	feats_to_string(td->feats_required ^
  

• 			td->feats_supported),
  

Should this be something like: td->feats_required & ~td->feats_supported ?

Otherwise we'll include features that are supported but not required, when printing the features that are NOT supported.

Alternatively, we could just print out the required and supported sets separately and leave it up to the user to obverse how they are different.

(Watch out for calling feats_to_string() twice in the same printf() call though.)

Ok. Reported information was poor in fact.

[..]

...

...

diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c new file mode 100644 index 000000000000..72e3f482b177 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (C) 2019 ARM Limited */ +#include "testcases.h"

+struct _aarch64_ctx *get_header(struct _aarch64_ctx *head, uint32_t magic,

• 		size_t resv_sz, size_t *offset)
  

+{

• size_t offs = 0;
• struct _aarch64_ctx *found = NULL;
• if (!head)

• return found;
  

I suggest you also check for resv_sz < HDR_SZ, since the while() condition assumes that resv_sz - HDR_SZ doesn't underflow.

For now, I think resv_sz is already sizeof(__reserved) so this is never true, but I suspect we will want to reuse this code eventually to looko at the contents of extra_context. Then, resv_sz would be the extra_context size rather than a fixed constant.

Ok....in fact I think I removed recently such check...not sure why o_O I'll fix it.

...

...

• while (offs <= resv_sz - HDR_SZ &&

•       head->magic != magic && head->magic) {
  

• offs += head->size;
  

• head = GET_RESV_NEXT_HEAD(head);
  

• }
• if (head->magic == magic) {

• found = head;
  

• if (offset)
  

• 	*offset = offs;
  

• }

Although there appears to be some code duplication here, I guess you need things this way to do the right thing if called with magic==0.

So I guess this is fine.

Yes that was exactly the point, and it seemed to me that removing further duplication would have made the code more complex and unreadable.

...

Ultimately it would be good to have GET_RESV_NEXT_HEAD() work more like an iterator, doing integrity bounds/alignment checks and updating offs as it goes, but for now I think the code is sufficient. We can always beef it up later to catch more kinds of error from the kernel.

Yes I remember you told me that on a previous iteration, but for now I left the GET_RESV_NEXT_HEAD() as it was without embedding the bounds checking logic because it is indirectly used also by the validation function that I use in the ASSERT_GOOD/BAD_CONTEXT() macros, so it should be able to handle artficially badly formed and corrupted frames without bailing out: it just walks and any kind of logic is handled outside...but maybe I'm overthinking (certainly I have not explained this reasons anywhere...I'll add a comment)

Sure, I think that's sufficient.

We could always improve this later, if needed.

[...]

...

...

+bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err) +{

• bool terminated = false;
• size_t offs = 0;
• int flags = 0;
• struct extra_context *extra = NULL;
• struct _aarch64_ctx *head =

• (struct _aarch64_ctx *)uc->uc_mcontext.__reserved;
  

• if (!err)

• return false;
  

• /* Walk till the end terminator verifying __reserved contents */
• while (head && !terminated && offs < resv_sz) {

• if ((uint64_t)head & 0x0fUL) {
  

• 	*err = "Misaligned HEAD";
  

• 	return false;
  

• }
  

• switch (head->magic) {
  

• case 0:
  

• 	if (head->size)
  

• 		*err = "Bad size for terminator";
  

• 	else
  

• 		terminated = true;
  

• 	break;
  

• case FPSIMD_MAGIC:
  

• 	if (flags & FPSIMD_CTX)
  

• 		*err = "Multiple FPSIMD_MAGIC";
  

• 	else if (head->size !=
  

• 		 sizeof(struct fpsimd_context))
  

• 		*err = "Bad size for fpsimd_context";
  

• 	flags |= FPSIMD_CTX;
  

• 	break;
  

• case ESR_MAGIC:
  

• 	if (head->size != sizeof(struct esr_context))
  

• 		fprintf(stderr,
  

• 			"Bad size for esr_context is not an error...just ignore.\n");
  

• 	break;
  

Although it's not essential, I'd prefer that we enforce the correct size here. All records, including esr_context are intended to be fixed-size.

In the kernel we check a bit more loosely -- this allows userspace to delete a record using head->size += next_head->size. This way no memmove() is needed to shuffle subsequent records down. I don't know whether any userspace code makes use of this -- prior to SVE there were no optional records except for esr_context, and sigreturn ignores that in any case so deleting it is pointless.

...

The kernel should never insert extra padding between records though, so I think it makes sense to have strict size checks in this test.

Ok, I'll do. I kept it loose as it is in Kernel, because in some past tests (now removed) I used to play also with esr_context size to build easily an inflated fake sigframe (but good) and adding some badness on top of it.

OK, I guess that would be a problem if we want to use the same validity checker for kernel-generated and user-generated signal frames.

I'm happy either way ... I'll take a look at v6 and comment.

Cheers ---Dave

On Mon, Sep 02, 2019 at 12:29:24pm +0100, Cristian Marussi wrote:

Add a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to set PSTATE DAIF bits to an invalid value (masking everything). Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

Reviewed-by: Dave Martin Dave.Martin@arm.com

---

v3 --> v4

• fixed commit message
• added testcase comment description

---

.../mangle_pstate_invalid_daif_bits.c | 35 +++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c new file mode 100644 index 000000000000..434b82597007 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_daif_bits.c @@ -0,0 +1,35 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, mangling the
• • DAIF bits in an illegal manner: this attempt must be spotted by Kernel
• • and the test case is expected to be terminated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si,

• 		     ucontext_t *uc)
  

+{

• ASSERT_GOOD_CONTEXT(uc);
• /*

• * This config should trigger a SIGSEGV by Kernel when it checks
  

• * the sigframe consistency in valid_user_regs() routine.
  

• */
  

• uc->uc_mcontext.pstate |= PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT;
• return 1;

+}

+struct tdescr tde = {

• .sanity_disabled = true,
  

• .name = "MANGLE_PSTATE_INVALID_DAIF_BITS",
  

• .descr = "Mangling uc_mcontext with INVALID DAIF_BITS",
  

• .sig_trig = SIGUSR1,
  

• .sig_ok = SIGSEGV,
  

• .run = mangle_invalid_pstate_run,
  

+};

2.17.1

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Sep 02, 2019 at 12:29:25pm +0100, Cristian Marussi wrote:

Add 6 simple mangle testcases that mess with the ucontext_t from within the signal handler, trying to toggle PSTATE mode bits to trick the system into switching to EL1/EL2/EL3 using both SP_EL0(t) and SP_ELx(h). Expects SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

We might be able to squash this down further using a single .c file and passing some magic -D option on the compiler command line, but it's probably not worth it.

This removes most of the actual code duplication already.

So,

Reviewed-by: Dave Martin Dave.Martin@arm.com

---

v3 --> v4

• fixed commit message
• macroization
• splitted into 6 macro-ised testcases to address t/h SP selection modes
• added test description

---

.../mangle_pstate_invalid_mode_el1h.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_el1t.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_el2h.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_el2t.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_el3h.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_el3t.c | 15 ++++++++++ .../mangle_pstate_invalid_mode_template.h | 28 +++++++++++++++++++ 7 files changed, 118 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h

diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c new file mode 100644 index 000000000000..95f821abdf46 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1h.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(1h); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c new file mode 100644 index 000000000000..cc222d8a618a --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el1t.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(1t); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c new file mode 100644 index 000000000000..2188add7d28c --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2h.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(2h); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c new file mode 100644 index 000000000000..df32dd5a479c --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el2t.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(2t); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c new file mode 100644 index 000000000000..9e6829b7e5db --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3h.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(3h); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c new file mode 100644 index 000000000000..5685a4f10d06 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_el3t.c @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, toggling
• • the mode bit to escalate exception level: this attempt must be spotted
• • by Kernel and the test case is expected to be termninated via SEGV.
• */

+#include "test_signals_utils.h" +#include "testcases.h"

+#include "mangle_pstate_invalid_mode_template.h"

+DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(3t); diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h new file mode 100644 index 000000000000..f5bf1804d858 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_invalid_mode_template.h @@ -0,0 +1,28 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2019 ARM Limited
• • Utility macro to ease definition of testcases toggling mode EL
• */

+#define DEFINE_TESTCASE_MANGLE_PSTATE_INVALID_MODE(_mode) \

• 							\
  

+static int mangle_invalid_pstate_run(struct tdescr *td, siginfo_t *si, \

• 		     ucontext_t *uc)			\
  

+{ \

• ASSERT_GOOD_CONTEXT(uc); \

• 							\
  

• uc->uc_mcontext.pstate &= ~PSR_MODE_MASK; \
• uc->uc_mcontext.pstate |= PSR_MODE_EL ## _mode; \

• 							\
  

• return 1; \

+} \

• 							\
  

+struct tdescr tde = { \

• .sanity_disabled = true,				\
  

• .name = "MANGLE_PSTATE_INVALID_MODE_EL"#_mode,		\
  

• .descr = "Mangling uc_mcontext INVALID MODE EL"#_mode,	\
  

• .sig_trig = SIGUSR1,					\
  

• .sig_ok = SIGSEGV,					\
  

• .run = mangle_invalid_pstate_run,			\
  

+}

2.17.1

---

linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

On Mon, Sep 02, 2019 at 12:29:26pm +0100, Cristian Marussi wrote:

Add a simple mangle testcase which messes with the ucontext_t from within the signal handler, trying to set the PSTATE SSBS bit. Expect SIGILL if SSBS feature is unsupported or that, on test PASS, the value set in PSTATE.SSBS in the signal frame is preserved by sigreturn.

Additionally, in order to support this test specific needs:

• extend signal testing framework to allow the definition of a custom per test initialization function to be run at the end of test setup.
• introduced a set_regval() helper to set system register values in a toolchain independent way.
• introduce also a new common utility function: get_current_context() which can be used to grab a ucontext without the help of libc, and detect if such ucontext has been actively used to jump back into it.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit message
• missing include signal.h
• added .init per-test init-func
• added set_regval() helper
• added SSBS clear to 0 custom .init function
• removed volatile qualifier associated with sig_atomic_t data
• added dsb inside handler to ensure the writes related to the grabbed ucontext have completed
• added test description

---

.../selftests/arm64/signal/test_signals.h | 20 +++- .../arm64/signal/test_signals_utils.c | 98 +++++++++++++++++++ .../arm64/signal/test_signals_utils.h | 2 + .../testcases/mangle_pstate_ssbs_regs.c | 69 +++++++++++++ 4 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c

diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h index a1cf69997604..0767e27fbe78 100644 --- a/tools/testing/selftests/arm64/signal/test_signals.h +++ b/tools/testing/selftests/arm64/signal/test_signals.h @@ -27,6 +27,14 @@ : "memory"); \ } +#define set_regval(regname, in) \ +{ \

• asm volatile("msr " __stringify(regname) ", %0" \
• : \
• : "r" (in) \
• : "memory"); \

+}

/* Regs encoding and masks naming copied in from sysreg.h */ #define SYS_ID_AA64MMFR1_EL1 S3_0_C0_C7_1 /* MRS Emulated */ #define SYS_ID_AA64MMFR2_EL1 S3_0_C0_C7_2 /* MRS Emulated */ @@ -89,12 +97,16 @@ struct tdescr { /* optional sa_flags for the installed handler */ int sa_flags; ucontext_t saved_uc;

• /* a custom setup function to be called before test starts */

• /* used by get_current_ctx() */
• size_t live_sz;
• ucontext_t *live_uc;
• sig_atomic_t live_uc_valid;
• /* a custom setup: called alternatively to default_setup */ int (*setup)(struct tdescr *td);
• /* a custom init: called by default test initialization */
• void (*init)(struct tdescr *td); /* a custom cleanup function called before test exits */ void (*cleanup)(struct tdescr *td);

• /* an optional function to be used as a trigger for test starting */ int (*trigger)(struct tdescr *td); /*

@@ -102,10 +114,8 @@ struct tdescr { * presence of the trigger function above; this is mandatory */ int (*run)(struct tdescr *td, siginfo_t *si, ucontext_t *uc);

• /* an optional function for custom results' processing */ void (*check_result)(struct tdescr *td);
• void *priv;

}; diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c index e2a5f37e6ad3..c6fdcb23f246 100644 --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c @@ -11,12 +11,16 @@ #include <linux/auxvec.h> #include <ucontext.h> +#include <asm/unistd.h>

#include "test_signals.h" #include "test_signals_utils.h" #include "testcases/testcases.h" extern struct tdescr *current; +static int sig_copyctx = SIGUSR2;

static char *feats_store[FMAX_END] = { " SSBS ", " PAN ", @@ -43,6 +47,81 @@ static inline char *feats_to_string(unsigned long feats) return feats_string; } +/*

• • Obtaining a valid and full-blown ucontext_t from userspace is tricky:
• • libc getcontext does() not save all the regs and messes with some of
• • them (pstate value in particular is not reliable).
• • Here we use a service signal to grab the ucontext_t from inside a
• • dedicated signal handler, since there, it is populated by Kernel
• • itself in setup_sigframe(). The grabbed context is then stored and
• • made available in td->live_uc.
• • Anyway this function really serves a dual purpose:
• • 1. grab a valid sigcontext into td->live_uc for result analysis: in
• • such case it returns 1.
• • 2. detect if somehow a previously grabbed live_uc context has been
• • used actively with a sigreturn: in such a case the execution would have
• • magically resumed in the middle of the function itself (seen_already==1):
• • in such a case return 0, since in fact we have not just simply grabbed
• • the context.
• • This latter case is useful to detect when a fake_sigreturn test-case has
• • unexpectedly survived without hittig a SEGV.
• */

+bool get_current_context(struct tdescr *td, ucontext_t *dest_uc) +{

• static sig_atomic_t seen_already;
• assert(td && dest_uc);
• /* it's a genuine invocation..reinit */
• seen_already = 0;
• td->live_uc_valid = 0;
• td->live_sz = sizeof(*dest_uc);
• memset(dest_uc, 0x00, td->live_sz);
• td->live_uc = dest_uc;
• /*

• * Grab ucontext_t triggering a signal...
  

• * ASM equivalent of raise(sig_copyctx);
  

• *
  

• * Note that:
  

• * - live_uc_valid is declared sig_atomic_t in struct tdescr
  

• *   since it will be changed inside the sig_copyctx handler
  

• * - the kill() syscall invocation returns only after any possible
  

• *   registered signal handler for the invoked signal has returned,
  

• *   so that live_uc_valid flag is surely up to date when this
  

• *   function return it.
  

• * - the additional 'memory' clobber is there to avoid possible
  

• *   compiler's assumption on live_uc_valid, seen-already and
  

• *   the content pointed by dest_uc, which are all changed inside
  

• *   the signal handler, without resorting to the volatile qualifier
  

• *   (and keeping quiet checkpatch.pl)
  

• */
  

• asm volatile ("mov x8, %0\n\t"

•       "svc #0\n\t"
  

•       "mov x1, %1\n\t"
  

•       "mov x8, %2\n\t"
  

•       "svc #0"
  

•       :
  

•       : "i" (__NR_getpid), "r" (sig_copyctx), "i" (__NR_kill)
  

•       : "x1", "x8", "x0", "memory");
  

• /*

• * If we get here with seen_already==1 it implies the td->live_uc
  

• * context has been used to get back here....this probably means
  

• * a test has failed to cause a SEGV...anyway the live_uc has not
  

• * just been acquired...so return 0
  

• */
  

• if (seen_already) {

• fprintf(stdout,
  

• 	"Successful sigreturn detected: live_uc is stale !\n");
  

• return 0;
  

• }
• seen_already = 1;
• return td->live_uc_valid;

+}

static void unblock_signal(int signum) { sigset_t sset; @@ -124,6 +203,17 @@ static void default_handler(int signum, siginfo_t *si, void *uc) * to terminate immediately exiting straight away */ default_result(current, 1);

• } else if (signum == sig_copyctx && current->live_uc) {

• memcpy(current->live_uc, uc, current->live_sz);
  

• ASSERT_GOOD_CONTEXT(current->live_uc);
  

• current->live_uc_valid = 1;
  

• /*
  

•  * Ensure above writes have completed before signal
  

•  * handler terminates
  

•  */
  

• asm volatile ("dsb sy" ::: "memory");
  

The dsb doesn't help here: this has no effect on how the compiler caches variables in registers etc.

Overall, I think some details need a bit of a rethink here.

We need some way to ensure coherency of accesses to variables around and inside the signal handler here, but since we're running in a single thread that may be interrupted by a signal handler (running in the same thread), it's compiler<->compiler coherency that's the issue here, not cpu<->cpu or cpu<->device coherency.

There may also be atomicity concerns, since the compiler might move stuff across and/or duplicate or tear reads/writes around the asm where the signal is delivered.

The classic solution to these problems is to use volatile, but this is a blunt tool and you often end up having to mark more objects volatile than you really want to in order to ensure correctness. The ordering behaviour of accesses to volatiles is also ill-specified for accesses made in different threads.

That said, efficiency is of no concern here and we're single-threaded, so a blunt, simple tool may still be adequate.

Another issue is that nothing stops the stack frame the captured SP points to from disappearing between get_current_context() and the fake_sigreturn() that tries to jump back to it.

To avoid this issue, we'd probably need to inline more of get_current_context(), i.e., turn it into a macro.

• fprintf(stderr,
  

• 	"GOOD CONTEXT grabbed from sig_copyctx handler\n");
  

  } else { if (signum == current->sig_unsupp && !are_feats_ok(current)) { fprintf(stderr,

@@ -222,7 +312,15 @@ static int test_init(struct tdescr *td) !feats_ok ? "NOT " : ""); }

• if (td->sig_trig == sig_copyctx)

• sig_copyctx = SIGUSR1;
  

• unblock_signal(sig_copyctx);
• /* Perform test specific additional initialization */
• if (td->init)

• td->init(td);
  

  td->initialized = 1;
• return 1;

} diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h index 8658d1a7d4b9..ce35be8ebc8e 100644 --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h @@ -10,4 +10,6 @@ int test_setup(struct tdescr *td); void test_cleanup(struct tdescr *td); int test_run(struct tdescr *td); void test_result(struct tdescr *td);

+bool get_current_context(struct tdescr *td, ucontext_t *dest_uc); #endif diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c new file mode 100644 index 000000000000..15e6f62512d5 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Try to mangle the ucontext from inside a signal handler, setting the
• • SSBS bit to 1 and veryfing that such modification is preserved.
• */

+#include <stdio.h> +#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+static void mangle_invalid_pstate_ssbs_init(struct tdescr *td) +{

• fprintf(stderr, "Clearing SSBS to 0\n");
• set_regval(SSBS_SYSREG, 0);

+}

+static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,

• 			  siginfo_t *si, ucontext_t *uc)
  

+{

• ASSERT_GOOD_CONTEXT(uc);
• /* set bit value */
• uc->uc_mcontext.pstate |= PSR_SSBS_BIT;

Can we check that uc->uc_mcontext.pstate & PSR_SSBS_BIT is initially 0?

If not, it suggests either a test bug, or modification of the SSBS flag by other C code before the test signal was delivered.

• fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",

• uc->uc_mcontext.pstate);
  

• /* Save after mangling...it should be preserved */
• td->saved_uc = *uc;
• return 1;

+}

+static void pstate_ssbs_bit_checks(struct tdescr *td) +{

• uint64_t val = 0;
• ucontext_t uc;
• /* This check reports some result even if MRS SSBS unsupported */
• if (get_current_context(td, &uc))

• fprintf(stderr,
  

• 	"INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
  

• 	uc.uc_mcontext.pstate,
  

• 	(td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
  

• 	(uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
  

• 	"PRESERVED" : "CLEARED");
  

• fprintf(stderr, "Checking with MRS SSBS...\n");
• get_regval(SSBS_SYSREG, val);
• fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
• /* pass when preserved */
• td->pass = (val & PSR_SSBS_BIT) ==

•    (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
  

+}

+struct tdescr tde = {

• .sanity_disabled = true,
  

• .name = "MANGLE_PSTATE_SSBS_REGS",
  

• .descr = "Mangling uc_mcontext changing SSBS.(PRESERVE)",
  

Can we come up with a clearer description here? I'm not sure how to read this.

[...]

Cheers ---Dave

Hi

On 04/09/2019 12:48, Dave Martin wrote:

On Mon, Sep 02, 2019 at 12:29:27pm +0100, Cristian Marussi wrote:

...

Add a simple fake_sigreturn testcase which builds a ucontext_t with a bad magic header and place it onto the stack. Expects a SIGSEGV on test PASS.

Introduce a common utility assembly trampoline function to invoke a sigreturn while placing the provided sigframe at wanted alignment and also an helper to make space when needed inside the sigframe reserved area.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• fix signal.S, handle misalign requests too
• remove unneeded comments
• add signal.h include
• added get_starting_head() helper
• added test description

---

tools/testing/selftests/arm64/signal/Makefile | 2 +- .../testing/selftests/arm64/signal/signals.S | 62 +++++++++++++++++++ .../arm64/signal/test_signals_utils.h | 1 + .../testcases/fake_sigreturn_bad_magic.c | 54 ++++++++++++++++ .../arm64/signal/testcases/testcases.c | 28 +++++++++ .../arm64/signal/testcases/testcases.h | 4 ++ 6 files changed, 150 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/arm64/signal/signals.S create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c

diff --git a/tools/testing/selftests/arm64/signal/Makefile b/tools/testing/selftests/arm64/signal/Makefile index f78f5190e3d4..b497cfea4643 100644 --- a/tools/testing/selftests/arm64/signal/Makefile +++ b/tools/testing/selftests/arm64/signal/Makefile @@ -28,5 +28,5 @@ clean: # Common test-unit targets to build common-layout test-cases executables # Needs secondary expansion to properly include the testcase c-file in pre-reqs .SECONDEXPANSION: -$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c $$@.c test_signals.h test_signals_utils.h testcases/testcases.h +$(PROGS): test_signals.c test_signals_utils.c testcases/testcases.c signals.S $$@.c test_signals.h test_signals_utils.h testcases/testcases.h $(CC) $(CFLAGS) $^ -o $@ diff --git a/tools/testing/selftests/arm64/signal/signals.S b/tools/testing/selftests/arm64/signal/signals.S new file mode 100644 index 000000000000..b89fec0d5ba0 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/signals.S @@ -0,0 +1,62 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* Copyright (C) 2019 ARM Limited */

+#include <asm/unistd.h>

+.section .rodata, "a" +call_fmt:

• .asciz "Calling sigreturn with fake sigframe sized:%zd at SP @%08lX\n"

+.text

+.globl fake_sigreturn

+/* fake_sigreturn x0:&sigframe, x1:sigframe_size, x2:misalign_bytes */ +fake_sigreturn:

Nit: the "bl printf" later on destroys lr.

This isn't a problem, since the function never tries to return anyway -- if things go wrong you just "b .".

But it may be helpful for debug purposes to at least create a frame record, e.g.:

stp x29, x30, [sp, #-16]! mov x29, sp

before doing anything else.

ok

...

• mov x20, x0
• mov x21, x1
• mov x22, x2
• mov x23, sp

Nit: to follow the conventional asm style for arm64 kernel code, can you format lines as

<TAB> op<TAB> operands

ok

...

• /* create space on the stack for fake sigframe 16 bytes-aligned */
• add x0, x21, #16
• bic x0, x0, #15
• sub x23, x23, x0
• /* any misalignment requested ? */
• add x23, x23, x22

Aren't we actually reducing the allocation here, rather than increasing it?

For the misalignment bytes if any yes...

Doing something like this may work to allocate guaranteed sufficient space:

add x0, x21, x22 add x0, x0, #15 bic x0, x0, #15 /* round_up(sigframe_size + misglian_bytes, 16) */ sub sp, sp, x0 add x23, sp, x22 /* new sigframe base with misaligment */

(You can drop the mov into x23 above in your function prologue if you code it this way.)

ok...but...shouldn't be

add x0, x0, #16

before

bic x-, x0, #15

instead of adding #15 ?

...

• ldr x0, =call_fmt
• mov x1, x21
• mov x2, x23
• bl printf
• mov sp, x23

AAPCS64 requires sp to be 16-byte aligned at function boundaries, so we may get stack alignments faults in mempcy() here. Possibly these can be confused with test failure SEGVs (I can't remember offhand how stack alignment faults are supported).

Didn't know about function boundaries requirements.

Coding something like what I have above to guarantee stack alignment should avoid this.

Nice I'll do.

...

• /* now fill it with the provided content... */
• mov x0, sp

With my version this would be mov x0, x23

ok

...

• mov x1, x20
• mov x2, x21
• bl memcpy
• /*

• * Here saving a last minute SP to current->token acts as a marker:
  

• * if we got here, we are successfully faking a sigreturn; in other
  

• * words we are sure no bad fatal signal has been raised till now
  

• * for unrelated reasons, so we should consider the possibly observed
  

• * fatal signal like SEGV coming from Kernel restore_sigframe() and
  

• * triggered as expected from our test-case.
  

• * For simplicity this assumes that current field 'token' is laid out
  

• * as first in struct tdescr
  

• */
  

• ldr x0, current

Nit: it probably doesn't matter since this will be a small binary after linking, but to avoid possible fixup errors during linking you could also do:

adrp x0, current ldr x0, [x0, #:lo12:current]

This raises the addressing range from 0.5 MB or so to a few GB, making link errors much more unlikely.

Ok...I'll fix this nitpick once I'll have understood it :D

...

• str x23, [x0]
• /* SP is already pointing back to the just built fake sigframe here */
• mov x8, #__NR_rt_sigreturn

And finally we would mov sp, x23 here.

Yes

...

• svc #0
• /*

• * Above sigreturn should not return...looping here leads to a timeout
  

• * and ensure proper and clean test failure, instead of jumping around
  

• * on a potentially corrupted stack.
  

• */
  

• b .

diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h index ce35be8ebc8e..68930f1e46e5 100644 --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h @@ -12,4 +12,5 @@ int test_run(struct tdescr *td); void test_result(struct tdescr *td); bool get_current_context(struct tdescr *td, ucontext_t *dest_uc); +int fake_sigreturn(void *sigframe, size_t sz, int misalign_bytes); #endif diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c new file mode 100644 index 000000000000..7fb700b9801b --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c @@ -0,0 +1,54 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including a BAD Unknown magic
• • record: on sigreturn Kernel must spot this attempt and the test
• • case is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_bad_magic_run(struct tdescr *td,

• 			siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• /* need at least 2*HDR_SZ space: KSFT_BAD_MAGIC + terminator. */
• need_sz = HDR_SZ * 2;
• head = get_starting_head(shead, need_sz, resv_sz, NULL);

Nit: are the need_sz and resv_sz variables required?

Maybe they help to highlight what these expressions mean in the get_starting_head() call though. I'm happy either way.

Not really required...I'll remove. (probably in other tests were and they landed here too...)

...

• if (head) {

• /*
  

•  * use a well known NON existent bad magic...something
  

•  * we should pretty sure won't be ever defined in Kernel
  

•  */
  

• head->magic = KSFT_BAD_MAGIC;
  

• head->size = HDR_SZ;
  

• write_terminator_record(GET_RESV_NEXT_HEAD(head));
  

• ASSERT_BAD_CONTEXT(&sf.uc);
  

• fake_sigreturn(&sf, sizeof(sf), 0);
  

• }
• return 1;

+}

+struct tdescr tde = {

• .name = "FAKE_SIGRETURN_BAD_MAGIC",
  

• .descr = "Trigger a sigreturn with a sigframe with a bad magic",
  

• .sig_ok = SIGSEGV,
  

• .timeout = 3,
  

• .run = fake_sigreturn_bad_magic_run,
  

+}; diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c index 72e3f482b177..2effb8ded935 100644 --- a/tools/testing/selftests/arm64/signal/testcases/testcases.c +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c @@ -149,3 +149,31 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err) return true; }

Maybe add a comment saying what this function does.

To check my understanding: The purpose is to find a place to append a new record, right? By default we append at the end (i.e., at the terminator), but because extra_context is optional we replace that instead if there isn't sufficient space after the terminator in __reserved[].

Yes I'll add a comment.

...

+struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,

• 		       size_t need_sz, size_t resv_sz,
  

• 		       size_t *offset)
  

+{

• size_t offs = 0;
• struct _aarch64_ctx *head;
• head = get_terminator(shead, resv_sz, &offs);
• /* not found a terminator...no need to update offset if any */
• if (!head)

• return head;
  

• if (resv_sz - offs < need_sz) {

• fprintf(stderr, "Low on space:%zd. Discarding extra_context.\n",
  

• 	resv_sz - offs);
  

• head = get_header(shead, EXTRA_MAGIC, resv_sz, &offs);
  

• if (!head || resv_sz - offs < need_sz) {
  

• 	fprintf(stderr,
  

• 		"Failed to reclaim space on sigframe.\n");
  

• 	return NULL;
  

• }
  

• }
• fprintf(stderr, "Available space:%zd\n", resv_sz - offs);
• if (offset)

• *offset = offs;
  

• return head;

+} diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.h b/tools/testing/selftests/arm64/signal/testcases/testcases.h index 00618c3202bb..7653f8a64b3d 100644 --- a/tools/testing/selftests/arm64/signal/testcases/testcases.h +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.h @@ -83,4 +83,8 @@ static inline void write_terminator_record(struct _aarch64_ctx *tail) tail->size = 0; } }

+struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,

• 		       size_t need_sz, size_t resv_sz,
  

• 		       size_t *offset);
  

#endif

Apart from the comments above, this looks reasonable.

Cheers ---Dave

Cheers

Cristian

On Mon, Sep 02, 2019 at 12:29:28pm +0100, Cristian Marussi wrote:

Add a simple fake_sigreturn testcase which builds a ucontext_t with a badly sized terminator record and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• add signal.h include
• using new get_starting_head() helper
• added test description

---

.../fake_sigreturn_bad_size_for_magic0.c | 49 +++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c new file mode 100644 index 000000000000..25017fe18214 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size_for_magic0.c @@ -0,0 +1,49 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including a badly sized terminator
• • record: on sigreturn Kernel must spot this attempt and the test case
• • is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_bad_size_for_magic0_run(struct tdescr *td,

• 				  siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• /* at least HDR_SZ for the badly sized terminator. */
• need_sz = HDR_SZ;

Nit: do we need the resv_sz and need_sz variables here?

• head = get_starting_head(shead, need_sz, resv_sz, NULL);
• if (head) {

Perhaps we could fail immediately rather than relying on timeout here?

Probably not a huge deal though.

• head->magic = 0;
  

• head->size = HDR_SZ;
  

• ASSERT_BAD_CONTEXT(&sf.uc);
  

• fake_sigreturn(&sf, sizeof(sf), 0);
  

• }
• return 1;

+}

+struct tdescr tde = {

• .name = "FAKE_SIGRETURN_BAD_SIZE_FOR_TERMINATOR",
  

• .descr = "Trigger a sigreturn using non-zero size terminator",
  

• .sig_ok = SIGSEGV,
  

• .timeout = 3,
  

• .run = fake_sigreturn_bad_size_for_magic0_run,
  

+};

Either way,

Reviewed-by: Dave Martin Dave.Martin@arm.com

On 04/09/2019 12:49, Dave Martin wrote:

On Mon, Sep 02, 2019 at 12:29:29pm +0100, Cristian Marussi wrote:

...

Add a simple fake_sigreturn testcase which builds a ucontext_t without the required fpsimd_context and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• added signal.h
• added test description

---

.../testcases/fake_sigreturn_missing_fpsimd.c | 50 +++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c new file mode 100644 index 000000000000..08ecd8073a1a --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_missing_fpsimd.c @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack missing the mandatory FPSIMD
• • record: on sigreturn Kernel must spot this attempt and the test
• • case is expected to be terminated via SEGV.
• */

+#include <stdio.h> +#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_missing_fpsimd_run(struct tdescr *td,

• 			     siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, offset;
• struct _aarch64_ctx *head = GET_SF_RESV_HEAD(sf);
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• head = get_header(head, FPSIMD_MAGIC, resv_sz, &offset);
• if (head && resv_sz - offset >= HDR_SZ) {

• fprintf(stderr, "Mangling template header. Spare space:%zd\n",
  

• 	resv_sz - offset);
  

• /* Just overwrite fpsmid_context */
  

• write_terminator_record(head);
  

Strictly speaking, we may be throwing away more than just the fpsimd_context record here.

But I think the test works nonetheless. fpsimd_context is the only record that's mandatory in any case.

Ok

...

• ASSERT_BAD_CONTEXT(&sf.uc);
  

• fake_sigreturn(&sf, sizeof(sf), 0);
  

• }
• return 1;

+}

+struct tdescr tde = {

• .name = "FAKE_SIGRETURN_MISSING_FPSIMD",
  

• .descr = "Triggers a sigreturn with a missing fpsimd_context",
  

• .sig_ok = SIGSEGV,
  

• .timeout = 3,
  

• .run = fake_sigreturn_missing_fpsimd_run,
  

+};

Assuming the comment I just posted on v3 of this patch makes sense to you,

Reviewed-by: Dave Martin Dave.Martin@arm.com

Thanks

Cheers

Cristian

On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:

Add a simple fake_sigreturn testcase which builds a ucontext_t with an anomalous additional fpsimd_context and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• missing include
• using new get_starting_head() helper
• added test description

---

.../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c new file mode 100644 index 000000000000..c7122c44f53f --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including an additional FPSIMD
• • record: on sigreturn Kernel must spot this attempt and the test
• • case is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,

• 				siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• need_sz = HDR_SZ + sizeof(struct fpsimd_context);

Nit: Maybe write this sum in the same order as the records we're going o append, i.e.:

need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */

Also, maybe fail this test if there is no fpsimd_context in the initial frame from get_current_context(): that would be a kernel bug, but we wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that case -- so this test wouldn't test the thing it's supposed to test.

• head = get_starting_head(shead, need_sz, resv_sz, NULL);
• if (head) {

• /* Add a spurios fpsimd_context */
  

• head->magic = FPSIMD_MAGIC;
  

• head->size = sizeof(struct fpsimd_context);
  

• /* and terminate */
  

• write_terminator_record(GET_RESV_NEXT_HEAD(head));
  

• ASSERT_BAD_CONTEXT(&sf.uc);
  

• fake_sigreturn(&sf, sizeof(sf), 0);
  

• }
• return 1;

[...]

Cheers ---Dave

On Thu, Sep 05, 2019 at 01:15:58PM +0100, Cristian Marussi wrote:

Hi

On 04/09/2019 12:49, Dave Martin wrote:

...

On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:

...

Add a simple fake_sigreturn testcase which builds a ucontext_t with an anomalous additional fpsimd_context and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• missing include
• using new get_starting_head() helper
• added test description

---

.../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c new file mode 100644 index 000000000000..c7122c44f53f --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including an additional FPSIMD
• • record: on sigreturn Kernel must spot this attempt and the test
• • case is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,

• 				siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• need_sz = HDR_SZ + sizeof(struct fpsimd_context);

Nit: Maybe write this sum in the same order as the records we're going o append, i.e.:

need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */

Ok

...

Also, maybe fail this test if there is no fpsimd_context in the initial frame from get_current_context(): that would be a kernel bug, but we wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that case -- so this test wouldn't test the thing it's supposed to test.

Any context grabbed by get_current_context() is verified at first to be sane when is copied in the handler by ASSERT_GOOD_CONTEXT()

...

} else if (signum == sig_copyctx && current->live_uc) { memcpy(current->live_uc, uc, current->live_sz); ASSERT_GOOD_CONTEXT(current->live_uc); current->live_uc_valid = 1;

A missing fpsimd in the original sigframe would lead to an abort() straight away while preparing the test, and the test will fail.

OK, but there is no abort() in ASSERT_GOOD_CONTEXT(), only assert(0). Can you add an abort() after the assert() in there in patch 2?

People probably aren't going to be building the tests with -DNDEBUG, but we should avoid unnecessary surprises...

Cheers ---Dave

On Thu, Sep 05, 2019 at 02:32:09PM +0100, Cristian Marussi wrote:

On 05/09/2019 13:39, Dave Martin wrote:

...

On Thu, Sep 05, 2019 at 01:15:58PM +0100, Cristian Marussi wrote:

...

Hi

On 04/09/2019 12:49, Dave Martin wrote:

...

On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:

...

Add a simple fake_sigreturn testcase which builds a ucontext_t with an anomalous additional fpsimd_context and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• missing include
• using new get_starting_head() helper
• added test description

---

.../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c new file mode 100644 index 000000000000..c7122c44f53f --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including an additional FPSIMD
• • record: on sigreturn Kernel must spot this attempt and the test
• • case is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,

• 				siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• need_sz = HDR_SZ + sizeof(struct fpsimd_context);

Nit: Maybe write this sum in the same order as the records we're going o append, i.e.:

need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */

Ok

...

Also, maybe fail this test if there is no fpsimd_context in the initial frame from get_current_context(): that would be a kernel bug, but we wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that case -- so this test wouldn't test the thing it's supposed to test.

Any context grabbed by get_current_context() is verified at first to be sane when is copied in the handler by ASSERT_GOOD_CONTEXT()

...

} else if (signum == sig_copyctx && current->live_uc) { memcpy(current->live_uc, uc, current->live_sz); ASSERT_GOOD_CONTEXT(current->live_uc); current->live_uc_valid = 1;

A missing fpsimd in the original sigframe would lead to an abort() straight away while preparing the test, and the test will fail.

OK, but there is no abort() in ASSERT_GOOD_CONTEXT(), only assert(0). Can you add an abort() after the assert() in there in patch 2?

Ok yes, I meant the abort coming from assert(0) in fact....I'll review all the critical asserts for additional aborts in V6

OK -- I guess this only matters for things that should be reported as test failures. Things that are purely bugs in the test are less of a concern.

Cheers ---Dave

On 04/09/2019 12:49, Dave Martin wrote:

On Mon, Sep 02, 2019 at 12:29:31pm +0100, Cristian Marussi wrote:

...

Add a simple fake_sigreturn testcase which builds a ucontext_t with a badly sized header that causes a overrun in the __reserved area and place it onto the stack. Expects a SIGSEGV on test PASS.

Signed-off-by: Cristian Marussi cristian.marussi@arm.com

v3 --> v4

• fix commit
• missing include
• using new get_starting_head() helper
• added test description

---

.../testcases/fake_sigreturn_bad_size.c | 77 +++++++++++++++++++ 1 file changed, 77 insertions(+) create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c

diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c new file mode 100644 index 000000000000..b1156afdb691 --- /dev/null +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_size.c @@ -0,0 +1,77 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2019 ARM Limited
• • Place a fake sigframe on the stack including a bad record overflowing
• • the __reserved space: on sigreturn Kernel must spot this attempt and
• • the test case is expected to be terminated via SEGV.
• */

+#include <signal.h> +#include <ucontext.h>

+#include "test_signals_utils.h" +#include "testcases.h"

+struct fake_sigframe sf;

+#define MIN_SZ_ALIGN 16

+static int fake_sigreturn_bad_size_run(struct tdescr *td,

• 		       siginfo_t *si, ucontext_t *uc)
  

+{

• size_t resv_sz, need_sz, offset;
• struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
• /* just to fill the ucontext_t with something real */
• if (!get_current_context(td, &sf.uc))

• return 1;
  

• resv_sz = GET_SF_RESV_SIZE(sf);
• /* at least HDR_SZ + bad sized esr_context needed */
• need_sz = HDR_SZ + sizeof(struct esr_context);

Nit: can we write this sum the other way round (see comment on patch 9)?