Re: [PATCH v4 06/16] rust: drm: restrict AlwaysRefCounted to Normal GEM Object context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Missing `gem_create_object` callback in `shmem::Object` causes the DRM core to allocate bare C structs, leading to out-of-bounds reads/writes when Rust drivers access them via `lookup_handle`.
--

commit 21c57b4cda527522a0ac16331bcf2d0d3de21563
Author: Danilo Krummrich <dakr@kernel.org>
Subject: rust: drm: restrict AlwaysRefCounted to Normal GEM Object context

Restrict AlwaysRefCounted for gem::Object and gem::shmem::Object to the Normal
context. This simplifies generic parameters, removes AlwaysRefCounted from
IntoGEMObject's supertraits, and adds it as an explicit bound on
lookup_handle().

> diff --git a/rust/kernel/drm/gem/mod.rs b/rust/kernel/drm/gem/mod.rs
> index 1023ddccd7857..d56cbe2663e29 100644
> --- a/rust/kernel/drm/gem/mod.rs
> +++ b/rust/kernel/drm/gem/mod.rs

[ ... ]

> @@ -198,8 +196,8 @@ fn create_handle<D, F>(&self, file: &drm::File<F>) -> Result<u32>
>      /// Looks up an object by its handle for a given `File`.
>      fn lookup_handle<D, F>(file: &drm::File<F>, handle: u32) -> Result<ARef<Self>>
>      where
> -        Self: AllocImpl<Driver = D>,
> -        D: drm::Driver<Object<Normal> = Self, File = F>,
> +        Self: AllocImpl<Driver = D> + AlwaysRefCounted,
> +        D: drm::Driver<Object = Self, File = F>,
>          F: drm::file::DriverFile<Driver = D>,
>      {

[Severity: Critical]
This is a pre-existing issue, but since we are modifying the bounds on
lookup_handle(), could the current implementation of shmem::Object lead to
out-of-bounds heap accesses?

In rust/kernel/drm/gem/shmem.rs, ALLOC_OPS sets gem_create_object to None
while enabling dumb_create and gem_prime_import_sg_table:

rust/kernel/drm/gem/shmem.rs:ALLOC_OPS {
    gem_create_object: None,
    ...
    gem_prime_import_sg_table: Some(bindings::drm_gem_shmem_prime_import_sg_table),
    dumb_create: Some(bindings::drm_gem_shmem_dumb_create),
}

When userspace creates a dumb buffer, __drm_gem_shmem_create() sees a NULL
gem_create_object and falls back to a pure C allocation using
kzalloc(sizeof(*shmem), GFP_KERNEL). This allocation lacks the trailing
Rust wrapper fields (inner and parent_resv_obj).

If a Rust driver later retrieves this handle via Object::lookup_handle(),
it casts the C-allocated struct back to the larger Rust wrapper:

rust/kernel/drm/gem/mod.rs:lookup_handle() {
    let ptr = unsafe { bindings::drm_gem_object_lookup(file.as_raw().cast(), handle) };
    if ptr.is_null() {
        return Err(ENOENT);
    }

    let obj = unsafe { Self::from_raw(ptr) };
    ...
}

Does this cause any subsequent access to obj.inner or obj.parent_resv_obj
by the Rust driver to read or write out of bounds on the kernel heap?

Should shmem::Object provide a gem_create_object callback to ensure the
full Rust struct is always allocated?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620184924.2247517-1-dakr@kernel.org?part=6