commit 0054a8a1162c2ea857fb02553835b804ead7b124
author Marco Vanotti <mvanotti@google.com> Tue May 05 05:50:45 2020 +0000
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Tue May 05 05:50:45 2020 +0000
tree 201ed9cfb555980935b33ef520fdf4df4a57d4dc
parent c6f44c876bc00affb968f4f712d912e8673ccbf4

[zircon][debugger] Disallow setting non-canonical rip addresses.

This CL modifies the `arch_set_general_regs` function by always checking
the `rip` for non-canonical addresses.

Without this change, callers of `zx_thread_write_state` could set the
`rip` to a non-canonical address on a suspended thread, causing the
`iretq` instruction to trigger a general protection fault.

Given that the `iretq` instruction would fail after executing the
`swapgs` instruction, the #GP exception would be handled with the `gs`
register from userspace. However, given that there was no change of
privileges, the handler will not issue another `swapgs`: causing the
handler to use the `gs` register from userspace.

Note that before, top-bit set addresses (kernel addresses) were
also disallowed. This change uses `x86_is_vaddr_canonical` instead (the
same mechanism used elsewhere in the kernel) and disallows kernel
addresses with `is_kernel_address`.

I also updated the `NoncanonicalRipAddressSyscall` test to use a real
kernel address (it was using a high address, but that was out of the
kernel address space).

I added a unit test in to test this behavior: writing a non-canonical
return address to the rip results in an invalid argument. Without
this fix, resuming that thread would crash the system.

To run the test:

```
$ fx set bringup.x64 \
    --with-base //garnet/packages/tests:zircon \
    --with-base //bundles/bringup:tests
```

And run it with `runtests -t threads-test`

BUG=50633
This vulnerability was reported by: Quarkslab

Change-Id: I09eaf28c48c4dfbb308cfa82617d6b682df31412
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/383356
Commit-Queue: Marco Vanotti <mvanotti@google.com>
Reviewed-by: Mark Seaborn <mseaborn@google.com>
Reviewed-by: Travis Geiselbrecht <travisg@google.com>
Reviewed-by: Nick Maniscalco <maniscalco@google.com>
Reviewed-by: Venkatesh Srinivas <venkateshs@google.com>
Reviewed-by: Kostya Kortchinsky <kostyak@google.com>
Testability-Review: Nick Maniscalco <maniscalco@google.com>