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

Add ability to add iomap to TSS (take 2) #194

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Add ability to add iomap to TSS (take 2) #194

Changes from 2 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
7e01e80
Add ability to add iomap to TSS
Restioson Oct 17, 2020
290d0c6
formatting
Restioson Oct 17, 2020
7d2fa06
doc & iomap limit calculation
Restioson Oct 17, 2020
027ad7b
missed a cast :(
Restioson Oct 17, 2020
ee27c96
expand on safety requirements
Restioson Oct 17, 2020
c8d062d
-1 after max
Restioson Oct 17, 2020
e5a6612
fmt
Restioson Oct 17, 2020
7051f97
slice for safety
Restioson Oct 17, 2020
780a8d6
error if base =/= expected base
Restioson Oct 17, 2020
25ce9aa
fmt (i always forget this...)
Restioson Oct 17, 2020
7691201
doc
Restioson Oct 17, 2020
fa5d40e
fmt
Restioson Oct 17, 2020
fa95830
handle iomap before tss
Restioson Oct 17, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions src/structures/gdt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,20 @@ impl Descriptor {
/// Creates a TSS system descriptor for the given TSS.
#[inline]
pub fn tss_segment(tss: &'static TaskStateSegment) -> Descriptor {
// SAFETY: if iomap_size is zero, there are no requirements to uphold.
unsafe { Self::tss_segment_with_iomap(tss, 0) }
}

/// Creates a TSS system descriptor for the given TSS, setting up the IO permissions bitmap.
///
/// # Safety
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if we could put enough checks here to make this method safe. We could take the IO map as a [u8] slice and then do all of the necessary checks (it's in range, byte past the end is all 1s, iomap_base is <= 0xDFFFH, etc..). We either need to do these checks or document all the requirements from the SDM in the # Safety section.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning a result if it fails the checks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either that or panic. We would need a custom error type for that (it could live in the tss module).

Copy link
Member Author

@Restioson Restioson Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, I've expanded on the safety requirements. I couldn't find any others than terminating 0xff byte, iomap_base <= 0xdfff, and it points to the IO map slice in the SDM, although maybe I missed some.

Copy link
Member Author

@Restioson Restioson Oct 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might also be possible to have a TssWithIoMap struct, as discussed here, but I'm unsure how non-8192-length io permissions bitmaps should be handled. Otherwise, that could also be safe.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Later on, we could add a structure (the TssWithIoMap) to this library. That structure would make sure things stay valid (iomap_addr is correctly initialized, there's always a trailing 0xff byte, etc...) And we'd have a descriptor method on that structure that calls this method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should just check that the iomap_base is correct, and return an error if it's wrong.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I've pushed a prototype of this now.

Copy link
Member Author

@Restioson Restioson Oct 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking over this change again, and I think it has pretty good potential to make mutation of the IO map impossible. As I understand it, it should be possible to change the IO permissions bitmap at runtime, or am I mistaken? If so, giving &'static [u8] would make any modifications instant UB. Perhaps it could take &'static [UnsafeCell<u8>] to get around this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first part of this conversation seems to be resolved.

I'm thinking over this change again, and I think it has pretty good potential to make mutation of the IO map impossible. As I understand it, it should be possible to change the IO permissions bitmap at runtime, or am I mistaken? If so, giving &'static [u8] would make any modifications instant UB. Perhaps it could take &'static [UnsafeCell<u8>] to get around this?

It's probably best to merge it as is and look into this in a separate PR. I'm not sure whether UnsafeCell would be enough for this or whether we need to e.g. use AtomicU8. We should ask that someone who has more knowledge of the Rust compiler.

///
/// If `iomap_size` is greater than zero, there **must** be a valid IO map at `tss_ptr + iomap_base`.
Restioson marked this conversation as resolved.
Show resolved Hide resolved
/// The size of the IO map must correspond with the given `iomap_size`.
pub unsafe fn tss_segment_with_iomap(
tss: &'static TaskStateSegment,
iomap_size: u16,
) -> Descriptor {
use self::DescriptorFlags as Flags;
use core::mem::size_of;

Expand All @@ -315,8 +329,11 @@ impl Descriptor {
// base
low.set_bits(16..40, ptr.get_bits(0..24));
low.set_bits(56..64, ptr.get_bits(24..32));
// limit (the `-1` in needed since the bound is inclusive)
low.set_bits(0..16, (size_of::<TaskStateSegment>() - 1) as u64);
// limit (the `-1` is needed since the bound is inclusive)
low.set_bits(
0..16,
(size_of::<TaskStateSegment>() + (tss.iomap_base + iomap_size) as usize - 1) as u64,
Restioson marked this conversation as resolved.
Show resolved Hide resolved
);
// type (0b1001 = available 64-bit tss)
low.set_bits(40..44, 0b1001);

Expand Down