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

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

[Danilo Krummrich]

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 as long as align_of::<T>(), used by Unique::dangling() to
derive the dangling pointer, resolves to a value between 0x0 and
ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
pointer however.

This isn't a case we should rely on, since there may be types whose
alignment may exceed the range still covered by krealloc(), plus other
kernel allocators are not as tolerant either.

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")
Reviewed-by: Alice Ryhl <aliceryhl@google.com>
Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
Signed-off-by: Danilo Krummrich <dakr@redhat.com>
---
Changes in v2:
  - correct the description of the pointer value produced by
    Unique::dangling() (Alice)
  - add RB tags (Alice, Boqun)
---
 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

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

[Wedson Almeida Filho]

On Tue, 30 Apr 2024 at 09:13, Danilo Krummrich <dakr@redhat.com> 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.

Nice catch, thanks!

A small nit on the wording above: this applies to any Vec<T>, not just
those created via VecExt<T>. For example, if we create with
Vec::new(), then use VecExt::push or VecExt::reserve, we'll run into
this. (IOW, which function is used to create the Vec is not a factor,
using VecExt to extend it from 0 to >0 is.)

>
> This only works as long as align_of::<T>(), used by Unique::dangling() to
> derive the dangling pointer, resolves to a value between 0x0 and
> ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
> pointer however.
>
> This isn't a case we should rely on, since there may be types whose
> alignment may exceed the range still covered by krealloc(), plus other
> kernel allocators are not as tolerant either.

Perhaps it would make sense to add a sample with such a type?

It would serve as a test as well when kunit and doctests are enabled.

>          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,
> +        };
> +

nit: why did you choose to use a match here? I don't think C
programmers would use a switch to determine if a value is zero or
non-zero.

I would have written:

let ptr = if cap != 0 { ptr } else { ptr::null_mut() };

>          // 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) };

In the case when reallocation fails, we need to rebuild with the
original pointer (the dangling one if cap is zero). This patch is
rebuilding it with null when cap is zero, which is incorrect.

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

[Danilo Krummrich]

On Tue, Apr 30, 2024 at 04:25:44PM -0300, Wedson Almeida Filho wrote:
> On Tue, 30 Apr 2024 at 09:13, Danilo Krummrich <dakr@redhat.com> 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.
> 
> Nice catch, thanks!
> 
> A small nit on the wording above: this applies to any Vec<T>, not just
> those created via VecExt<T>. For example, if we create with

True, I think I wanted to say "whenever a new Vec<T>'s backing storage is
allocated through VecExt<T> extension functions".

> Vec::new(), then use VecExt::push or VecExt::reserve, we'll run into
> this. (IOW, which function is used to create the Vec is not a factor,
> using VecExt to extend it from 0 to >0 is.)
> 
> >
> > This only works as long as align_of::<T>(), used by Unique::dangling() to
> > derive the dangling pointer, resolves to a value between 0x0 and
> > ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
> > pointer however.
> >
> > This isn't a case we should rely on, since there may be types whose
> > alignment may exceed the range still covered by krealloc(), plus other
> > kernel allocators are not as tolerant either.
> 
> Perhaps it would make sense to add a sample with such a type?
> 
> It would serve as a test as well when kunit and doctests are enabled.

Yeah, that sounds reasonable. Let me see whether I can get to that.

> 
> >          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,
> > +        };
> > +
> 
> nit: why did you choose to use a match here?

I felt like it reads nicely.

> I don't think C
> programmers would use a switch to determine if a value is zero or
> non-zero.

Well, I think for writing Rust code it doesn't matter too much what C
programmers would do? ;-)

What is idiomatic in this case?

> 
> I would have written:
> 
> let ptr = if cap != 0 { ptr } else { ptr::null_mut() };
> 
> >          // 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) };
> 
> In the case when reallocation fails, we need to rebuild with the
> original pointer (the dangling one if cap is zero). This patch is
> rebuilding it with null when cap is zero, which is incorrect.
> 

Indeed, good catch. Gonna fix that up in a v3.

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

[Wedson Almeida Filho]

On Tue, 30 Apr 2024 at 19:04, Danilo Krummrich <dakr@redhat.com> wrote:
>
> On Tue, Apr 30, 2024 at 04:25:44PM -0300, Wedson Almeida Filho wrote:
> > On Tue, 30 Apr 2024 at 09:13, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > >          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,
> > > +        };
> > > +
> >
> > nit: why did you choose to use a match here?
>
> I felt like it reads nicely.
>
> > I don't think C
> > programmers would use a switch to determine if a value is zero or
> > non-zero.
>
> Well, I think for writing Rust code it doesn't matter too much what C
> programmers would do? ;-)
>
> What is idiomatic in this case?

