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.

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

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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
54 changes: 50 additions & 4 deletions src/structures/gdt.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
//! Types for the Global Descriptor Table and segment selectors.

use crate::structures::tss::InvalidIoMap;
use crate::structures::{tss::TaskStateSegment, DescriptorTablePointer};
use crate::PrivilegeLevel;
use bit_field::BitField;
use bitflags::bitflags;
use core::fmt;
use core::{cmp, fmt, mem};

/// Specifies which element to load into a segment from
/// descriptor tables (i.e., is a index to LDT or GDT table
Expand Down Expand Up @@ -306,17 +307,62 @@ 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_raw(tss, 0) }
}

/// Creates a TSS system descriptor for the given TSS, setting up the IO permissions bitmap.
pub fn tss_segment_with_iomap(
tss: &'static TaskStateSegment,
iomap: &'static [u8],
) -> Result<Descriptor, InvalidIoMap> {
if iomap.len() > 8193 {
return Err(InvalidIoMap::TooLong { len: iomap.len() });
}

let base = iomap.as_ptr() as usize - tss as *const _ as usize;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to use wrapping_offset_from here and get an isize. We need to deal with isizes because the iomap could come before the tss, and that would also be wrong.

Copy link
Member Author

@Restioson Restioson Oct 17, 2020

Choose a reason for hiding this comment

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

wrapping_offset_from doesn't exist. I think that offset_from would also be incorrect, as according to offset_from's safety contract:

The distance between the pointers, in bytes, cannot overflow an isize.

This is overflowed if TSS is higher half (say, 0xffffffff80000000) and IO permissions bitmap is lower half (say, 0x1000), I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just handled this with a manual if check.

if base > 0xdfff {
return Err(InvalidIoMap::TooFarFromTss { distance: base });
}

let last_byte = *iomap.last().unwrap_or(&0xff);
phil-opp marked this conversation as resolved.
Show resolved Hide resolved
if last_byte != 0xff {
return Err(InvalidIoMap::InvalidTerminatingByte { byte: last_byte });
}

if tss.iomap_base != base as u16 {
return Err(InvalidIoMap::InvalidBase {
expected: base as u16,
got: tss.iomap_base,
});
}

// SAFETY: all invariants checked above
Ok(unsafe { Self::tss_segment_raw(tss, iomap.len() as u16) })
}

/// 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

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

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

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.

///
/// There must be a valid IO map at `(tss as *const u8).offset(tss.iomap_base)`
/// of length `iomap_size`, with the terminating `0xFF` byte. Additionally, `iomap_base` must
/// not exceed `0xDFFF`.
unsafe fn tss_segment_raw(tss: &'static TaskStateSegment, iomap_size: u16) -> Descriptor {
use self::DescriptorFlags as Flags;
use core::mem::size_of;

let ptr = tss as *const _ as u64;

let mut low = Flags::PRESENT.bits();
// 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)
let iomap_limit = tss.iomap_base as u64 + iomap_size as u64;
low.set_bits(
0..16,
cmp::max(mem::size_of::<TaskStateSegment>() as u64, iomap_limit) - 1,
);
// type (0b1001 = available 64-bit tss)
low.set_bits(40..44, 0b1001);

Expand Down
31 changes: 30 additions & 1 deletion src/structures/tss.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ pub struct TaskStateSegment {
pub interrupt_stack_table: [VirtAddr; 7],
reserved_3: u64,
reserved_4: u16,
/// The 16-bit offset to the I/O permission bit map from the 64-bit TSS base.
/// The 16-bit offset to the I/O permission bit map from the 64-bit TSS base. It must not
/// exceed `0xDFFF`.
pub iomap_base: u16,
}

Expand All @@ -37,3 +38,31 @@ impl TaskStateSegment {
}
}
}

/// The given IO permissions bitmap is invalid.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub enum InvalidIoMap {
Copy link
Member

Choose a reason for hiding this comment

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

It would be great to have a Display implementation for this struct as a quick way to print an error.

/// The IO permissions bitmap is too far from the TSS. It must be within `0xdfff` bytes of the
/// start of the TSS.
TooFarFromTss {
/// The distance of the IO permissions bitmap from the beginning of the TSS.
distance: usize,
phil-opp marked this conversation as resolved.
Show resolved Hide resolved
},
/// The final byte of the IO permissions bitmap was not 0xff
InvalidTerminatingByte {
/// The byte found at the end of the IO permissions bitmap.
byte: u8,
},
/// The IO permissions bitmap exceeds the maximum length (8193).
TooLong {
/// The length of the IO permissions bitmap.
len: usize,
},
/// The `iomap_base` in the `TaskStateSegment` struct was not what was expected.
InvalidBase {
/// The expected `iomap_base` to be set in the `TaskStateSegment` struct.
expected: u16,
/// The actual `iomap_base` set in the `TaskStateSegment` struct.
got: u16,
},
}