diff mbox series

[10/13] kselftest: arm64: fake_sigreturn_bad_magic

Message ID 20190613111335.7645-11-cristian.marussi@arm.com (mailing list archive)
State New, archived
Headers show
Series Add arm64/signal initial kselftest support | expand

Commit Message

Cristian Marussi June 13, 2019, 11:13 a.m. UTC 
Added 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.

Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
---
 .../arm64/signal/testcases/.gitignore         |  1 +
 .../testcases/fake_sigreturn_bad_magic.c      | 42 +++++++++++++++++++
 2 files changed, 43 insertions(+)
 create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c

Comments

Dave Martin June 21, 2019, 10:36 a.m. UTC | #1 
On Thu, Jun 13, 2019 at 12:13:32PM +0100, Cristian Marussi wrote:
> Added 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.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../arm64/signal/testcases/.gitignore         |  1 +
>  .../testcases/fake_sigreturn_bad_magic.c      | 42 +++++++++++++++++++
>  2 files changed, 43 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
> 
> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> index 3e6b26be6727..c353b7bd899d 100644
> --- a/tools/testing/selftests/arm64/signal/testcases/.gitignore
> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> @@ -7,3 +7,4 @@ mangle_pstate_invalid_mode_el2
>  mangle_pstate_invalid_mode_el3
>  mangle_pstate_ssbs_regs
>  fake_sigreturn_misaligned
> +fake_sigreturn_bad_magic
> 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..de81ea10393f
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2019 ARM Limited */
> +
> +#include <asm/sigcontext.h>
> +#include <ucontext.h>
> +#include <stdio.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +struct a_sigframe sf;
> +
> +static int fake_sigreturn_bad_magic_run(struct tdescr *td,
> +					siginfo_t *si, ucontext_t *uc)
> +{
> +	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;
> +
> +	/* find the terminator, preserving existig headers */
> +	head = get_terminator(head, GET_SF_RESV_SIZE(sf), NULL);
> +	if (head) {
> +		head->magic = 0xdeadbeef;
> +		head->size = 128;

How can we be sure this won't overrun the end of the sigframe, once
the new terminator is appended?

If there is extra_context in the frame, the frame is only as large as
needed, so adding another record will always overrun the size of the
frame in that case.

I suggest splitting this test into a few cases, something like the
following.

(Note, some of these are already covered by your tests.  I'm just trying
to give the broad view here.)

1) Add a bogus record that doesn't overrun the frame.

2) Add a bogus record that does overrun the frame.

3) Add a record with a bogus size (i.e., not a multiple of 16 bytes, or
smaller than _aarch64_ctx).

3) Hack the size field of an existing record so that it overruns the
frame.  Thee are two subcases here: a record big enough that the next
_aarch64_ctx doesn't fit (probably already tested by
fake_sigreturn_overflow_reserved), and a record that overruns the frame
all by itself (tested by fake_sigreturn_bad_size, but it would be good
to check the exact boundary condition -- see my comments on that patch).

When checking that the kernel rejects an _aarch64_ctx header that
overruns the end of the frame, we should nonetheless write data for it
that looks valid, so that we know the kernel is rejecting it because of
the overrun and not because the header contents are invalid.

4) Delete a required record from the frame (say, fpsimd_context).
Or just delete everything.


For this series, maybe just do (1): this test nearly does that.

If there is extra_context, we can delete it and replace it with our
bogus record: this can't overrun if the bogus record's size is no bigger
than extra_context.

We should have tests to cover the other cases, but they can be TODO for
now.

[...]

Cheers
---Dave
Cristian Marussi July 3, 2019, 5:41 p.m. UTC | #2 
Hi Dave

a few updates related to v2 of this patchset

