recursive page table with recursive index 511 is unsound

[Freax13]

Constructing a recursive page table with recursive index 511 is unsound because calculating the end pointer of the page table causes an overflow. This isn't technically an issue caused by this crate because we don't create references to recursive page tables but only consume them, but I still feel like the RecursivePageTable type encourages users to just contruct a reference to the recursive page table by deriving the address from the page table index and unfortunately 511 seems to be a common choice for the recursive index.

I'm not sure if there is anything we can do about this other than warning against using index 511 in the documentation.

I have seen miscompilations when the recursive page table address was hard-coded to 0xffff_ffff_ffff_f000 i.e. the recursive index was 511.

[phil-opp]

Thanks for reporting this! Could you clarify where the actual overflow happens? Where do we calculate the end pointer of the page table?

[Freax13]

PageTable::iter returns a core::slice::Iter which contains the end pointer as one of its fields. As an example this code compiles to a single ud2 instruction:

use x86_64::structures::paging::{Page, PageTable, PageTableIndex};

pub fn access_page_table() {
    let index = PageTableIndex::new(511);
    let table = unsafe { recursive_table_from_index(index) };
    for entry in table.iter().skip(1) {
        dbg!(entry);
    }
}

unsafe fn recursive_table_from_index(index: PageTableIndex) -> &'static mut PageTable {
    let page = Page::from_page_table_indices(index, index, index, index);
    unsafe { &mut *page.start_address().as_mut_ptr() }
}

It's also possible to convert a mutable reference to the page table to a slice at which point it's pretty easy to implicitly calculate the end pointer again e.g. by iterating. This code also compiles to a single ud2 instruction:

use x86_64::structures::paging::{Page, PageTable, PageTableIndex};

pub fn access_page_table() {
    let index = PageTableIndex::new(511);
    let table = unsafe { recursive_table_from_index(index) };

    let slice = core::slice::from_mut(table);
    for table in slice {
        dbg!(table);
    }
}

unsafe fn recursive_table_from_index(index: PageTableIndex) -> &'static mut PageTable {
    let page = Page::from_page_table_indices(index, index, index, index);
    unsafe { &mut *page.start_address().as_mut_ptr() }
}

[phil-opp]

Thanks for explaining! I'm not sure if we can do anything to prevent the issue in your second example. We should definitely fix the first example though. I think we can even do this as a non-breaking change by creating our own iterator type because iter and iter_mut currently return an opaque impl Iterator type.

[Freax13]

Thanks for explaining! I'm not sure if we can do anything to prevent the issue in your second example. We should definitely fix the first example though. I think we can even do this as a non-breaking change by creating our own iterator type because iter and iter_mut currently return an opaque impl Iterator type.

That should work, yes. I'd still like to add a warning against using index 511 to RecursivePageTable::new.

[phil-opp]

Sure, I'm fine with that. I still think that we should try to fix all overflow issues on our end, if possible. Did you find any other sources for overflows except for the slice iterators?

[Freax13]

I'm not aware of more any overflows. I'll check if there are any more later today and create a pr with fixes.

[phil-opp]

Thanks a lot!