`idt::Entry::set_handler_addr` causes unpredictable behavior due to it reading the current value of the `CS` register

[ChocolateLoverRaj]

I recently am re-writing part of my kernel to work with Limine and multiple CPUs. I set up the GDT and IDT and then did a int3 to test out the handler. It caused a triple fault. The cause of the problem turned out to be that Limine uses a different GDT layout than the GDT that my kernel loads, and a different CS register value. Since I was adding entries to a InterruptDescriptorTable before loading my kernel's GDT, the entries were using Limine's value for CS. So I saw

pub unsafe fn set_handler_addr(&mut self, addr: VirtAddr) -> &mut EntryOptions {
    use crate::instructions::segmentation::{Segment, CS};

    let addr = addr.as_u64();
    self.pointer_low = addr as u16;
    self.pointer_middle = (addr >> 16) as u16;
    self.pointer_high = (addr >> 32) as u32;

    self.options = EntryOptions::minimal();
    // SAFETY: The current CS is a valid, long-mode code segment.
    unsafe { self.options.set_code_selector(CS::get_reg()) };
    self.options.set_present(true);
    &mut self.options
}

and it is loading the current CS value which I think can cause many unexpected bugs, especially since the function doesn't even document that it does this. Imo the IDT Rust wrapper should have consistent behavior and its value should not change depending on what GDT is loaded before it. My suggestion would be to change it to something like this:

pub unsafe fn set_handler_addr(&mut self, addr: VirtAddr, cs: SegmentSelector)  -> &mut EntryOptions

that way when setting up a GDT and IDT, the following things can be done:

• The GDT is created
• The IDT is created with entries referencing the CS according to the created GDT
• The GDT is loaded
• The IDT is loaded

And even though the change I'm requesting is a breaking change I think it's worth it. If you don't want a breaking change then I would request adding functions like set_handler_addr_with_cs and set_handler_fn_with_cs in addition to the current functions.

I kind of improved this in https://github.com/ChocolateLoverRaj/x86_64/commit/2c79ab5c919238ac68eac7fa5dad92f6a73f5512 but it is a messy commit

[Freax13]

[...] especially since the function doesn't even document that it does this.

This behavior is documented: https://github.com/rust-osdev/x86_64/blob/f01a291ecb0c1c8e8a5f5d58c29fbd9588a1486c/src/structures/idt.rs#L813

Imo the IDT Rust wrapper should have consistent behavior and its value should not change depending on what GDT is loaded before it. My suggestion would be to change it to something like this:

pub unsafe fn set_handler_addr(&mut self, addr: VirtAddr, cs: SegmentSelector) -> &mut EntryOptions

You can overwrite the CS segment with EntryOptions::set_code_selector.

[ChocolateLoverRaj]

Ig in my brain I did not make the connection between "code selector" and CS. I still think it would be more proper to have an option not to even read CS in the first place instead of overwriting it afterwards.

[Freax13]

Using the current CS value is a reasonable default. We also provide defaults for other fields. I don't think replacing all these defaults with explicit arguments in set_handler_addr is worth it. A lot of users will just stick with the defaults because that's all that they need.

[Freax13]

Can we close this?

[ChocolateLoverRaj]

Yes