diff mbox series

[3/4] kselftests/arm64: add PAuth test for whether exec() changes keys

Message ID 20200828131606.7946-4-boyan.karatotev@arm.com
State New
Headers show
Series kselftests/arm64: add PAuth tests | expand

Commit Message

Boyan Karatotev Aug. 28, 2020, 1:16 p.m. UTC
Kernel documentation states that it will change PAuth keys on exec() calls.

Verify that all keys are correctly switched to new ones.

Cc: Shuah Khan <shuah@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
---
 tools/testing/selftests/arm64/pauth/Makefile  |   4 +
 .../selftests/arm64/pauth/exec_target.c       |  35 +++++
 tools/testing/selftests/arm64/pauth/helper.h  |  10 ++
 tools/testing/selftests/arm64/pauth/pac.c     | 148 ++++++++++++++++++
 4 files changed, 197 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c

Comments

Vincenzo Frascino Aug. 28, 2020, 2:33 p.m. UTC | #1
On 8/28/20 2:16 PM, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
> 
> Verify that all keys are correctly switched to new ones.
> 

Reviewed-by: Vincenzo Frascino <Vincenzo.Frascino@arm.com>

> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> ---
>  tools/testing/selftests/arm64/pauth/Makefile  |   4 +
>  .../selftests/arm64/pauth/exec_target.c       |  35 +++++
>  tools/testing/selftests/arm64/pauth/helper.h  |  10 ++
>  tools/testing/selftests/arm64/pauth/pac.c     | 148 ++++++++++++++++++
>  4 files changed, 197 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
> 
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>  
>  TEST_GEN_PROGS := pac
>  TEST_GEN_FILES := pac_corruptor.o helper.o
> +TEST_GEN_PROGS_EXTENDED := exec_target
>  
>  include ../../lib.mk
>  
> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
>  # greater, gcc emits pac* instructions which are not in HINT NOP space,
>  # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
>  # run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
> +	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
>  $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
>  	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>  
> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
> new file mode 100644
> index 000000000000..07addef5a1d7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/auxv.h>
> +
> +#include "helper.h"
> +
> +
> +int main(void)
> +{
> +	struct signatures signed_vals;
> +	unsigned long hwcaps;
> +	size_t val;
> +
> +	fread(&val, sizeof(size_t), 1, stdin);
> +
> +	/* don't try to execute illegal (unimplemented) instructions) caller
> +	 * should have checked this and keep worker simple
> +	 */
> +	hwcaps = getauxval(AT_HWCAP);
> +
> +	if (hwcaps & HWCAP_PACA) {
> +		signed_vals.keyia = keyia_sign(val);
> +		signed_vals.keyib = keyib_sign(val);
> +		signed_vals.keyda = keyda_sign(val);
> +		signed_vals.keydb = keydb_sign(val);
> +	}
> +	signed_vals.keyg = (hwcaps & HWCAP_PACG) ?  keyg_sign(val) : 0;
> +
> +	fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
> +
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index b3cf709e249d..fceaa1e4824a 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -6,6 +6,16 @@
>  
>  #include <stdlib.h>
>  
> +#define NKEYS 5
> +
> +
> +struct signatures {
> +	size_t keyia;
> +	size_t keyib;
> +	size_t keyda;
> +	size_t keydb;
> +	size_t keyg;
> +};
>  
>  void pac_corruptor(void);
>  
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index cdbffa8bf61e..16dea47b11c7 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -2,6 +2,8 @@
>  // Copyright (C) 2020 ARM Limited
>  
>  #include <sys/auxv.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
>  #include <signal.h>
>  
>  #include "../../kselftest_harness.h"
> @@ -33,6 +35,117 @@ do { \
>  } while (0)
>  
>  
> +void sign_specific(struct signatures *sign, size_t val)
> +{
> +	sign->keyia = keyia_sign(val);
> +	sign->keyib = keyib_sign(val);
> +	sign->keyda = keyda_sign(val);
> +	sign->keydb = keydb_sign(val);
> +}
> +
> +void sign_all(struct signatures *sign, size_t val)
> +{
> +	sign->keyia = keyia_sign(val);
> +	sign->keyib = keyib_sign(val);
> +	sign->keyda = keyda_sign(val);
> +	sign->keydb = keydb_sign(val);
> +	sign->keyg  = keyg_sign(val);
> +}
> +
> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
> +{
> +	int res = 0;
> +
> +	res |= old->keyia == new->keyia;
> +	res |= old->keyib == new->keyib;
> +	res |= old->keyda == new->keyda;
> +	res |= old->keydb == new->keydb;
> +	if (nkeys == NKEYS)
> +		res |= old->keyg  == new->keyg;
> +
> +	return res;
> +}
> +
> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> +{
> +	int new_stdin[2];
> +	int new_stdout[2];
> +	int status;
> +	ssize_t ret;
> +	pid_t pid;
> +
> +	ret = pipe(new_stdin);
> +	if (ret == -1) {
> +		perror("pipe returned error");
> +		return -1;
> +	}
> +
> +	ret = pipe(new_stdout);
> +	if (ret == -1) {
> +		perror("pipe returned error");
> +		return -1;
> +	}
> +
> +	pid = fork();
> +	// child
> +	if (pid == 0) {
> +		dup2(new_stdin[0], STDIN_FILENO);
> +		if (ret == -1) {
> +			perror("dup2 returned error");
> +			exit(1);
> +		}
> +
> +		dup2(new_stdout[1], STDOUT_FILENO);
> +		if (ret == -1) {
> +			perror("dup2 returned error");
> +			exit(1);
> +		}
> +
> +		close(new_stdin[0]);
> +		close(new_stdin[1]);
> +		close(new_stdout[0]);
> +		close(new_stdout[1]);
> +
> +		ret = execl("exec_target", "exec_target", (char *) NULL);
> +		if (ret == -1) {
> +			perror("exec returned error");
> +			exit(1);
> +		}
> +	}
> +
> +	close(new_stdin[0]);
> +	close(new_stdout[1]);
> +
> +	ret = write(new_stdin[1], &val, sizeof(size_t));
> +	if (ret == -1) {
> +		perror("write returned error");
> +		return -1;
> +	}
> +
> +	/*
> +	 * wait for the worker to finish, so that read() reads all data
> +	 * will also context switch with worker so that this function can be used
> +	 * for context switch tests
> +	 */
> +	waitpid(pid, &status, 0);
> +	if (WIFEXITED(status) == 0) {
> +		fprintf(stderr, "worker exited unexpectedly\n");
> +		return -1;
> +	}
> +	if (WEXITSTATUS(status) != 0) {
> +		fprintf(stderr, "worker exited with error\n");
> +		return -1;
> +	}
> +
> +	ret = read(new_stdout[0], signed_vals, sizeof(struct signatures));
> +	if (ret == -1) {
> +		perror("read returned error");
> +		return -1;
> +	}
> +
> +	return 0;
> +}
> +
>  /* check that a corrupted PAC results in SIGSEGV */
>  TEST_SIGNAL(corrupt_pac, SIGSEGV)
>  {
> @@ -79,5 +192,40 @@ TEST(pac_instructions_not_nop_generic)
>  	ASSERT_NE(0, keyg)  TH_LOG("keyg instructions did nothing");
>  }
>  
> +/*
> + * fork() does not change keys. Only exec() does so call a worker program.
> + * Its only job is to sign a value and report back the resutls
> + */
> +TEST(exec_unique_keys)
> +{
> +	struct signatures new_keys;
> +	struct signatures old_keys;
> +	int ret;
> +	int different = 0;
> +	int nkeys = NKEYS;
> +	unsigned long hwcaps = getauxval(AT_HWCAP);
> +
> +	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
> +	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
> +	if (!(hwcaps & HWCAP_PACG)) {
> +		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
> +		nkeys = NKEYS - 1;
> +	}
> +
> +	for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
> +		ret = exec_sign_all(&new_keys, i);
> +		ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
> +
> +		if (nkeys == NKEYS)
> +			sign_all(&old_keys, i);
> +		else
> +			sign_specific(&old_keys, i);
> +
> +		different |= !are_same(&old_keys, &new_keys, nkeys);
> +	}
> +
> +	ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
> +}
> +
>  TEST_HARNESS_MAIN
>  
>
Amit Kachhap Aug. 31, 2020, 8:13 a.m. UTC | #2
On 8/28/20 6:46 PM, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
> 
> Verify that all keys are correctly switched to new ones.
> 
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>

