mbox series

[kvm-unit-tests,v2,00/16] x86: cleanups, fixes and new tests

Message ID 20230413184219.36404-1-minipli@grsecurity.net (mailing list archive)
Headers show
Series x86: cleanups, fixes and new tests | expand

Message

Mathias Krause April 13, 2023, 6:42 p.m. UTC 
v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/

This is v2 of the "non-canonical memory access" test. It evolved into a
small series, bringing cleanups and fixes along the way.

I integrated Sean's feedback and changed the test to make use of
ASM_TRY() instead of using the hand-rolled exception handler. I also
switched all other users in emulator64.c to ASM_TRY() and was able to
drop the one-off exception handler all together.

Sean, this should be a solid ground to refine it further when [1] lands?

[1] https://lkml.kernel.org/r/20230406025117.738014-1-seanjc@google.com

As for the fixes, run_in_user() didn't restore the exception handler it
overwrites, which leads to interesting bugs when the handler fires again
for an unrelated exception -- that longjmp() won't do the right thing in
this case ;)

I fixed fault_test() as well, as it has the same behaviour.

For new tests, I added the non-canonical memory access exception test of
v1 and added another SS segment register load test to check non-NULL
selectors as well, as I stumbled over the bugs in run_in_user() while
switching test_sreg() over to TRY_ASM().

