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

Maybe unsound in Vring::_flags #834

Open
lwz23 opened this issue Dec 10, 2024 · 1 comment
Open

Maybe unsound in Vring::_flags #834

lwz23 opened this issue Dec 10, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@lwz23
Copy link

lwz23 commented Dec 10, 2024

Hello, thank you for your contribution in this project, I am scanning the unsoundness problem in rust project.
I notice the following code:

pub struct Vring<T> {
	mem: *const u8,
	_marker: PhantomData<*const T>,
}

impl<T> Vring<T> {
	pub fn new(mem: *const u8) -> Self {
		Vring {
			mem,
			_marker: PhantomData,
		}
	}

	pub fn _flags(&self) -> u16 {
		unsafe {
			#[allow(clippy::cast_ptr_alignment)]
			*(self.mem as *const u16)
		}
	}

	pub fn index(&self) -> u16 {
		unsafe {
			#[allow(clippy::cast_ptr_alignment)]
			*(self.mem.offset(2) as *const u16)
		}
	}
............................

Considering that pub mod virtqueue, and new is also a pub function. I assume that users can directly call this function. This potential situation could result in *(self.mem as *const u16) being dereference a null pointer, and directly dereferencing it might trigger undefined behavior (UB). For safety reasons, I felt it necessary to report this issue. If you have performed checks elsewhere that ensure this is safe, please don’t take offense at my raising this issue.
I suggest Several possible fixes:

  1. If there is no external usage for Vring or new, they should not marked as pub, at least its new should not marked as pub
  2. new method should add additional check for null pointer.
  3. mark new method as unsafe and proper doc to let users know that they should provide valid Pointers.
@mkroening
Copy link
Member

That's true. This also applies to #643. The logic also looks overflowy to me. We should make them unsafe or rework them to be sound.

@n0toose n0toose added the bug Something isn't working label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants