diff mbox series

[03/13] kselftest: arm64: mangle_sp_misaligned

Message ID 20190613111335.7645-4-cristian.marussi@arm.com
State New
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 mangle testcase which messes with the ucontext_t
from within the sig_handler, trying to badly modify and misalign the SP.
Expects SIGBUS on test PASS.

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

Comments

Dave Martin June 21, 2019, 10:35 a.m. UTC | #1
On Thu, Jun 13, 2019 at 12:13:25PM +0100, Cristian Marussi wrote:
> Added a simple mangle testcase which messes with the ucontext_t
> from within the sig_handler, trying to badly modify and misalign the SP.
> Expects SIGBUS on test PASS.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
>  .../arm64/signal/testcases/.gitignore         |  1 +
>  .../signal/testcases/mangle_sp_misaligned.c   | 24 +++++++++++++++++++
>  2 files changed, 25 insertions(+)
>  create mode 100644 tools/testing/selftests/arm64/signal/testcases/.gitignore
>  create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> 
> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> new file mode 100644
> index 000000000000..7f7414d241f2
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> @@ -0,0 +1 @@
> +mangle_sp_misaligned
> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> new file mode 100644
> index 000000000000..41bd27312e54
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> @@ -0,0 +1,24 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2019 ARM Limited */
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +static int mangle_misaligned_sp_run(struct tdescr *td, siginfo_t *si,
> +				    ucontext_t *uc)
> +{
> +	ASSERT_GOOD_CONTEXT(uc);
> +
> +	uc->uc_mcontext.sp += 3;

What are we testing here?

It is archietcturally permitted (if unusual) to have a misaligned sp in
userspace.

So are we just getting a SIGBUS after the sigreturn, when the thread
tries to dereference sp?  If so, we aren't really testing anything about
sigreturn here -- I don't see any check in the kernel when restoring sp
in sigreturn.

Even if there were no SIGBUS, the thread stack is now corrupt (due to
wrong sp), so the interrupted code is unlikely to continue running
successfully.

Am I missing something?

[...]

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

On 6/21/19 11:35 AM, Dave Martin wrote:
> On Thu, Jun 13, 2019 at 12:13:25PM +0100, Cristian Marussi wrote:
>> Added a simple mangle testcase which messes with the ucontext_t
>> from within the sig_handler, trying to badly modify and misalign the SP.
>> Expects SIGBUS on test PASS.
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>>   .../arm64/signal/testcases/.gitignore         |  1 +
>>   .../signal/testcases/mangle_sp_misaligned.c   | 24 +++++++++++++++++++
>>   2 files changed, 25 insertions(+)
>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/.gitignore
>>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
>>
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> new file mode 100644
>> index 000000000000..7f7414d241f2
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
>> @@ -0,0 +1 @@
>> +mangle_sp_misaligned
>> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
>> new file mode 100644
>> index 000000000000..41bd27312e54
>> --- /dev/null
>> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* Copyright (C) 2019 ARM Limited */
>> +
>> +#include "test_signals_utils.h"
>> +#include "testcases.h"
>> +
>> +static int mangle_misaligned_sp_run(struct tdescr *td, siginfo_t *si,
>> +				    ucontext_t *uc)
>> +{
>> +	ASSERT_GOOD_CONTEXT(uc);
>> +
>> +	uc->uc_mcontext.sp += 3;
> 
> What are we testing here?
> 
> It is archietcturally permitted (if unusual) to have a misaligned sp in
> userspace.
> 
> So are we just getting a SIGBUS after the sigreturn, when the thread
> tries to dereference sp?  If so, we aren't really testing anything about
> sigreturn here -- I don't see any check in the kernel when restoring sp
> in sigreturn.
> 
> Even if there were no SIGBUS, the thread stack is now corrupt (due to
> wrong sp), so the interrupted code is unlikely to continue running
> successfully.
> 
> Am I missing something?
> 

The initial (flawed) attempt was to test the check in arm64 rt_sigreturn 
kernel code:

if (regs->sp & 15)
	goto badframe;

BUT in fact such initial check happens at the start of rt_sigreturn 
syscall well before the regs are restored from the uc context in the 
sigframe which I mangled

i.e.
restore_sigframe() -->> __get_user_error(regs->sp...)

==>> uc.uc_mcontext.sp --> regs->sp

happens AFTER the above regs->sp alignment check.

So the check is performed on the effective SP value at the time of 
kernel entry of sigreturn NOT on the uc.uc_mcontext.sp MANGLED value, so 
this is not really a sigreturn related test at this point. (and hence 
the SIGBUS instead of the SEGV).

So an option could be as you proposed in another similarly flawed test 
to mangle uc.uc_mcontext.sp to point to something unreasonable and in 
Kernel space (at least virtually)

I'll give it a try.

Cristian

> [...]
> 
> Cheers
> ---Dave
>
Dave Martin July 3, 2019, 8:39 a.m. UTC | #3
On Tue, Jul 02, 2019 at 04:39:44PM +0100, Cristian Marussi wrote:
> Hi
>
> On 6/21/19 11:35 AM, Dave Martin wrote:
> > On Thu, Jun 13, 2019 at 12:13:25PM +0100, Cristian Marussi wrote:
> >> Added a simple mangle testcase which messes with the ucontext_t
> >> from within the sig_handler, trying to badly modify and misalign the SP.
> >> Expects SIGBUS on test PASS.
> >>
> >> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >> ---
> >>   .../arm64/signal/testcases/.gitignore         |  1 +
> >>   .../signal/testcases/mangle_sp_misaligned.c   | 24 +++++++++++++++++++
> >>   2 files changed, 25 insertions(+)
> >>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/.gitignore
> >>   create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> >>
> >> diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> >> new file mode 100644
> >> index 000000000000..7f7414d241f2
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
> >> @@ -0,0 +1 @@
> >> +mangle_sp_misaligned
> >> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> >> new file mode 100644
> >> index 000000000000..41bd27312e54
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
> >> @@ -0,0 +1,24 @@
> >> +/* SPDX-License-Identifier: GPL-2.0 */
> >> +/* Copyright (C) 2019 ARM Limited */
> >> +
> >> +#include "test_signals_utils.h"
> >> +#include "testcases.h"
> >> +
> >> +static int mangle_misaligned_sp_run(struct tdescr *td, siginfo_t *si,
> >> +                              ucontext_t *uc)
> >> +{
> >> +  ASSERT_GOOD_CONTEXT(uc);
> >> +
> >> +  uc->uc_mcontext.sp += 3;
> >
> > What are we testing here?
> >
> > It is archietcturally permitted (if unusual) to have a misaligned sp in
> > userspace.
> >
> > So are we just getting a SIGBUS after the sigreturn, when the thread
> > tries to dereference sp?  If so, we aren't really testing anything about
> > sigreturn here -- I don't see any check in the kernel when restoring sp
> > in sigreturn.
> >
> > Even if there were no SIGBUS, the thread stack is now corrupt (due to
> > wrong sp), so the interrupted code is unlikely to continue running
> > successfully.
> >
> > Am I missing something?
> >
>
> The initial (flawed) attempt was to test the check in arm64 rt_sigreturn
> kernel code:
>
> if (regs->sp & 15)
>       goto badframe;
>
> BUT in fact such initial check happens at the start of rt_sigreturn
> syscall well before the regs are restored from the uc context in the
> sigframe which I mangled
>
> i.e.
> restore_sigframe() -->> __get_user_error(regs->sp...)
>
> ==>> uc.uc_mcontext.sp --> regs->sp
>
> happens AFTER the above regs->sp alignment check.
>
> So the check is performed on the effective SP value at the time of
> kernel entry of sigreturn NOT on the uc.uc_mcontext.sp MANGLED value, so
> this is not really a sigreturn related test at this point. (and hence
> the SIGBUS instead of the SEGV).
>
> So an option could be as you proposed in another similarly flawed test
> to mangle uc.uc_mcontext.sp to point to something unreasonable and in
> Kernel space (at least virtually)

I think a misaligned sp (i.e., the sp register, not uc_mcontext.sp) at
sigreturn, and an sp that points to unmapped or kernel address space
would be useful tests.

If those are already done elsewhere in the series, that's fine.

(There are many possible variations on this theme, but we have to stop
somewhere.)

Cheers
---Dave
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.
diff mbox series

Patch

diff --git a/tools/testing/selftests/arm64/signal/testcases/.gitignore b/tools/testing/selftests/arm64/signal/testcases/.gitignore
new file mode 100644
index 000000000000..7f7414d241f2
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/.gitignore
@@ -0,0 +1 @@ 
+mangle_sp_misaligned
diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
new file mode 100644
index 000000000000..41bd27312e54
--- /dev/null
+++ b/tools/testing/selftests/arm64/signal/testcases/mangle_sp_misaligned.c
@@ -0,0 +1,24 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (C) 2019 ARM Limited */
+
+#include "test_signals_utils.h"
+#include "testcases.h"
+
+static int mangle_misaligned_sp_run(struct tdescr *td, siginfo_t *si,
+				    ucontext_t *uc)
+{
+	ASSERT_GOOD_CONTEXT(uc);
+
+	uc->uc_mcontext.sp += 3;
+
+	return 1;
+}
+
+struct tdescr tde = {
+		.sanity_disabled = true,
+		.name = "MANGLE_SP_MISALIGNED",
+		.descr = "Mangling uc_mcontext with MISALIGNED SP",
+		.sig_trig = SIGUSR1,
+		.sig_ok = SIGBUS,
+		.run = mangle_misaligned_sp_run,
+};