Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

greq: bugfixes, layout correctness tests and other cleanups #163

Merged
merged 12 commits into from
Nov 23, 2023
Merged

greq: bugfixes, layout correctness tests and other cleanups #163

merged 12 commits into from
Nov 23, 2023

Conversation

00xc
Copy link
Member

@00xc 00xc commented Nov 23, 2023

  • Fix UB in SnpGuestRequestMsgHdr::get_aad_slice()
  • Fix reachable assertions in SnpGuestRequestExtData::boxed_new() and SnpGuestRequestMsg::boxed_new().
  • Add tests for layout-sensitive structs.
  • Refactor assertions that can be evaluated at compile time
  • Other syntax & readability cleanups

00xc added 12 commits November 23, 2023 01:23
@00xc
greq/msg: fix potential undefined behavior in get_aad_slice()
ceb99a8 
Miri was emits an error for the aad_size test (trace below). This is
party because of the use of `ptr::offset_from`, which has the safety
requirement that both arguments belong to the same allocated object [0],
which in this case did not (one was a field inside the other).

Fix this by:

1. Using core::ptr::addr_of!() to get both pointers, which is cleaner
   and does not create an intermediate reference.

2. Computing the offset as follows, as per the documentation [0] (`T`
   is `u8` so we do not need to divide).

    (PTR_A as isize - PTR_B as isize) / (size_of::<T>() as isize)

3. Reinterpret the whole header struct as a slice of bytes, and then
   giving out a subslice of it.

Miri trace:

test greq::msg::tests::aad_size ... error: Undefined Behavior: trying to retag from <19320449> for SharedReadOnly permission at alloc7317635[0x31], but that tag does not exist in the borrow stack for this location
   --> /home/clopez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:102:9
    |
102 |         &*ptr::slice_from_raw_parts(data, len)
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |         |
    |         trying to retag from <19320449> for SharedReadOnly permission at alloc7317635[0x31], but that tag does not exist in the borrow stack for this location
    |         this error occurs as part of retag at alloc7317635[0x30..0x60]
    |
    = help: this indicates a potential bug in the program: it performed an invalid operation, but the Stacked Borrows rules it violated are still experimental
    = help: see https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md for further information
help: <19320449> was created by a SharedReadOnly retag at offsets [0x30..0x31]
   --> src/greq/msg.rs:172:24
    |
172 |         let algo_gva = &self.algo as *const u8;
    |                        ^^^^^^^^^^
    = note: BACKTRACE (of the first span):
    = note: inside `core::slice::from_raw_parts::<'_, u8>` at /home/clopez/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/slice/raw.rs:102:9: 102:47
note: inside `greq::msg::SnpGuestRequestMsgHdr::get_aad_slice`
   --> src/greq/msg.rs:176:18
    |
176 |         unsafe { from_raw_parts(algo_gva, MSG_HDR_SIZE - algo_offset) }
    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
note: inside `greq::msg::tests::aad_size`
   --> src/greq/msg.rs:566:19
    |
566 |         let aad = hdr.get_aad_slice();
    |                   ^^^^^^^^^^^^^^^^^^^
note: inside closure
   --> src/greq/msg.rs:564:18
    |
563 |     #[test]
    |     ------- in this procedural macro expansion
564 |     fn aad_size() {
    |                  ^
    = note: this error originates in the attribute macro `test` (in Nightly builds, run with -Z macro-backtrace for more info)

[0] https://doc.rust-lang.org/std/primitive.pointer.html#method.offset_from

Fixes: 9211b4f ("greq: Add SnpGuestRequestMsg and SnpGuestRequestExtData")
Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg: fix test imports
b663bb6 
The bulk of imports is in the same file, which is the parent
submodule, so import from there directly.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg: use VirtAddr operators to compute region bounds
1d27869 
Instead of converting via VirtAddr::bits(), adding and then converting
back, simply use VirtAddr::add(), which overloads the `+` operator.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg: move SnpGuestRequestMsgHdr next to method implementations
2f0c10f 
This is more conventional Rust.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg: fix reachable assertions due to insufficient allocation layout
58740d0 
From the docs [0]:

> An array of [T; N] has a size of size_of::<T>() * N and the same
> alignment of T

SnpGuestRequestExtData is an array of u8s, while SnpGuestRequestMsg
contains SnpGuestRequestMsgHdr, a packed struct of 96 bytes, followed
by a u8 array.

This means that when allocating these structures via their
boxed_new() methods an assertion might trigger if the address is not
page aligned, which the allocator may legally return.

Fix this by setting an explicit alignment. Both structures can afford
not being packed: SnpGuestRequestExtData contains a contiguous byte
array, so there will not be padding; SnpGuestRequestMsg contains only
two fields, of which the first one ends naturally at an 8-byte
boundary, and the second one has a required alignment of 1 (a u8
array), which means there will never be padding.

While we are at it, add two tests to verify that the assertions are
not triggered.

[0] https://doc.rust-lang.org/reference/type-layout.html#array-layout

Fixes: 9211b4f ("greq: Add SnpGuestRequestMsg and SnpGuestRequestExtData")
Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg: fix stray comment
cc82968 
Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq: use Self in various places for better readability
b9054f9 
Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/pld_report: add SAFETY comments to unsafe casts
de614dc 
Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq: add structure layout tests
c9dfb80 
Add tests to verify that the layout for the attestation report and
the SNP_GUEST_REQUEST command structures are correct.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq: use addr_of!() / addr_of_mut() instead of "as" casting
8a393cd 
This makes code more explicit and clearer.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq: sanity check struct sizes at compile time
60cdfbe 
The size of SnpGuestRequestMsg, SnpGuestRequestMsgType, and
AttestationReport can be asserted at compile time. This allows us to
remove asserts within several functions and to remove a corresponding
redundant test, since the SVSM will not build at all if one of these
assertions fails.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
greq/msg/tests: use unwrap() instead of asserting is_ok()
dd610f5 
This will show the causing error on failure.

Signed-off-by: Carlos López <carlos.lopez@suse.com>
@00xc
Copy link
Member Author

00xc commented Nov 23, 2023 •
edited
Loading

By the way, I verified that doing addr_of!(*var) is the same as var as *const _, even in debug builds. My concern was that *var could make a copy to the stack if optimizations were disabled, but this is not the case:

https://godbolt.org/z/s7WfevPvT

@joergroedel joergroedel merged commit 99a2da3 into coconut-svsm:main Nov 23, 2023
2 checks passed
@00xc 00xc deleted the greq branch July 1, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@00xc @joergroedel