On 6/21/19 11:36 AM, Dave Martin wrote:
> On Thu, Jun 13, 2019 at 12:13:32PM +0100, Cristian Marussi wrote:
>> Added 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.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>>   .../arm64/signal/testcases/.gitignore         |  1 +
>>   .../testcases/fake_sigreturn_bad_magic.c      | 42 +++++++++++++++++++
>>   2 files changed, 43 insertions(+)
>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> index 3e6b26be6727..c353b7bd899d 100644
>> --- a/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> @@ -7,3 +7,4 @@ mangle_pstate_invalid_mode_el2
>>   mangle_pstate_invalid_mode_el3
>>   mangle_pstate_ssbs_regs
>>   fake_sigreturn_misaligned
>> +fake_sigreturn_bad_magic
>> 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..de81ea10393f
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
>> @@ -0,0 +1,42 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#include <asm/sigcontext.h>
>> +#include <ucontext.h>
>> +#include <stdio.h>
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +struct a_sigframe sf;
>> +
>> +static int fake_sigreturn_bad_magic_run(struct tdescr *td,
>> +					siginfo_t *si, ucontext_t *uc)
>> +{
>> +	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;
>> +
>> +	/* find the terminator, preserving existig headers */
>> +	head = get_terminator(head, GET_SF_RESV_SIZE(sf), NULL);
>> +	if (head) {
>> +		head->magic = 0xdeadbeef;
>> +		head->size = 128;
> 
> How can we be sure this won't overrun the end of the sigframe, once
> the new terminator is appended?
> 
> If there is extra_context in the frame, the frame is only as large as
> needed, so adding another record will always overrun the size of the
> frame in that case.

I underestimated this, since extra is currently never found and the 
__reserved area is mostly empty.

I have fixed in these regards some tests in V2, taking care to verify 
the available space and throwing away EXTRA to make room as advised.

> 
> I suggest splitting this test into a few cases, something like the
> following.
> 
> (Note, some of these are already covered by your tests.  I'm just trying
> to give the broad view here.)
> 
> 1) Add a bogus record that doesn't overrun the frame.
> 
> 2) Add a bogus record that does overrun the frame.
> 
> 3) Add a record with a bogus size (i.e., not a multiple of 16 bytes, or
> smaller than _aarch64_ctx).

These are tests TBD...in V2 I did something similar with wrong sizes and 
aligments (and overrun) but not full cases. I'll add something or some 
TODO notes in V3.

> 
> 3) Hack the size field of an existing record so that it overruns the
> frame.  Thee are two subcases here: a record big enough that the next
> _aarch64_ctx doesn't fit (probably already tested by
> fake_sigreturn_overflow_reserved),
fake_sigreturn_overflow_reserved has been removed in V2, will fix and 
re-enable in V3

and a record that overruns the frame
> all by itself (tested by fake_sigreturn_bad_size, but it would be good
> to check the exact boundary condition -- see my comments on that patch).

This should have been fixed now in fake_sigreturn_bad_size following 
advice (hopefuly)
> 
> When checking that the kernel rejects an _aarch64_ctx header that
> overruns the end of the frame, we should nonetheless write data for it
> that looks valid, so that we know the kernel is rejecting it because of
> the overrun and not because the header contents are invalid.
> 
> 4) Delete a required record from the frame (say, fpsimd_context).
> Or just delete everything.
> 
2 tests added in V2 about fpsimd: mising and duplicated

> 
> For this series, maybe just do (1): this test nearly does that.
> 
> If there is extra_context, we can delete it and replace it with our
> bogus record: this can't overrun if the bogus record's size is no bigger
> than extra_context.
> 
Followed this approach in v2
> We should have tests to cover the other cases, but they can be TODO for
> now.
> 
> [...]
> 
> Cheers
> ---Dave
> 

Cheers

Cristian
diff mbox series

Patch

diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
index 3e6b26be6727..c353b7bd899d 100644
--- a/tools/testing/selftests/arm64/signal/testcases/.gitignore
+++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
@@ -7,3 +7,4 @@  mangle_pstate_invalid_mode_el2
 mangle_pstate_invalid_mode_el3
 mangle_pstate_ssbs_regs
 fake_sigreturn_misaligned
+fake_sigreturn_bad_magic
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..de81ea10393f
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_bad_magic.c
@@ -0,0 +1,42 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019 ARM Limited */
+
+#include <asm/sigcontext.h>
+#include <ucontext.h>
+#include <stdio.h>
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+struct a_sigframe sf;
+
+static int fake_sigreturn_bad_magic_run(struct tdescr *td,
+					siginfo_t *si, ucontext_t *uc)
+{
+	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;
+
+	/* find the terminator, preserving existig headers */
+	head = get_terminator(head, GET_SF_RESV_SIZE(sf), NULL);
+	if (head) {
+		head->magic = 0xdeadbeef;
+		head->size = 128;
+		write_terminator(GET_RESV_NEXT_HEAD(head));
+
+		ASSERT_BAD_CONTEXT(&sf.uc);
+		fake_sigreturn(&sf, sizeof(sf), 16);
+	}
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.name = "FAKE_SIGRETURN_BAD_MAGIC",
+		.descr = "Triggers a fake sigreturn with a sigframe including a bad non-existent magic",
+		.sig_ok = SIGSEGV,
+		.timeout = 3,
+		.run = fake_sigreturn_bad_magic_run,
+};