The changes look fine so,
Reviewed-by: Amit Daniel Kachhap <amit.kachhap@arm.com>

> ---
>   tools/testing/selftests/arm64/pauth/Makefile  |   4 +
>   .../selftests/arm64/pauth/exec_target.c       |  35 +++++
>   tools/testing/selftests/arm64/pauth/helper.h  |  10 ++
>   tools/testing/selftests/arm64/pauth/pac.c     | 148 ++++++++++++++++++
>   4 files changed, 197 insertions(+)
>   create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
> 
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>
Dave Martin Sept. 2, 2020, 5 p.m. UTC | #3
On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
> Kernel documentation states that it will change PAuth keys on exec() calls.
> 
> Verify that all keys are correctly switched to new ones.
> 
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> ---
>  tools/testing/selftests/arm64/pauth/Makefile  |   4 +
>  .../selftests/arm64/pauth/exec_target.c       |  35 +++++
>  tools/testing/selftests/arm64/pauth/helper.h  |  10 ++
>  tools/testing/selftests/arm64/pauth/pac.c     | 148 ++++++++++++++++++
>  4 files changed, 197 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
> 
> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
> index a017d1c8dd58..2e237b21ccf6 100644
> --- a/tools/testing/selftests/arm64/pauth/Makefile
> +++ b/tools/testing/selftests/arm64/pauth/Makefile
> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>  
>  TEST_GEN_PROGS := pac
>  TEST_GEN_FILES := pac_corruptor.o helper.o
> +TEST_GEN_PROGS_EXTENDED := exec_target
>  
>  include ../../lib.mk
>  
> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
>  # greater, gcc emits pac* instructions which are not in HINT NOP space,
>  # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
>  # run on earlier targets and print a meaningful error messages
> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
> +	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
> +
>  $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
>  	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>  
> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
> new file mode 100644
> index 000000000000..07addef5a1d7
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) 2020 ARM Limited
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/auxv.h>
> +
> +#include "helper.h"
> +
> +
> +int main(void)
> +{
> +	struct signatures signed_vals;
> +	unsigned long hwcaps;
> +	size_t val;
> +
> +	fread(&val, sizeof(size_t), 1, stdin);
> +
> +	/* don't try to execute illegal (unimplemented) instructions) caller
> +	 * should have checked this and keep worker simple
> +	 */
> +	hwcaps = getauxval(AT_HWCAP);
> +
> +	if (hwcaps & HWCAP_PACA) {
> +		signed_vals.keyia = keyia_sign(val);
> +		signed_vals.keyib = keyib_sign(val);
> +		signed_vals.keyda = keyda_sign(val);
> +		signed_vals.keydb = keydb_sign(val);
> +	}
> +	signed_vals.keyg = (hwcaps & HWCAP_PACG) ?  keyg_sign(val) : 0;
> +
> +	fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
> +
> +	return 0;
> +}
> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
> index b3cf709e249d..fceaa1e4824a 100644
> --- a/tools/testing/selftests/arm64/pauth/helper.h
> +++ b/tools/testing/selftests/arm64/pauth/helper.h
> @@ -6,6 +6,16 @@
>  
>  #include <stdlib.h>
>  
> +#define NKEYS 5
> +
> +
> +struct signatures {
> +	size_t keyia;
> +	size_t keyib;
> +	size_t keyda;
> +	size_t keydb;
> +	size_t keyg;
> +};
>  
>  void pac_corruptor(void);
>  
> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> index cdbffa8bf61e..16dea47b11c7 100644
> --- a/tools/testing/selftests/arm64/pauth/pac.c
> +++ b/tools/testing/selftests/arm64/pauth/pac.c
> @@ -2,6 +2,8 @@
>  // Copyright (C) 2020 ARM Limited
>  
>  #include <sys/auxv.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
>  #include <signal.h>
>  
>  #include "../../kselftest_harness.h"
> @@ -33,6 +35,117 @@ do { \
>  } while (0)
>  
>  
> +void sign_specific(struct signatures *sign, size_t val)
> +{
> +	sign->keyia = keyia_sign(val);
> +	sign->keyib = keyib_sign(val);
> +	sign->keyda = keyda_sign(val);
> +	sign->keydb = keydb_sign(val);
> +}
> +
> +void sign_all(struct signatures *sign, size_t val)
> +{
> +	sign->keyia = keyia_sign(val);
> +	sign->keyib = keyib_sign(val);
> +	sign->keyda = keyda_sign(val);
> +	sign->keydb = keydb_sign(val);
> +	sign->keyg  = keyg_sign(val);
> +}
> +
> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
> +{
> +	int res = 0;
> +
> +	res |= old->keyia == new->keyia;
> +	res |= old->keyib == new->keyib;
> +	res |= old->keyda == new->keyda;
> +	res |= old->keydb == new->keydb;
> +	if (nkeys == NKEYS)
> +		res |= old->keyg  == new->keyg;
> +
> +	return res;
> +}
> +
> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> +{

Could popen(3) be used here?

Fork-and-exec is notoriously fiddly, so it's preferable to use a library
function to do it where applicable.

[...]

Cheers
---Dave
Boyan Karatotev Sept. 3, 2020, 10:20 a.m. UTC | #4
On 02/09/2020 18:00, Dave Martin wrote:
> On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
>> Kernel documentation states that it will change PAuth keys on exec() calls.
>>
>> Verify that all keys are correctly switched to new ones.
>>
>> Cc: Shuah Khan <shuah@kernel.org>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
>> ---
>>  tools/testing/selftests/arm64/pauth/Makefile  |   4 +
>>  .../selftests/arm64/pauth/exec_target.c       |  35 +++++
>>  tools/testing/selftests/arm64/pauth/helper.h  |  10 ++
>>  tools/testing/selftests/arm64/pauth/pac.c     | 148 ++++++++++++++++++
>>  4 files changed, 197 insertions(+)
>>  create mode 100644 tools/testing/selftests/arm64/pauth/exec_target.c
>>
>> diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
>> index a017d1c8dd58..2e237b21ccf6 100644
>> --- a/tools/testing/selftests/arm64/pauth/Makefile
>> +++ b/tools/testing/selftests/arm64/pauth/Makefile
>> @@ -5,6 +5,7 @@ CFLAGS += -mbranch-protection=pac-ret
>>  
>>  TEST_GEN_PROGS := pac
>>  TEST_GEN_FILES := pac_corruptor.o helper.o
>> +TEST_GEN_PROGS_EXTENDED := exec_target
>>  
>>  include ../../lib.mk
>>  
>> @@ -20,6 +21,9 @@ $(OUTPUT)/helper.o: helper.c
>>  # greater, gcc emits pac* instructions which are not in HINT NOP space,
>>  # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
>>  # run on earlier targets and print a meaningful error messages
>> +$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
>> +	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>> +
>>  $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
>>  	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
>>  
>> diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
>> new file mode 100644
>> index 000000000000..07addef5a1d7
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/pauth/exec_target.c
>> @@ -0,0 +1,35 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +// Copyright (C) 2020 ARM Limited
>> +
>> +#include <stdio.h>
>> +#include <stdlib.h>
>> +#include <sys/auxv.h>
>> +
>> +#include "helper.h"
>> +
>> +
>> +int main(void)
>> +{
>> +	struct signatures signed_vals;
>> +	unsigned long hwcaps;
>> +	size_t val;
>> +
>> +	fread(&val, sizeof(size_t), 1, stdin);
>> +
>> +	/* don't try to execute illegal (unimplemented) instructions) caller
>> +	 * should have checked this and keep worker simple
>> +	 */
>> +	hwcaps = getauxval(AT_HWCAP);
>> +
>> +	if (hwcaps & HWCAP_PACA) {
>> +		signed_vals.keyia = keyia_sign(val);
>> +		signed_vals.keyib = keyib_sign(val);
>> +		signed_vals.keyda = keyda_sign(val);
>> +		signed_vals.keydb = keydb_sign(val);
>> +	}
>> +	signed_vals.keyg = (hwcaps & HWCAP_PACG) ?  keyg_sign(val) : 0;
>> +
>> +	fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
>> +
>> +	return 0;
>> +}
>> diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
>> index b3cf709e249d..fceaa1e4824a 100644
>> --- a/tools/testing/selftests/arm64/pauth/helper.h
>> +++ b/tools/testing/selftests/arm64/pauth/helper.h
>> @@ -6,6 +6,16 @@
>>  
>>  #include <stdlib.h>
>>  
>> +#define NKEYS 5
>> +
>> +
>> +struct signatures {
>> +	size_t keyia;
>> +	size_t keyib;
>> +	size_t keyda;
>> +	size_t keydb;
>> +	size_t keyg;
>> +};
>>  
>>  void pac_corruptor(void);
>>  
>> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
>> index cdbffa8bf61e..16dea47b11c7 100644
>> --- a/tools/testing/selftests/arm64/pauth/pac.c
>> +++ b/tools/testing/selftests/arm64/pauth/pac.c
>> @@ -2,6 +2,8 @@
>>  // Copyright (C) 2020 ARM Limited
>>  
>>  #include <sys/auxv.h>
>> +#include <sys/types.h>
>> +#include <sys/wait.h>
>>  #include <signal.h>
>>  
>>  #include "../../kselftest_harness.h"
>> @@ -33,6 +35,117 @@ do { \
>>  } while (0)
>>  
>>  
>> +void sign_specific(struct signatures *sign, size_t val)
>> +{
>> +	sign->keyia = keyia_sign(val);
>> +	sign->keyib = keyib_sign(val);
>> +	sign->keyda = keyda_sign(val);
>> +	sign->keydb = keydb_sign(val);
>> +}
>> +
>> +void sign_all(struct signatures *sign, size_t val)
>> +{
>> +	sign->keyia = keyia_sign(val);
>> +	sign->keyib = keyib_sign(val);
>> +	sign->keyda = keyda_sign(val);
>> +	sign->keydb = keydb_sign(val);
>> +	sign->keyg  = keyg_sign(val);
>> +}
>> +
>> +int are_same(struct signatures *old, struct signatures *new, int nkeys)
>> +{
>> +	int res = 0;
>> +
>> +	res |= old->keyia == new->keyia;
>> +	res |= old->keyib == new->keyib;
>> +	res |= old->keyda == new->keyda;
>> +	res |= old->keydb == new->keydb;
>> +	if (nkeys == NKEYS)
>> +		res |= old->keyg  == new->keyg;
>> +
>> +	return res;
>> +}
>> +
>> +int exec_sign_all(struct signatures *signed_vals, size_t val)
>> +{
> 
> Could popen(3) be used here?
> 
> Fork-and-exec is notoriously fiddly, so it's preferable to use a library
> function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
only gives a unidirectional stream.
> 
> [...]
> 
> Cheers
> ---Dave
>
Dave Martin Sept. 7, 2020, 10:27 a.m. UTC | #5
On Thu, Sep 03, 2020 at 11:20:25AM +0100, Boyan Karatotev wrote:
> On 02/09/2020 18:00, Dave Martin wrote:
> > On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
> >> Kernel documentation states that it will change PAuth keys on exec() calls.
> >>
> >> Verify that all keys are correctly switched to new ones.
> >>
> >> Cc: Shuah Khan <shuah@kernel.org>
> >> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >> Cc: Will Deacon <will@kernel.org>
> >> Signed-off-by: Boyan Karatotev <boyan.karatotev@arm.com>
> >> ---

[...]

> >> diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
> >> index cdbffa8bf61e..16dea47b11c7 100644
> >> --- a/tools/testing/selftests/arm64/pauth/pac.c
> >> +++ b/tools/testing/selftests/arm64/pauth/pac.c

[...]

> >> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> >> +{
> > 
> > Could popen(3) be used here?
> > 
> > Fork-and-exec is notoriously fiddly, so it's preferable to use a library
> > function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
> only gives a unidirectional stream.

Ah, fair point.

Would it help if you created an additional pipe before calling popen()?

May not be worth it, though.  For one thing, wiring that extra pipe to
stdin or stdout in the child process would require some extra work...

Cheers
---Dave
Boyan Karatotev Sept. 15, 2020, 3:18 p.m. UTC | #6
On 07/09/2020 11:27 am, Dave Martin wrote:
> On Thu, Sep 03, 2020 at 11:20:25AM +0100, Boyan Karatotev wrote:
>> On 02/09/2020 18:00, Dave Martin wrote:
>>> On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
>>>> +int exec_sign_all(struct signatures *signed_vals, size_t val)
>>>> +{
>>>
>>> Could popen(3) be used here?
>>>
>>> Fork-and-exec is notoriously fiddly, so it's preferable to use a library
>>> function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
>> only gives a unidirectional stream.
> 
> Ah, fair point.
> 
> Would it help if you created an additional pipe before calling popen()?
> 
> May not be worth it, though.  For one thing, wiring that extra pipe to
> stdin or stdout in the child process would require some extra work...
Well, I probably could, but I doubt the result would be any better. I
agree that I'm not sure the effort is worth it and would rather keep it
the same.
Dave Martin Sept. 16, 2020, 3:38 p.m. UTC | #7
On Tue, Sep 15, 2020 at 04:18:28PM +0100, Boyan Karatotev wrote:
> On 07/09/2020 11:27 am, Dave Martin wrote:
> > On Thu, Sep 03, 2020 at 11:20:25AM +0100, Boyan Karatotev wrote:
> >> On 02/09/2020 18:00, Dave Martin wrote:
> >>> On Fri, Aug 28, 2020 at 02:16:05PM +0100, Boyan Karatotev wrote:
> >>>> +int exec_sign_all(struct signatures *signed_vals, size_t val)
> >>>> +{
> >>>
> >>> Could popen(3) be used here?
> >>>
> >>> Fork-and-exec is notoriously fiddly, so it's preferable to use a library
> >>> function to do it where applicable.I would love to, but the worker needs a bidirectional channel and popen
> >> only gives a unidirectional stream.
> > 
> > Ah, fair point.
> > 
> > Would it help if you created an additional pipe before calling popen()?
> > 
> > May not be worth it, though.  For one thing, wiring that extra pipe to
> > stdin or stdout in the child process would require some extra work...
> Well, I probably could, but I doubt the result would be any better. I
> agree that I'm not sure the effort is worth it and would rather keep it
> the same.

Sure, fair enough.

Ideally kselftest would provide some common code for this sort of thing,
but I guess that's a separate discussion.

Cheers
---Dave
diff mbox series

Patch

diff --git a/tools/testing/selftests/arm64/pauth/Makefile b/tools/testing/selftests/arm64/pauth/Makefile
index a017d1c8dd58..2e237b21ccf6 100644
--- a/tools/testing/selftests/arm64/pauth/Makefile
+++ b/tools/testing/selftests/arm64/pauth/Makefile
@@ -5,6 +5,7 @@  CFLAGS += -mbranch-protection=pac-ret
 
 TEST_GEN_PROGS := pac
 TEST_GEN_FILES := pac_corruptor.o helper.o
+TEST_GEN_PROGS_EXTENDED := exec_target
 
 include ../../lib.mk
 
@@ -20,6 +21,9 @@  $(OUTPUT)/helper.o: helper.c
 # greater, gcc emits pac* instructions which are not in HINT NOP space,
 # preventing the tests from occurring at all. Compile for ARMv8.2 so tests can
 # run on earlier targets and print a meaningful error messages
+$(OUTPUT)/exec_target: exec_target.c $(OUTPUT)/helper.o
+	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
+
 $(OUTPUT)/pac: pac.c $(OUTPUT)/pac_corruptor.o $(OUTPUT)/helper.o
 	$(CC) $^ -o $@ $(CFLAGS) -march=armv8.2-a
 
diff --git a/tools/testing/selftests/arm64/pauth/exec_target.c b/tools/testing/selftests/arm64/pauth/exec_target.c
new file mode 100644
index 000000000000..07addef5a1d7
--- /dev/null
+++ b/tools/testing/selftests/arm64/pauth/exec_target.c
@@ -0,0 +1,35 @@ 
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/auxv.h>
+
+#include "helper.h"
+
+
+int main(void)
+{
+	struct signatures signed_vals;
+	unsigned long hwcaps;
+	size_t val;
+
+	fread(&val, sizeof(size_t), 1, stdin);
+
+	/* don't try to execute illegal (unimplemented) instructions) caller
+	 * should have checked this and keep worker simple
+	 */
+	hwcaps = getauxval(AT_HWCAP);
+
+	if (hwcaps & HWCAP_PACA) {
+		signed_vals.keyia = keyia_sign(val);
+		signed_vals.keyib = keyib_sign(val);
+		signed_vals.keyda = keyda_sign(val);
+		signed_vals.keydb = keydb_sign(val);
+	}
+	signed_vals.keyg = (hwcaps & HWCAP_PACG) ?  keyg_sign(val) : 0;
+
+	fwrite(&signed_vals, sizeof(struct signatures), 1, stdout);
+
+	return 0;
+}
diff --git a/tools/testing/selftests/arm64/pauth/helper.h b/tools/testing/selftests/arm64/pauth/helper.h
index b3cf709e249d..fceaa1e4824a 100644
--- a/tools/testing/selftests/arm64/pauth/helper.h
+++ b/tools/testing/selftests/arm64/pauth/helper.h
@@ -6,6 +6,16 @@ 
 
 #include <stdlib.h>
 
+#define NKEYS 5
+
+
+struct signatures {
+	size_t keyia;
+	size_t keyib;
+	size_t keyda;
+	size_t keydb;
+	size_t keyg;
+};
 
 void pac_corruptor(void);
 
diff --git a/tools/testing/selftests/arm64/pauth/pac.c b/tools/testing/selftests/arm64/pauth/pac.c
index cdbffa8bf61e..16dea47b11c7 100644
--- a/tools/testing/selftests/arm64/pauth/pac.c
+++ b/tools/testing/selftests/arm64/pauth/pac.c
@@ -2,6 +2,8 @@ 
 // Copyright (C) 2020 ARM Limited
 
 #include <sys/auxv.h>
+#include <sys/types.h>
+#include <sys/wait.h>
 #include <signal.h>
 
 #include "../../kselftest_harness.h"
@@ -33,6 +35,117 @@  do { \
 } while (0)
 
 
+void sign_specific(struct signatures *sign, size_t val)
+{
+	sign->keyia = keyia_sign(val);
+	sign->keyib = keyib_sign(val);
+	sign->keyda = keyda_sign(val);
+	sign->keydb = keydb_sign(val);
+}
+
+void sign_all(struct signatures *sign, size_t val)
+{
+	sign->keyia = keyia_sign(val);
+	sign->keyib = keyib_sign(val);
+	sign->keyda = keyda_sign(val);
+	sign->keydb = keydb_sign(val);
+	sign->keyg  = keyg_sign(val);
+}
+
+int are_same(struct signatures *old, struct signatures *new, int nkeys)
+{
+	int res = 0;
+
+	res |= old->keyia == new->keyia;
+	res |= old->keyib == new->keyib;
+	res |= old->keyda == new->keyda;
+	res |= old->keydb == new->keydb;
+	if (nkeys == NKEYS)
+		res |= old->keyg  == new->keyg;
+
+	return res;
+}
+
+int exec_sign_all(struct signatures *signed_vals, size_t val)
+{
+	int new_stdin[2];
+	int new_stdout[2];
+	int status;
+	ssize_t ret;
+	pid_t pid;
+
+	ret = pipe(new_stdin);
+	if (ret == -1) {
+		perror("pipe returned error");
+		return -1;
+	}
+
+	ret = pipe(new_stdout);
+	if (ret == -1) {
+		perror("pipe returned error");
+		return -1;
+	}
+
+	pid = fork();
+	// child
+	if (pid == 0) {
+		dup2(new_stdin[0], STDIN_FILENO);
+		if (ret == -1) {
+			perror("dup2 returned error");
+			exit(1);
+		}
+
+		dup2(new_stdout[1], STDOUT_FILENO);
+		if (ret == -1) {
+			perror("dup2 returned error");
+			exit(1);
+		}
+
+		close(new_stdin[0]);
+		close(new_stdin[1]);
+		close(new_stdout[0]);
+		close(new_stdout[1]);
+
+		ret = execl("exec_target", "exec_target", (char *) NULL);
+		if (ret == -1) {
+			perror("exec returned error");
+			exit(1);
+		}
+	}
+
+	close(new_stdin[0]);
+	close(new_stdout[1]);
+
+	ret = write(new_stdin[1], &val, sizeof(size_t));
+	if (ret == -1) {
+		perror("write returned error");
+		return -1;
+	}
+
+	/*
+	 * wait for the worker to finish, so that read() reads all data
+	 * will also context switch with worker so that this function can be used
+	 * for context switch tests
+	 */
+	waitpid(pid, &status, 0);
+	if (WIFEXITED(status) == 0) {
+		fprintf(stderr, "worker exited unexpectedly\n");
+		return -1;
+	}
+	if (WEXITSTATUS(status) != 0) {
+		fprintf(stderr, "worker exited with error\n");
+		return -1;
+	}
+
+	ret = read(new_stdout[0], signed_vals, sizeof(struct signatures));
+	if (ret == -1) {
+		perror("read returned error");
+		return -1;
+	}
+
+	return 0;
+}
+
 /* check that a corrupted PAC results in SIGSEGV */
 TEST_SIGNAL(corrupt_pac, SIGSEGV)
 {
@@ -79,5 +192,40 @@  TEST(pac_instructions_not_nop_generic)
 	ASSERT_NE(0, keyg)  TH_LOG("keyg instructions did nothing");
 }
 
+/*
+ * fork() does not change keys. Only exec() does so call a worker program.
+ * Its only job is to sign a value and report back the resutls
+ */
+TEST(exec_unique_keys)
+{
+	struct signatures new_keys;
+	struct signatures old_keys;
+	int ret;
+	int different = 0;
+	int nkeys = NKEYS;
+	unsigned long hwcaps = getauxval(AT_HWCAP);
+
+	/* generic and data key instructions are not in NOP space. This prevents a SIGILL */
+	ASSERT_NE(0, hwcaps & HWCAP_PACA) TH_LOG("PAUTH not enabled");
+	if (!(hwcaps & HWCAP_PACG)) {
+		TH_LOG("WARNING: Generic PAUTH not enabled. Skipping generic key checks");
+		nkeys = NKEYS - 1;
+	}
+
+	for (int i = 0; i < PAC_COLLISION_ATTEMPTS; i++) {
+		ret = exec_sign_all(&new_keys, i);
+		ASSERT_EQ(0, ret) TH_LOG("failed to run worker");
+
+		if (nkeys == NKEYS)
+			sign_all(&old_keys, i);
+		else
+			sign_specific(&old_keys, i);
+
+		different |= !are_same(&old_keys, &new_keys, nkeys);
+	}
+
+	ASSERT_EQ(1, different) TH_LOG("exec() did not change keys");
+}
+
 TEST_HARNESS_MAIN