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

Restioson · 2020-10-17T07:34:08Z

I've refactored #68 to add a new method which allows you to specify IO permissions bitmap size, rather than incorporating it into the existing method. The existing method is now implemented by calling the new, unsafe method with an io permissions bitmap size of 0.

I've tested this by allowing usermode to access the first serial point and then trying to print, which does work. I'm not sure what kind of other unit tests I should write for this.

I've left out any kind of IoPermissionsBitmap structure for now, as this can be implemented on top of this change, and I'm not quite sure about the specifics (for instance, what if you don't want the full 8192 bytes?).

src/structures/gdt.rs

josephlr · 2020-10-17T10:07:22Z

src/structures/gdt.rs

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


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.

Returning a result if it fails the checks?

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

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.

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.

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.

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

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

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?

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.

src/structures/gdt.rs

Restioson · 2020-10-17T13:21:56Z

Maybe it would be possible to write an integration test for this wherein the kernel is dropped to userspace and tests out a few ports? I can't get the integration tests to compile though so I'm unsure how to proceed.

Restioson · 2020-10-17T13:22:43Z

Argh, I should really set up cargo fmt as a pre-commit hook or something.

josephlr · 2020-10-17T13:29:29Z

src/structures/gdt.rs

+            return Err(InvalidIoMap::TooLong { len: iomap.len() });
+        }
+
+        let base = iomap.as_ptr() as usize - tss as *const _ as usize;


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.

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.

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

src/structures/gdt.rs

src/structures/tss.rs

phil-opp · 2020-12-28T10:32:11Z

@josephlr Do you have time to continue your review or should I try to take over?

phil-opp · 2021-04-01T20:06:21Z

@Restioson Sorry for the long silence! It seems like @josephlr does no longer have time to review this, so I'll take a look.

phil-opp

Looks good to me, thanks a lot! This only needs a rebase, then it should be ready for merging. We can look into the modification problem in a follow-up PR.

phil-opp · 2021-04-01T21:18:21Z

src/structures/tss.rs

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


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

src/structures/tss.rs

src/structures/gdt.rs

phil-opp · 2021-04-01T21:29:16Z

src/structures/gdt.rs

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


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.

Restioson · 2021-04-12T11:59:36Z

Thanks for the review, I will try add the display impl and rebase it shortly! Right now I have a bit of university work to finish but hopefully soon after I can get to this.

ChocolateLoverRaj · 2025-03-18T22:44:29Z

src/structures/gdt.rs

+        tss: &'static TaskStateSegment,
+        iomap: &'static [u8],


I don't like how this makes the caller of this function have to make sure that the tss and iomap are close enough to each other. Also idk if it's bad to put other data between the tss and iomap but personally I think it's better to have the iomap go right after the tss.

I don't like how this makes the caller of this function have to make sure that the tss and iomap are close enough to each other.

It's certainly a trade-off, but I think it's one worth making. The big advantage of this approach is that we don't force a given layout and users are free to choose their own. This API is compatible with both statically sized and dynamically sized I/O maps and I'm not sure if we can easily achieve this if we enforce a given layout with a struct in this crate.

When it comes to actually using this, I think runtime errors can be easily avoided. Users can define their own structs containing the TSS and an I/O map of their choice:

#[repr(C)] struct TssWithIOMap { tss: TaskStateSegment, iomap: [u8; 26], } let tss: &'static TssWithIOMap = todo!(); let tss = Descriptor::tss_segment_with_iomap(&tss.tss, &tss.iomap).unwrap();

We might want to consider adding something like this as an example in the doc comment.

For more advanced uses (e.g. dynamically sized I/O maps on the heap) users can implement whatever layout fits them.

Also idk if it's bad to put other data between the tss and iomap but personally I think it's better to have the iomap go right after the tss.

Having other data between the two should be completely fine.

Freax13

This looks good to me. There are a few things left to do (the Display impl and adding a bit more documentation), but I think once we have that we can merge this. I'll try to find some time later today to finish this up.

@ChocolateLoverRaj you put a lot of thoughts on I/O map support into #539. This PR is going in a different direction, but I don't think it's incompatible with yours. It might still be worthwhile to add a TssWithIoBitMap struct to this crate for users that want a statically sized TSS. This could be done in a later PR.

Freax13 · 2025-03-19T07:01:51Z

src/structures/gdt.rs

+        tss: &'static TaskStateSegment,
+        iomap: &'static [u8],


I don't like how this makes the caller of this function have to make sure that the tss and iomap are close enough to each other.

It's certainly a trade-off, but I think it's one worth making. The big advantage of this approach is that we don't force a given layout and users are free to choose their own. This API is compatible with both statically sized and dynamically sized I/O maps and I'm not sure if we can easily achieve this if we enforce a given layout with a struct in this crate.

When it comes to actually using this, I think runtime errors can be easily avoided. Users can define their own structs containing the TSS and an I/O map of their choice:

#[repr(C)] struct TssWithIOMap { tss: TaskStateSegment, iomap: [u8; 26], } let tss: &'static TssWithIOMap = todo!(); let tss = Descriptor::tss_segment_with_iomap(&tss.tss, &tss.iomap).unwrap();

We might want to consider adding something like this as an example in the doc comment.

For more advanced uses (e.g. dynamically sized I/O maps on the heap) users can implement whatever layout fits them.

Also idk if it's bad to put other data between the tss and iomap but personally I think it's better to have the iomap go right after the tss.

Having other data between the two should be completely fine.

Freax13 · 2025-03-19T19:08:50Z

I rebased this PR and addressed some of the remaining review comments.

Restioson · 2025-03-20T12:27:28Z

I'm no longer working on anything osdev-related, so is it possible to transfer this PR to someone else so they have control over it?

Freax13 · 2025-03-20T12:40:00Z

I'm no longer working on anything osdev-related, so is it possible to transfer this PR to someone else so they have control over it?

GitHub has this neat little feature where maintainers can push to the branch used with a pull request (this is enabled by default). I already pushed some changes to your branch :)

Alternatively, we could close this PR and open a new one, but I think we're fine with keeping this one for now.

Restioson · 2025-03-20T12:44:01Z

Thanks. Can I unwatch this PR in that case?

Freax13 · 2025-03-20T12:51:58Z

Sure

josephlr reviewed Oct 17, 2020

View reviewed changes

josephlr mentioned this pull request Oct 17, 2020

The iomap_base of TaskStateSegment shouldn't be initialized to zero #195

Closed

josephlr reviewed Oct 17, 2020

View reviewed changes

src/structures/gdt.rs Outdated Show resolved Hide resolved

josephlr reviewed Oct 17, 2020

View reviewed changes

Restioson commented Oct 17, 2020

View reviewed changes

src/structures/gdt.rs Show resolved Hide resolved

Restioson commented Oct 17, 2020

View reviewed changes

src/structures/tss.rs Show resolved Hide resolved

phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Dec 28, 2020

phil-opp approved these changes Apr 1, 2021

View reviewed changes

phil-opp added waiting-on-author Waiting for the author to act on review feedback. and removed waiting-for-review Waiting for a review from the maintainers. labels Nov 8, 2021

ChocolateLoverRaj mentioned this pull request Mar 18, 2025

Safe API for using I/O Permission Bitmap in TSS #539

Open

ChocolateLoverRaj reviewed Mar 18, 2025

View reviewed changes

Freax13 reviewed Mar 19, 2025

View reviewed changes

Restioson added 12 commits March 19, 2025 19:50

Add ability to add iomap to TSS

0e66d82

formatting

e17b928

doc & iomap limit calculation

6cd1a99

missed a cast :(

d19a5ae

expand on safety requirements

5a0ffc2

-1 after max

2fef3bd

fmt

05c0828

slice for safety

f13551e

error if base =/= expected base

c03a90a

fmt (i always forget this...)

a85c1e2

doc

9c30209

fmt

7e49ece

Restioson and others added 4 commits March 19, 2025 19:57

handle iomap before tss

cfa2083

use From instead of casts

411b42b

add Display impl for InvalidIoMap

6667531

add an example for Descriptor::tss_segment_with_iomap

8e1d5f8

Freax13 force-pushed the iomap branch from fa95830 to 8e1d5f8 Compare March 19, 2025 19:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Restioson commented Oct 17, 2020 •

edited

Loading

josephlr Oct 17, 2020

Restioson Oct 17, 2020

josephlr Oct 17, 2020

Restioson Oct 17, 2020 •

edited

Loading

Restioson Oct 17, 2020 •

edited

Loading

josephlr Oct 17, 2020

josephlr Oct 17, 2020

Restioson Oct 17, 2020

Restioson Oct 19, 2020 •

edited

Loading

phil-opp Apr 1, 2021

Restioson commented Oct 17, 2020

Restioson commented Oct 17, 2020

josephlr Oct 17, 2020

Restioson Oct 17, 2020 •

edited

Loading

Restioson Oct 17, 2020

phil-opp commented Dec 28, 2020

phil-opp commented Apr 1, 2021

phil-opp left a comment

phil-opp Apr 1, 2021

phil-opp Apr 1, 2021

Restioson commented Apr 12, 2021

ChocolateLoverRaj Mar 18, 2025

Freax13 Mar 19, 2025

Freax13 left a comment

Freax13 Mar 19, 2025

Freax13 commented Mar 19, 2025

Restioson commented Mar 20, 2025

Freax13 commented Mar 20, 2025

Restioson commented Mar 20, 2025

Freax13 commented Mar 20, 2025

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

Are you sure you want to change the base?

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

Conversation

Restioson commented Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Restioson Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson Oct 19, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson commented Oct 17, 2020

Restioson commented Oct 17, 2020

Choose a reason for hiding this comment

Restioson Oct 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phil-opp commented Dec 28, 2020

phil-opp commented Apr 1, 2021

phil-opp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Restioson commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freax13 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Freax13 commented Mar 19, 2025

Restioson commented Mar 20, 2025

Freax13 commented Mar 20, 2025

Restioson commented Mar 20, 2025

Freax13 commented Mar 20, 2025

Restioson commented Oct 17, 2020 •

edited

Loading

Restioson Oct 17, 2020 •

edited

Loading

Restioson Oct 17, 2020 •

edited

Loading

Restioson Oct 19, 2020 •

edited

Loading

Restioson Oct 17, 2020 •

edited

Loading