Is this a soundness issue in Ixgbe driver?

[tatetian]

I am wondering if there is a soundness issue in the Ixgbe driver.

There are only two occurrences of unsafe in kernel/ixgbe/src/lib.rs. Both of them creates a Box<_> out of an address within a MappedPages using Box::from_raw.

Theseus/kernel/ixgbe/src/lib.rs

Lines 468 to 480 in ee7688f

	for i in 0..QUEUES_IN_MP {
	// This is safe because we have checked that the number of queues we want to partition from these mapped pages fit into the allocated memory,
	// and that each queue starts at the end of the previous.
	// We also ensure that the backing mapped pages are included in the same struct as the registers, almost as a pseudo OwningRef
	let registers = unsafe {
	Box::from_raw((starting_address.value() + (i * RX_QUEUE_REGISTERS_SIZE_BYTES)) as *mut RegistersRx)
	};
	pointers_to_queues.push(
	IxgbeRxQueueRegisters {
	regs: ManuallyDrop::new(registers),
	backing_pages: shared_mp.clone()
	}
	);

The first sign of a potential memory safety problem is that the usage pattern is against the best practice as documented by Rust std.

More precisely, a value: *mut T that has been allocated with the Global allocator with Layout::for_value(&*value) may be converted into a box using Box::::from_raw(value).
...
In general, the best practice is to only use Box for pointers that originated from the global allocator.

The source address (starting_address.value() + (i * RX_QUEUE_REGISTERS_SIZE_BYTES)) given to Box::from_raw is obviously not allocated by the global allocator, but points to a MMIO region. Box is never meant to represent objects on MMIO regions.

As the source address is not on the heap, the resulting Box<_> must never be dropped. Deallocating an object that is not on the heap is obviously a UB. To prevent this from happening, the driver code (correctly) wraps the Box<_> object into a ManuallyDrop.

         IxgbeRxQueueRegisters { 
             // What if stack unwinding happens here?
             regs: ManuallyDrop::new(registers), 
             backing_pages: shared_mp.clone() 
         }

But here is the problem: what if stack unwinding is triggered after Box::from_raw but before ManuallyDrop by a kill request? This is how Theseus's OSDK paper describes stack unwinding and task killing.

Theseus starts the unwinder only upon a software or hardware exception or a request to kill a task.
...
Theseus can forcibly revoke generic resources by killing and unwinding an uncooperative task.

My understanding is that the process of starting a driver like Ixgbe and the decision of killing tasks is independent to each other. So although it is unlikely that a kill request is received between Box::from_raw and ManuallyDrop in practice, this could happen in theory. In this sense, the driver code is unsound.