Add variable privilege to gdt #68
Conversation
|
I've made a breaking (but neccesary imo) change to allow for the use of the IO permissions bitmap - it adds an argument to |
|
To fix this compile error in bootimage: error[E0061]: this function takes 2 parameters but 1 parameter was supplied
--> src/gdt.rs:24:42
|
24 | let tss_selector = gdt.add_entry(Descriptor::tss_segment(&TSS));
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected 2 parameters
error: aborting due to previous errorthe relevant line should be changed to let tss_selector = gdt.add_entry(Descriptor::tss_segment(&TSS, 0)); |
| } | ||
| }; | ||
| SegmentSelector::new(index as u16, PrivilegeLevel::Ring0) | ||
| SegmentSelector::new(index as u16, PrivilegeLevel::from_u16(privilege as u16)) |
There was a problem hiding this comment.
How about adding a privilege_level(&self) -> PrivilegeLevel method to Descriptor? Then we could keep the bit shifts at one place. Also, we could use the get_bits method instead of doing the shift and logical AND ourselves.
| low.set_bits( | ||
| 0..16, | ||
| (size_of::<TaskStateSegment>() + (tss.iomap_base + iomap_size) as usize - 1) as u64 | ||
| ); |
There was a problem hiding this comment.
This code only works if the iomap is directly adjacent to the tss. Also, it makes the function unsafe because setting an iomap_size with no iomap would lead to invalid memory accesses.
To solve this, we could create an internal (non-public) tss_segment_with_limit(tss, limit) function and move the complete implementation to it. The function would then just do low.set_bits(0..16, limit - 1). On top of that function we can recreate the current tss_segment function by calling tss_segment_with_limit(tss, size_of::<TaskStateSegment>()).
Now we can create a new pub unsafe fn tss_segment_with_custom_limit(tss: […], limit: u64) function that allows taking the iomap_size into account. Internally, the function just calls tss_segment_with_limit(tss, limit). This way, the original tss_segment function can stay safe and we avoid the breaking parameter addition. This approach also allows us to create a TssWithIomap struct in the future and add a safe tss_segment_with_iomap function.
What do you think about this?
There was a problem hiding this comment.
This code only works if the iomap is directly adjacent to the tss.
I think that it should work for the TSS anywhere (so long as it is within a 16 bit pointer)? That's why iomap_base is added to the size.
What do you think about this?
Sounds good. Will do.
There was a problem hiding this comment.
That's why iomap_base is added to the size.
Sorry, missed that!
Sounds good. Will do.
Thanks!
|
@Restioson Are you still interested in this? |
|
I completely forgot about this, sorry! Looks like there is one main review comment left, unless you remember anything else. I can try fix this soon. |
|
@Restioson Sorry for the delay in answering. I just took a look at this PR again and it seems like the first part of the functionality was already implemented in #153 recently. The |
|
Hi there, sorry for the hiatus! I'm just looking through this again and reviewing what I've done so far. Indeed, it looks like #153 did the first part, so I will try to get the iomap stuff working. |
|
I'm going to reopen the PR as that will probably be the easiest. |
This PR allows users to vary the privilege level in gdt entries by manually editing the descriptor entry. This has been tested for code/data segments, but not for system segments. It also allows the user to specify an IO permissions bitmap.