linux-kernel - Re: [PATCH v4 04/28] rust: alloc: implement `Allocator` for `Kmalloc`

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZrP90NR1lOpDrQ0X@pollux>
Date: Thu, 8 Aug 2024 01:05:52 +0200
From: Danilo Krummrich <dakr@...nel.org>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: ojeda@...nel.org, alex.gaynor@...il.com, wedsonaf@...il.com,
	boqun.feng@...il.com, gary@...yguo.net, bjorn3_gh@...tonmail.com,
	a.hindborg@...sung.com, aliceryhl@...gle.com,
	akpm@...ux-foundation.org, daniel.almeida@...labora.com,
	faith.ekstrand@...labora.com, boris.brezillon@...labora.com,
	lina@...hilina.net, mcanal@...lia.com, zhiw@...dia.com,
	acurrid@...dia.com, cjia@...dia.com, jhubbard@...dia.com,
	airlied@...hat.com, ajanulgu@...hat.com, lyude@...hat.com,
	linux-kernel@...r.kernel.org, rust-for-linux@...r.kernel.org,
	linux-mm@...ck.org
Subject: Re: [PATCH v4 04/28] rust: alloc: implement `Allocator` for `Kmalloc`

On Wed, Aug 07, 2024 at 08:15:41PM +0000, Benno Lossin wrote:
> > So, that's not permitted. `free` can't be called with a dangling pointer. The
> > kernel free functions (*1) can't handle it, and I can't detect it, since a
> > dangling pointer does not have a descrete value.
> 
> That is true, but only if we do not have access to the old layout of the
> allocation. If we add `old_layout` as a parameter, then we can handle
> dangling pointers.

Then we'd need `free` to be `fn free(ptr: NonNull<u8>, layout: Layout)` just to
compare whether `ptr.as_ptr() == layout.align() as _`. Same for `realloc`, but
that's less weird.

It's also not that we safe code with that. `Box`, `Vec`, any other user, still
would have to create the `Layout` before they call `A::free`.

> > Surely, we could also let the caller pass the old alignment, but this all sounds
> > complicated for something that is very trivial for the caller to take care of,
> > i.e. just don't try to free something that was never actually allocated.
> > 
> > It can also lead to subtle bugs, e.g. what if someone calls `Box::from_raw` for
> > a ZST with some other random pointer? Currently, that doesn't hurt us, which for
> > robustness, seems to be a good thing.
> 
> I think we should forbid that. To me it's just plain wrong to take a
> random integer literal and cast it to a ZST. IIRC it even is UB if that
> points to a previously allocated object that has been freed (but I don't
> remember where I read it...).

I think my argument about robustness still holds even if we forbid it.

The documentation says "For operations of size zero, every pointer is valid,
including the null pointer." [1]

[1] https://doc.rust-lang.org/std/ptr/index.html

> 
> Also if we use the size of the old layout instead of comparing alignment
> with the address of the pointer, then we avoid this issue.

That is just another problem when passing the old `Layout` (or maybe just the
old size as usize). Neither do we need the old size, nor do we honor it with any
kernel allocator. This has the following implications:

(1) We never see any error if the old size that is passed is garbage (unless
    it's non-zero, when it should be zero and vice versa), which is bad.

(2) We'd need `free` to be `fn free(ptr: NonNull<u8>, size: usize)`, which is
    confusing, because it implies that an actual free relies on this size for
    freeing the memory.

If we want to avoid (1) and (2), we'd need to make it
`fn free(ptr: NonNull<u8>, zero: bool)` instead, but then also the caller can
just check this boolean and conditionally call `free`.

I don't really see why it's better to let `free` do the `if !zero` check.