Be aware that the types.h removal (first patch) has an unfortunate side
effect. It breaks compilation in already build trees, as the dependency
files (.*.d) don't get regenerated / cleaned if a source file changes.
This leads to stale references to types.h which can only be solved by a
'make clean'. :(

We really should change the dependency file generation to avoid that
problem, as the current state is kinda awkward. Tho, I didn't had the
time to look into it further myself.

Please apply!

Thanks,
Mathias

PS: I'm on holidays for three weeks from Saturday on, so won't respond
to feedback any time soon.

Mathias Krause (16):
  x86: Drop types.h
  x86: Use symbolic names in exception_mnemonic()
  x86: Add vendor specific exception vectors
  x86/cet: Use symbolic name for #CP
  x86/access: Use 'bool' type as defined via libcflat.h
  x86/run_in_user: Change type of code label
  x86/run_in_user: Preserve exception handler
  x86/run_in_user: Relax register constraints of inline asm
  x86/run_in_user: Reload SS after successful return
  x86/fault_test: Preserve exception handler
  x86/emulator64: Relax register constraints for usr_gs_mov()
  x86/emulator64: Switch test_sreg() to ASM_TRY()
  x86/emulator64: Add non-null selector test
  x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
  x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
  x86/emulator64: Test non-canonical memory access exceptions

 lib/x86/processor.h  |  13 ++++++
 lib/x86/desc.c       |  43 ++++++++++--------
 lib/x86/fault_test.c |   4 +-
 lib/x86/usermode.c   |  42 ++++++++++-------
 x86/types.h          |  21 ---------
 x86/access.c         |  11 ++---
 x86/cet.c            |   2 +-
 x86/cmpxchg8b.c      |   1 -
 x86/emulator.c       |   1 -
 x86/emulator64.c     | 105 ++++++++++++++++++++++++-------------------
 x86/pmu_pebs.c       |   1 -
 x86/svm.c            |   1 -
 x86/svm_tests.c      |   1 -
 x86/vmx_tests.c      |   1 -
 14 files changed, 129 insertions(+), 118 deletions(-)
 delete mode 100644 x86/types.h

Comments

Sean Christopherson June 13, 2023, 9:40 p.m. UTC | #1 
On Thu, 13 Apr 2023 20:42:03 +0200, Mathias Krause wrote:
> v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/
> 
> This is v2 of the "non-canonical memory access" test. It evolved into a
> small series, bringing cleanups and fixes along the way.
> 
> I integrated Sean's feedback and changed the test to make use of
> ASM_TRY() instead of using the hand-rolled exception handler. I also
> switched all other users in emulator64.c to ASM_TRY() and was able to
> drop the one-off exception handler all together.
> 
> [...]

Applied everything except the code label change to kvm-x86 next.  I replied to
that specific patch, feel free to follow-up there.

I tweaked "x86/run_in_user: Reload SS after successful return" to use an "rm"
constraint instead of hardcoding use of AX, holler if that's wrong for some
reason.  I'm planning on sending a pull request later this week, so you've got
a few days to object.

Thanks a ton for the cleanups!

[01/16] x86: Drop types.h
        https://github.com/kvm-x86/kvm-unit-tests/commit/0452fa5aecea
[02/16] x86: Use symbolic names in exception_mnemonic()
        https://github.com/kvm-x86/kvm-unit-tests/commit/8cfb268d401b
[03/16] x86: Add vendor specific exception vectors
        https://github.com/kvm-x86/kvm-unit-tests/commit/f224dba008df
[04/16] x86/cet: Use symbolic name for #CP
        https://github.com/kvm-x86/kvm-unit-tests/commit/00d585d8731b
[05/16] x86/access: Use 'bool' type as defined via libcflat.h
        https://github.com/kvm-x86/kvm-unit-tests/commit/c304eda6ae7f
[06/16] x86/run_in_user: Change type of code label
	DID NOT APPLY
[07/16] x86/run_in_user: Preserve exception handler
        https://github.com/kvm-x86/kvm-unit-tests/commit/d0ef95181cfb
[08/16] x86/run_in_user: Relax register constraints of inline asm
        https://github.com/kvm-x86/kvm-unit-tests/commit/45bafaf28fbb
[09/16] x86/run_in_user: Reload SS after successful return
        https://github.com/kvm-x86/kvm-unit-tests/commit/8338209b8245
[10/16] x86/fault_test: Preserve exception handler
        https://github.com/kvm-x86/kvm-unit-tests/commit/11aac640d01b
[11/16] x86/emulator64: Relax register constraints for usr_gs_mov()
        https://github.com/kvm-x86/kvm-unit-tests/commit/c66547850058
[12/16] x86/emulator64: Switch test_sreg() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/9d74b31d1c81
[13/16] x86/emulator64: Add non-null selector test
        https://github.com/kvm-x86/kvm-unit-tests/commit/23c647d0ef29
[14/16] x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/ac4f843474b4
[15/16] x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
        https://github.com/kvm-x86/kvm-unit-tests/commit/5a3515ea1bc2
[16/16] x86/emulator64: Test non-canonical memory access exceptions
        https://github.com/kvm-x86/kvm-unit-tests/commit/e3a9b2f5490e

--
https://github.com/kvm-x86/kvm-unit-tests/tree/next
Mathias Krause June 14, 2023, 9:58 p.m. UTC | #2 
On 13.06.23 23:40, Sean Christopherson wrote:
> On Thu, 13 Apr 2023 20:42:03 +0200, Mathias Krause wrote:
>> v1: https://lore.kernel.org/kvm/b6322bd0-3639-fb2a-7211-974386865bac@grsecurity.net/
>>
>> This is v2 of the "non-canonical memory access" test. It evolved into a
>> small series, bringing cleanups and fixes along the way.
>>
>> I integrated Sean's feedback and changed the test to make use of
>> ASM_TRY() instead of using the hand-rolled exception handler. I also
>> switched all other users in emulator64.c to ASM_TRY() and was able to
>> drop the one-off exception handler all together.
>>
>> [...]
> 
> Applied everything except the code label change to kvm-x86 next.  I replied to
> that specific patch, feel free to follow-up there.

I did, but I have no strong opinion on getting it merged. It's just the
coding style I'm used to. If KUT's different, then that be it.

> 
> I tweaked "x86/run_in_user: Reload SS after successful return" to use an "rm"
> constraint instead of hardcoding use of AX, holler if that's wrong for some
> reason.  I'm planning on sending a pull request later this week, so you've got
> a few days to object.

I used "i" as a constraint as that's what KERNEL_DS really is: an
integer constant. But I can see that the stunt of actually loading %ss
without clobbering a register isn't all that nice looking. I used (R)AX
specifically as that avoids allocating yet another register for this ASM
block. However, there are still enough left and the stack pointer is
already restored at that point so using "rm" instead should be fine.

Just noticed that the constraints for 'rax' should be "=&a" instead of
"+a" as the ASM doesn't care about its initial value, just needs to
prevent the compiler from allocating AX for any of the input register
variables. But that can be a separate cleanup.

> 
> Thanks a ton for the cleanups!
> 
> [01/16] x86: Drop types.h
>         https://github.com/kvm-x86/kvm-unit-tests/commit/0452fa5aecea
> [02/16] x86: Use symbolic names in exception_mnemonic()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/8cfb268d401b
> [03/16] x86: Add vendor specific exception vectors
>         https://github.com/kvm-x86/kvm-unit-tests/commit/f224dba008df
> [04/16] x86/cet: Use symbolic name for #CP
>         https://github.com/kvm-x86/kvm-unit-tests/commit/00d585d8731b
> [05/16] x86/access: Use 'bool' type as defined via libcflat.h
>         https://github.com/kvm-x86/kvm-unit-tests/commit/c304eda6ae7f
> [06/16] x86/run_in_user: Change type of code label
> 	DID NOT APPLY
> [07/16] x86/run_in_user: Preserve exception handler
>         https://github.com/kvm-x86/kvm-unit-tests/commit/d0ef95181cfb
> [08/16] x86/run_in_user: Relax register constraints of inline asm
>         https://github.com/kvm-x86/kvm-unit-tests/commit/45bafaf28fbb
> [09/16] x86/run_in_user: Reload SS after successful return
>         https://github.com/kvm-x86/kvm-unit-tests/commit/8338209b8245
> [10/16] x86/fault_test: Preserve exception handler
>         https://github.com/kvm-x86/kvm-unit-tests/commit/11aac640d01b
> [11/16] x86/emulator64: Relax register constraints for usr_gs_mov()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/c66547850058
> [12/16] x86/emulator64: Switch test_sreg() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/9d74b31d1c81
> [13/16] x86/emulator64: Add non-null selector test
>         https://github.com/kvm-x86/kvm-unit-tests/commit/23c647d0ef29
> [14/16] x86/emulator64: Switch test_jmp_noncanonical() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/ac4f843474b4
> [15/16] x86/emulator64: Switch test_mmx_movq_mf() to ASM_TRY()
>         https://github.com/kvm-x86/kvm-unit-tests/commit/5a3515ea1bc2
> [16/16] x86/emulator64: Test non-canonical memory access exceptions
>         https://github.com/kvm-x86/kvm-unit-tests/commit/e3a9b2f5490e

Thanks,
Mathias

> 
> --
> https://github.com/kvm-x86/kvm-unit-tests/tree/next