linux-kernel - Re: [RFC PATCH] drm: panthor: add dev

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: <CAH5fLgitm2qrGn3BBFyspmTD7Km+pp2qbvA9GN4fjyUnuWffWg@mail.gmail.com>
Date: Tue, 23 Jul 2024 15:45:02 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Daniel Almeida <daniel.almeida@...labora.com>
Cc: Wedson Almeida Filho <wedsonaf@...il.com>, Miguel Ojeda <ojeda@...nel.org>, 
	Danilo Krummrich <dakr@...hat.com>, lyude@...hat.com, robh@...nel.org, lina@...hilina.net, 
	mcanal@...lia.com, airlied@...il.com, rust-for-linux@...r.kernel.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] drm: panthor: add dev_coredumpv support

On Tue, Jul 23, 2024 at 3:41 PM Daniel Almeida
<daniel.almeida@...labora.com> wrote:
>
> Hi Alice, thanks for the review!
>
>
> >> +        fn alloc_mem(&mut self, size: usize) -> Option<*mut u8> {
> >> +            assert!(size % 8 == 0, "Allocation size must be 8-byte aligned");
> >> +            if isize::try_from(size).unwrap() == isize::MAX {
> >> +                return None;
> >> +            } else if self.pos + size > self.capacity {
> >> +                kernel::pr_debug!("DumpAllocator out of memory");
> >> +                None
> >> +            } else {
> >> +                let offset = self.pos;
> >> +                self.pos += size;
> >> +
> >> +                // Safety: we know that this is a valid allocation, so
> >> +                // dereferencing is safe. We don't ever return two pointers to
> >> +                // the same address, so we adhere to the aliasing rules. We make
> >> +                // sure that the memory is zero-initialized before being handed
> >> +                // out (this happens when the allocator is first created) and we
> >> +                // enforce a 8 byte alignment rule.
> >> +                Some(unsafe { self.mem.as_ptr().offset(offset as isize) as *mut u8 })
> >> +            }
> >> +        }
> >> +
> >> +        pub(crate) fn alloc<T>(&mut self) -> Option<&mut T> {
> >> +            let mem = self.alloc_mem(core::mem::size_of::<T>())? as *mut T;
> >> +            // Safety: we uphold safety guarantees in alloc_mem(), so this is
> >> +            // safe to dereference.
> >
> > This code doesn't properly handle when T requires a large alignment.
> >
>
> Can you expand a bit on this? IIRC the alignment of a structure/enum will be dictated
> by the field with the largest alignment requirement, right? Given that the largest primitive
> allowed in the kernel is u64/i64, shouldn’t this suffice, e.g.:

It's possible for Rust types to have a larger alignment using e.g.
#[repr(align(64))].

>  +            assert!(size % 8 == 0, "Allocation size must be 8-byte aligned");
>
>
> >> +            Some(unsafe { &mut *mem })
> >> +        }
> >> +
> >> +        pub(crate) fn alloc_bytes(&mut self, num_bytes: usize) -> Option<&mut [u8]> {
> >> +            let mem = self.alloc_mem(num_bytes)?;
> >> +
> >> +            // Safety: we uphold safety guarantees in alloc_mem(), so this is
> >> +            // safe to build a slice
> >> +            Some(unsafe { core::slice::from_raw_parts_mut(mem, num_bytes) })
> >> +        }
> >
> > Using references for functions that allocate is generally wrong.
> > References imply that you don't have ownership of the memory, but
> > allocator functions would normally return ownership of the allocation.
> > As-is, the code seems to leak these allocations.
>
> All the memory must be given to dev_coredumpv(), which will then take
> ownership.  dev_coredumpv() will free all the memory, so there should be no
> leaks here.
>
> I’ve switched to KVec in v2, so that will also cover the error paths,
> which do leak in this version, sadly.
>
> As-is, all the memory is pre-allocated as a single chunk. When space is carved
> for a given T, a &mut is returned so that the data can be written in-place at
> the right spot in said chunk.
>
> Not only there shouldn’t be any leaks, but I can actually decode this from
> userspace.
>
> I agree that this pattern isn’t usual, but I don’t see anything
> incorrect. Maybe I missed something?

Interesting. So the memory is deallocated when self is destroyed? A
bit unusual, but I agree it is correct if so. Sorry for the confusion
:)

Alice