From 451b78bb73d1a26e6090baedcaebc2f7205dcec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lanteri=20Thauvin?= Date: Sun, 1 Aug 2021 02:01:01 +0200 Subject: [PATCH 1/4] Add some missing `// SAFETY` comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Léo Lanteri Thauvin --- rust/kernel/miscdev.rs | 3 ++- rust/kernel/pages.rs | 24 ++++++++++++++++++++---- rust/kernel/print.rs | 4 +++- 3 files changed, 25 insertions(+), 6 deletions(-) diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index cf0cabe95dd1e1..d1ded9c056a0eb 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -147,7 +147,7 @@ impl Registration { open_data: T::OpenData, opts: &Options<'_>, ) -> Result { - // SAFETY: We must ensure that we never move out of `this`. + // SAFETY: we don't move out of `this`. let this = unsafe { self.get_unchecked_mut() }; if this.registered { // Already registered. @@ -170,6 +170,7 @@ impl Registration { this.registered = true; this.open_data.write(open_data); + // SAFETY: FFI call. let ret = unsafe { bindings::misc_register(&mut this.mdev) }; if ret < 0 { // INVARIANT: `registered` is set back to `false` and the `open_data` is destructued. diff --git a/rust/kernel/pages.rs b/rust/kernel/pages.rs index de8358629fdd95..219113b731702c 100644 --- a/rust/kernel/pages.rs +++ b/rust/kernel/pages.rs @@ -82,7 +82,7 @@ impl Pages { /// /// # Safety /// - /// Callers must ensure that the destination buffer is valid for the given length. + /// Callers must ensure that the destination buffer is valid for writes for the given length. /// Additionally, if the raw buffer is intended to be recast, they must ensure that the data /// can be safely cast; [`crate::io_buffer::ReadableFromBytes`] has more details about it. pub unsafe fn read(&self, dest: *mut u8, offset: usize, len: usize) -> Result { @@ -93,7 +93,15 @@ impl Pages { } let mapping = self.kmap(0).ok_or(Error::EINVAL)?; - unsafe { ptr::copy((mapping.ptr as *mut u8).add(offset), dest, len) }; + // SAFETY: we just checked that `offset + len <= PAGE_SIZE`, so `mapping.ptr` and + // `mapping.ptr + offset` are in the same allocated object, the page we're + // reading from, with no overflow. Also, `offset <= PAGE_LEN < isize::MAX` + // so `offset` can't overflow an `isize`. + let src = unsafe { (mapping.ptr as *mut u8).add(offset) }; + // SAFETY: `src` is valid for reads from the type invariants, and `dest` is + // guaranteed by the caller to be valid for writes. Because we're copying `u8`s + // which have an alignment of 1, `src` and `dest` are always properly aligned. + unsafe { ptr::copy(src, dest, len) }; Ok(()) } @@ -101,7 +109,7 @@ impl Pages { /// /// # Safety /// - /// Callers must ensure that the buffer is valid for the given length. Additionally, if the + /// Callers must ensure that the source buffer is valid for reads for the given length. Additionally, if the /// page is (or will be) mapped by userspace, they must ensure that no kernel data is leaked /// through padding if it was cast from another type; [`crate::io_buffer::WritableToBytes`] has /// more details about it. @@ -113,7 +121,15 @@ impl Pages { } let mapping = self.kmap(0).ok_or(Error::EINVAL)?; - unsafe { ptr::copy(src, (mapping.ptr as *mut u8).add(offset), len) }; + // SAFETY: we just checked that `offset + len <= PAGE_SIZE`, so `mapping.ptr` and + // `mapping.ptr + offset` are in the same allocated object, the page we're + // reading from, with no overflow. Also, `offset <= PAGE_LEN < isize::MAX` + // so `offset` can't overflow an `isize`. + let dest = unsafe { (mapping.ptr as *mut u8).add(offset) }; + // SAFETY: `src` is guaranteed by the caller to be valid for reads, and `dest` is + //valid for writes from the type invariants. Because we're copying `u8`s + // which have an alignment of 1, `src` and `dest` are always properly aligned. + unsafe { ptr::copy(src, dest, len) }; Ok(()) } diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index 328d893f87aafe..10dd83a5f0c535 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -33,7 +33,7 @@ unsafe fn rust_fmt_argument(buf: *mut c_char, end: *mut c_char, ptr: *const c_vo // `buf` goes past `end`. let len_to_copy = cmp::min(buf_new, self.end).saturating_sub(self.buf); - // SAFETY: In any case, `buf` is non-null and properly aligned. + // SAFETY: The caller guarantees `buf` is non-null and properly aligned. // If `len_to_copy` is non-zero, then we know `buf` has not past // `end` yet and so is valid. unsafe { @@ -53,6 +53,7 @@ unsafe fn rust_fmt_argument(buf: *mut c_char, end: *mut c_char, ptr: *const c_vo buf: buf as _, end: end as _, }; + // SAFETY: the caller must guarantee that `ptr` is valid. let _ = w.write_fmt(unsafe { *(ptr as *const fmt::Arguments<'_>) }); w.buf as _ } @@ -132,6 +133,7 @@ pub unsafe fn call_printk( args: fmt::Arguments<'_>, ) { // `_printk` does not seem to fail in any path. + // SAFETY: FFI call. unsafe { bindings::_printk( format_string.as_ptr() as _, From 68821be78080625e42c826cf3f3e85e710e70818 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lanteri=20Thauvin?= Date: Tue, 2 Nov 2021 17:02:41 +0100 Subject: [PATCH 2/4] Address review comments --- rust/kernel/miscdev.rs | 4 +++- rust/kernel/pages.rs | 4 ++-- rust/kernel/print.rs | 4 +++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/rust/kernel/miscdev.rs b/rust/kernel/miscdev.rs index d1ded9c056a0eb..bc6b5903ab09b2 100644 --- a/rust/kernel/miscdev.rs +++ b/rust/kernel/miscdev.rs @@ -170,7 +170,9 @@ impl Registration { this.registered = true; this.open_data.write(open_data); - // SAFETY: FFI call. + // SAFETY: the `fops`, `name` and `minor` fields of `this.mdev` have + // been properly initialized above (the other fields are zeroed and will be + // initialized by `misc_register`). let ret = unsafe { bindings::misc_register(&mut this.mdev) }; if ret < 0 { // INVARIANT: `registered` is set back to `false` and the `open_data` is destructued. diff --git a/rust/kernel/pages.rs b/rust/kernel/pages.rs index 219113b731702c..1bdc9704bee2d2 100644 --- a/rust/kernel/pages.rs +++ b/rust/kernel/pages.rs @@ -95,7 +95,7 @@ impl Pages { let mapping = self.kmap(0).ok_or(Error::EINVAL)?; // SAFETY: we just checked that `offset + len <= PAGE_SIZE`, so `mapping.ptr` and // `mapping.ptr + offset` are in the same allocated object, the page we're - // reading from, with no overflow. Also, `offset <= PAGE_LEN < isize::MAX` + // reading from, with no overflow. Also, `offset <= PAGE_SIZE < isize::MAX` // so `offset` can't overflow an `isize`. let src = unsafe { (mapping.ptr as *mut u8).add(offset) }; // SAFETY: `src` is valid for reads from the type invariants, and `dest` is @@ -123,7 +123,7 @@ impl Pages { let mapping = self.kmap(0).ok_or(Error::EINVAL)?; // SAFETY: we just checked that `offset + len <= PAGE_SIZE`, so `mapping.ptr` and // `mapping.ptr + offset` are in the same allocated object, the page we're - // reading from, with no overflow. Also, `offset <= PAGE_LEN < isize::MAX` + // reading from, with no overflow. Also, `offset <= PAGE_SIZE < isize::MAX` // so `offset` can't overflow an `isize`. let dest = unsafe { (mapping.ptr as *mut u8).add(offset) }; // SAFETY: `src` is guaranteed by the caller to be valid for reads, and `dest` is diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index 10dd83a5f0c535..d93deb725e92fb 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -133,7 +133,9 @@ pub unsafe fn call_printk( args: fmt::Arguments<'_>, ) { // `_printk` does not seem to fail in any path. - // SAFETY: FFI call. + // SAFETY: references are guaranteed to be valid for reads. + // The caller guarantees that `format_string` is a valid format + // specifier and that `module_name` is null-terminated. unsafe { bindings::_printk( format_string.as_ptr() as _, From f82aef79137dfd046c1b3b042256dcf26eb7bdd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lanteri=20Thauvin?= Date: Wed, 3 Nov 2021 22:37:00 +0100 Subject: [PATCH 3/4] Document the safety requirements of `rust_fmt_argument` --- rust/kernel/print.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index d93deb725e92fb..7fbab45e8e07d2 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -12,7 +12,12 @@ use core::fmt; use crate::bindings; use crate::c_types::{c_char, c_void}; -// Called from `vsprintf` with format specifier `%pA`. +/// Called from `vsprintf` with format specifier `%pA`. +/// +/// # Safety +/// +/// The region between `buf` and `end` must be valid for writes. +/// `ptr` must point to a valid instance of `fmt::Arguments`. #[no_mangle] unsafe fn rust_fmt_argument(buf: *mut c_char, end: *mut c_char, ptr: *const c_void) -> *mut c_char { use fmt::Write; From feb20cfc34d32244a1a08a57f402e7d885d55969 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lanteri=20Thauvin?= Date: Wed, 2 Feb 2022 09:30:01 +0100 Subject: [PATCH 4/4] Address review comments --- rust/kernel/pages.rs | 2 +- rust/kernel/print.rs | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/rust/kernel/pages.rs b/rust/kernel/pages.rs index 1bdc9704bee2d2..f5847a81fc5b8d 100644 --- a/rust/kernel/pages.rs +++ b/rust/kernel/pages.rs @@ -127,7 +127,7 @@ impl Pages { // so `offset` can't overflow an `isize`. let dest = unsafe { (mapping.ptr as *mut u8).add(offset) }; // SAFETY: `src` is guaranteed by the caller to be valid for reads, and `dest` is - //valid for writes from the type invariants. Because we're copying `u8`s + // valid for writes from the type invariants. Because we're copying `u8`s // which have an alignment of 1, `src` and `dest` are always properly aligned. unsafe { ptr::copy(src, dest, len) }; Ok(()) diff --git a/rust/kernel/print.rs b/rust/kernel/print.rs index 7fbab45e8e07d2..1eb53cb3d621d3 100644 --- a/rust/kernel/print.rs +++ b/rust/kernel/print.rs @@ -138,9 +138,7 @@ pub unsafe fn call_printk( args: fmt::Arguments<'_>, ) { // `_printk` does not seem to fail in any path. - // SAFETY: references are guaranteed to be valid for reads. - // The caller guarantees that `format_string` is a valid format - // specifier and that `module_name` is null-terminated. + // SAFETY: this is safe by the safety contract. unsafe { bindings::_printk( format_string.as_ptr() as _,