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 Descriptor::dpl const method and use it in GDT construction #410

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

josephlr
Copy link
Contributor

@josephlr josephlr commented Mar 8, 2023

This requires making PrivilegeLevel::from_u16 const. That, in turn, requires making the panic message slightly less informative.

Note that this is also a functional change when building a GDT. Before, if you had a User/System Segment, the RPL would always be 0, unless it was a User Segment with a DPL of 3, in which case the RPL would be 3. Now the RPL will consistently match the DPL. This seems like the more intuitive approach. However, I doubt it makes much of a difference in practice (as the RPL mechanism is sort of useless to begin with).

If this functional change is an issue, I can add in code (and documentation) to explicitly define the DPL -> RPL mapping.

THis requires making `PrivilegeLevel::from_u16` const

Signed-off-by: Joe Richey <joerichey@google.com>
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Looks good!

That, in turn, requires making the panic message slightly less informative.

That's fine with me.

Note that this is also a functional change when building a GDT. Before, if you had a User/System Segment, the RPL would always be 0, unless it was a User Segment with a DPL of 3, in which case the RPL would be 3. Now the RPL will consistently match the DPL. This seems like the more intuitive approach. However, I doubt it makes much of a difference in practice (as the RPL mechanism is sort of useless to begin with).

As I understand it, the CPU only checks that RPL<=DPL on access, so setting RPL=DPL instead of RPL=0 should not change anything. Right?

@Freax13
Copy link
Member

Freax13 commented Mar 8, 2023

Note that this is also a functional change when building a GDT. Before, if you had a User/System Segment, the RPL would always be 0, unless it was a User Segment with a DPL of 3, in which case the RPL would be 3. Now the RPL will consistently match the DPL. This seems like the more intuitive approach. However, I doubt it makes much of a difference in practice (as the RPL mechanism is sort of useless to begin with).

As I understand it, the CPU only checks that RPL<=DPL on access, so setting RPL=DPL instead of RPL=0 should not change anything. Right?

Yes, that is correct for data segments. However, accessing stack segments requires the CPL and the RPL and DPL of the stack segment to be equal. The changes introduced here should make it possible to access stack segments with a CPL of 1 and 2.

Copy link
Member

@Freax13 Freax13 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks!

@josephlr
Copy link
Contributor Author

josephlr commented Mar 8, 2023

Added some comments clarifying the DPL_RING_3 flag, removing the the comment saying that the DPL is ignored in 64-bit mode data segments, explaining the exception for stack segments.

src/structures/gdt.rs Outdated Show resolved Hide resolved
Clarify the DPL_RING_3 flag, remove the the comment saying that the DPL
is ignored in 64-bit mode data segments, explain the exception for stack
segments.

Signed-off-by: Joe Richey <joerichey@google.com>
@josephlr josephlr enabled auto-merge March 8, 2023 22:25
@josephlr josephlr merged commit 2d41827 into master Mar 8, 2023
@josephlr josephlr deleted the dpl branch March 8, 2023 22:25
@Freax13 Freax13 mentioned this pull request Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants