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: <CAH5fLgjdpF7F03ORSKkb+r3+nGfrnA+q1GKw=KHCHASrkz1NPw@mail.gmail.com>
Date: Wed, 2 Oct 2024 15:36:33 +0200
From: Alice Ryhl <aliceryhl@...gle.com>
To: Christian Brauner <brauner@...nel.org>
Cc: Arnd Bergmann <arnd@...db.de>, Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
	Miguel Ojeda <ojeda@...nel.org>, Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>, 
	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 Wed, Oct 2, 2024 at 3:24 PM Christian Brauner <brauner@...nel.org> wrote:
>
> On Wed, Oct 02, 2024 at 12:48:12PM GMT, Arnd Bergmann wrote:
> > 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
>
> - Handle versioning by size for ioctl()s correctly so stuff like:
>
>         /* extensible ioctls */
>         switch (_IOC_NR(ioctl)) {
>         case _IOC_NR(NS_MNT_GET_INFO): {
>                 struct mnt_ns_info kinfo = {};
>                 struct mnt_ns_info __user *uinfo = (struct mnt_ns_info __user *)arg;
>                 size_t usize = _IOC_SIZE(ioctl);
>
>                 if (ns->ops->type != CLONE_NEWNS)
>                         return -EINVAL;
>
>                 if (!uinfo)
>                         return -EINVAL;
>
>                 if (usize < MNT_NS_INFO_SIZE_VER0)
>                         return -EINVAL;
>
>                 return copy_ns_info_to_user(to_mnt_ns(ns), uinfo, usize, &kinfo);
>         }
>
> This is not well-known and noone versions ioctl()s correctly and if they
> do it's their own hand-rolled thing. Ideally, this would be a first
> class concept with Rust bindings and versioning like this would be
> universally enforced.

Could you point me at some more complete documentation or example of
how to correctly do versioning?

Alice