Re: [PATCH] rust: alloc: fix dangling pointer in VecExt<T>::reserve()

[Boqun Feng <boqun.feng@gmail.com>]

On Mon, Apr 29, 2024 at 09:24:04PM +0200, Danilo Krummrich wrote:
> Currently, a Vec<T>'s ptr value, after calling Vec<T>::new(), is
> initialized to Unique::dangling(). Hence, in VecExt<T>::reserve(), we're
> passing a dangling pointer (instead of NULL) to krealloc() whenever a
> new Vec<T> is created through VecExt<T> extension functions.
> 
> This only works since it happens that Unique::dangling()'s value (0x1)
> falls within the range between 0x0 and ZERO_SIZE_PTR (0x10) and
> krealloc() hence treats it the same as a NULL pointer however.
> 

Good catch!

> This isn't a case we should rely on, especially since other kernel
> allocators are not as tolerant. Instead, pass a real NULL pointer to
> krealloc_aligned() if Vec<T>'s capacity is zero.
> 
> Fixes: 5ab560ce12ed ("rust: alloc: update `VecExt` to take allocation flags")

However, since this commit is not upstreamed yet, so it's suject to
change, I'd avoid the "Fixes" tag here. Alternatively, Miguel can fold
this patch into that commit in his tree.

Either way:

Reviewed-by: Boqun Feng <boqun.feng@gmail.com>

Regards,
Boqun

> Signed-off-by: Danilo Krummrich <dakr@redhat.com>
> ---
>  rust/kernel/alloc/vec_ext.rs | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/rust/kernel/alloc/vec_ext.rs b/rust/kernel/alloc/vec_ext.rs
> index 6a916fcf8bf1..ffcf8a19f715 100644
> --- a/rust/kernel/alloc/vec_ext.rs
> +++ b/rust/kernel/alloc/vec_ext.rs
> @@ -4,6 +4,7 @@
>  
>  use super::{AllocError, Flags};
>  use alloc::vec::Vec;
> +use core::ptr;
>  use core::result::Result;
>  
>  /// Extensions to [`Vec`].
> @@ -137,6 +138,15 @@ fn reserve(&mut self, additional: usize, flags: Flags) -> Result<(), AllocError>
>  
>          let (ptr, len, cap) = destructure(self);
>  
> +        // We need to make sure that ptr is either NULL or comes from a previous call to
> +        // `krealloc_aligned`. A `Vec<T>`'s `ptr` value is not guaranteed to be NULL and might be
> +        // dangling after being created with `Vec::new`. Instead, we can rely on `Vec<T>'s capacity
> +        // to be zero if no memory has been allocated yet.
> +        let ptr = match cap {
> +            0 => ptr::null_mut(),
> +            _ => ptr,
> +        };
> +
>          // SAFETY: `ptr` is valid because it's either NULL or comes from a previous call to
>          // `krealloc_aligned`. We also verified that the type is not a ZST.
>          let new_ptr = unsafe { super::allocator::krealloc_aligned(ptr.cast(), layout, flags) };
> 
> base-commit: 2c1092853f163762ef0aabc551a630ef233e1be3
> -- 
> 2.44.0
>