The idiomatic way is to use `if`. Even in cases where a match was
required (e.g., when checking an enum) and only two options are ever
used, the languages has the matches!
(https://doc.rust-lang.org/std/macro.matches.html) macro so you'd do:

if matches!(a, A::X(_)) {
} else {
}

instead of

match a {
    A::X(_) => {
    }
    _ => {
    }
}

They even changed the language with if-let constructs
(https://rust-lang.github.io/rfcs/0160-if-let.html) so that we'd do

if let A::X(_) = a {
} else {
}

instead of the match above. Here's the first paragraph for the
justification for this feature:

"Introduce a new if let PAT = EXPR { BODY } construct. This allows for
refutable pattern matching without the syntactic and semantic overhead
of a full match, and without the corresponding extra rightward drift.
Informally this is known as an “if-let statement”."

So the language designers and std library implementers provide a macro
and a language construct to avoid using matches when needed, but
you're going in the opposite direction where you don't need it but
want to use it anyway.

Please change this.

Thanks,
-Wedson

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

[Danilo Krummrich]

On Tue, May 07, 2024 at 12:24:56AM -0300, Wedson Almeida Filho wrote:
> On Tue, 30 Apr 2024 at 19:04, Danilo Krummrich <dakr@redhat.com> wrote:
> >
> > On Tue, Apr 30, 2024 at 04:25:44PM -0300, Wedson Almeida Filho wrote:
> > > On Tue, 30 Apr 2024 at 09:13, Danilo Krummrich <dakr@redhat.com> wrote:
> > >
> > > >          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,
> > > > +        };
> > > > +
> > >
> > > nit: why did you choose to use a match here?
> >
> > I felt like it reads nicely.
> >
> > > I don't think C
> > > programmers would use a switch to determine if a value is zero or
> > > non-zero.
> >
> > Well, I think for writing Rust code it doesn't matter too much what C
> > programmers would do? ;-)
> >
> > What is idiomatic in this case?
> 
> The idiomatic way is to use `if`. Even in cases where a match was
> required (e.g., when checking an enum) and only two options are ever
> used, the languages has the matches!
> (https://doc.rust-lang.org/std/macro.matches.html) macro so you'd do:
> 
> if matches!(a, A::X(_)) {
> } else {
> }
> 
> instead of
> 
> match a {
>     A::X(_) => {
>     }
>     _ => {
>     }
> }
> 
> They even changed the language with if-let constructs
> (https://rust-lang.github.io/rfcs/0160-if-let.html) so that we'd do
> 
> if let A::X(_) = a {
> } else {
> }
> 
> instead of the match above. Here's the first paragraph for the
> justification for this feature:
> 
> "Introduce a new if let PAT = EXPR { BODY } construct. This allows for
> refutable pattern matching without the syntactic and semantic overhead
> of a full match, and without the corresponding extra rightward drift.
> Informally this is known as an “if-let statement”."
> 

Very valuable information - thanks for sharing!

This is what I was potentially looking for when I was asking "Is there any
benefit using an if here, or is that your personal preference?" in v3.

> So the language designers and std library implementers provide a macro
> and a language construct to avoid using matches when needed, but
> you're going in the opposite direction where you don't need it but
> want to use it anyway.

Please let me clarify this:

It's not really that I want to use it or insist on it. I really just chose it
because I felt like it reads nicely. I did not feel like there is an actual
request for change. Especially, since you phrased it as "nit: why did you choose
to use a match here?", which is a question and even attenuated by the "nit".

When you mentioned in v3 that you "still think this should be an `if`", I
noticed that you might actually want this to be changed and I wondered why.

Me asking "Is there any benefit using an if here, or is that your personal
preference?" wasn't meant to read as if I wouldn't want to change it, but honest
interest in why it's more than just a question and more than a "nit".

Please also see my reply [1] to Miguel.

> 
> Please change this.

Sure, I sent out another patch changing this, since v3 has been merged already [2].
Feel free to melt this one into the original one though.

[1] https://lore.kernel.org/rust-for-linux/b57dde93-06db-405b-ab94-864779c76010@redhat.com/
[2] https://lore.kernel.org/rust-for-linux/CANiq72nSQAE7c8ypWQA__BMoaBYqiEjXvHpuzn-=CsCky9uu7A@mail.gmail.com/

> 
> Thanks,
> -Wedson
> 

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

[Benno Lossin]

On 30.04.24 14:13, 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 as long as align_of::<T>(), used by Unique::dangling() to
> derive the dangling pointer, resolves to a value between 0x0 and
> ZERO_SIZE_PTR (0x10) and krealloc() hence treats it the same as a NULL
> pointer however.
> 
> This isn't a case we should rely on, since there may be types whose
> alignment may exceed the range still covered by krealloc(), plus other
> kernel allocators are not as tolerant either.
> 
> 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")
> Reviewed-by: Alice Ryhl <aliceryhl@google.com>
> Reviewed-by: Boqun Feng <boqun.feng@gmail.com>
> Signed-off-by: Danilo Krummrich <dakr@redhat.com>

Nice find!

Reviewed-by: Benno Lossin <benno.lossin@proton.me>

I don't mind the `match`, but an `if` is also fine.

-- 
Cheers,
Benno