linux-kernel - Re: [PATCH v2 2/2] rust: miscdevice: add base miscdevice abstraction

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: <af1bf81f-ae37-48b9-87c0-acf39cf7eca7@app.fastmail.com>
Date: Wed, 02 Oct 2024 12:48:12 +0000
From: "Arnd Bergmann" <arnd@...db.de>
To: "Alice Ryhl" <aliceryhl@...gle.com>,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Miguel Ojeda" <ojeda@...nel.org>,
 "Alexander Viro" <viro@...iv.linux.org.uk>,
 "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>
Cc: "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>,
 "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>,
 "Trevor Gross" <tmgross@...ch.edu>, rust-for-linux@...r.kernel.org,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] rust: miscdevice: add base miscdevice abstraction

On Tue, Oct 1, 2024, at 08:22, Alice Ryhl wrote:
> +#[cfg(CONFIG_COMPAT)]
> +unsafe extern "C" fn fops_compat_ioctl<T: MiscDevice>(
> +    file: *mut bindings::file,
> +    cmd: c_uint,
> +    arg: c_ulong,
> +) -> c_long {
> +    // SAFETY: The compat ioctl call of a file can access the private 
> data.
> +    let private = unsafe { (*file).private_data };
> +    // SAFETY: Ioctl calls can borrow the private data of the file.
> +    let device = unsafe { <T::Ptr as ForeignOwnable>::borrow(private) 
> };
> +
> +    match T::compat_ioctl(device, cmd as u32, arg as usize) {
> +        Ok(ret) => ret as c_long,
> +        Err(err) => err.to_errno() as c_long,
> +    }
> +}

I think this works fine as a 1:1 mapping of the C API, so this
is certainly something we can do. On the other hand, it would be
nice to improve the interface in some way and make it better than
the C version.

The changes that I think would be straightforward and helpful are:

- combine native and compat handlers and pass a flag argument
  that the callback can check in case it has to do something
  special for compat mode

- pass the 'arg' value as both a __user pointer and a 'long'
  value to avoid having to cast. This specifically simplifies
  the compat version since that needs different types of
  64-bit extension for incoming 32-bit values.

On top of that, my ideal implementation would significantly
simplify writing safe ioctl handlers by using the information
encoded in the command word:

 - copy the __user data into a kernel buffer for _IOW()
   and back for _IOR() type commands, or both for _IOWR()
 - check that the argument size matches the size of the
   structure it gets assigned to

We have a couple of subsystems in the kernel that already
do something like this, but they all do it differently.
For newly written drivers in rust, we could try to do
this well from the start and only offer a single reliable
way to do it. For drivers implementing existing ioctl
commands, an additional complication is that there are
many command codes that encode incorrect size/direction
data, or none at all.

I don't know if there is a good way to do that last bit
in rust, and even if there is, we may well decide to not
do it at first in order to get something working.

      Arnd