linux-kernel - Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust 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: <592946c7-ab76-42c5-977a-fcad04b0c439@app.fastmail.com>
Date: Fri, 06 Dec 2024 12:15:35 +0100
From: "Arnd Bergmann" <arnd@...db.de>
To: "Alice Ryhl" <aliceryhl@...gle.com>
Cc: "Lee Jones" <lee@...nel.org>, linux-kernel@...r.kernel.org,
 "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
 "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>,
 "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
Subject: Re: [PATCH v4 2/4] samples: rust: Provide example using the new Rust
 MiscDevice abstraction

On Fri, Dec 6, 2024, at 11:40, Alice Ryhl wrote:
> On Fri, Dec 6, 2024 at 11:31 AM Arnd Bergmann <arnd@...db.de> wrote:

>> I wonder if we should change the prototype of the ioctl
>> callback to always pass a __user pointer and just not allow
>> the few commands that pass an integer in rust drivers, and
>> worry about it only when it's absolutely needed.
>
> One option is to let the Rust Miscdevice trait have three ioctl methods:
>
> fn ioctl(cmd: u32, arg: UserPtr);
> fn ioctl_raw(cmd: u32, arg: usize);
> fn compat_ioctl(cmd: u32, arg: usize);
>
> Then when building the fops vtable, we do one of:
>
> 1. If `ioctl` is specified, use that implementation with compat_ptr_ioctl().
> 2. If `ioctl_raw` and `compat_ioctl` are specified, use those two
> implementations.
> 3. If none of the above are specified, use null pointers.
> 4. All other cases trigger an error at build time.
>
> Thoughts?

I think we can combine the latter two into

fn ioctl_raw(cmd: u32, arg: usize, compat: bool);

and only need two cases: either all arguments are pointers to
structures with compatible layout and you can use the simple
ioctl() callback, or there is some special case (incompatible
struct layout or integer arguments) and you use the raw
version. I think this would work just fine.

Or we could take it one step further (going back to the
discussion we had the last time this came up) and make
the default version copy the data as well and pass it
as a kernel pointer. In C code this would look roughly
like

long file_ioctl_wrapper(struct file *f, u32 cmd, unsigned long arg)
{
       void *argp;
       void __user *uarg;
       bool compat = in_compat_syscall());
       size_t size = _IOC_SIZE(cmd);
       int ret = -ENOIOCTLCMD;

       /* Get a pointer argument for both native and compat mode */
       if (compat)
              uarg = compat_ptr(arg);
       else
              uarg = (void __user*)arg;

       /*
        * allow .ioctl_raw to provide a custom version for
        * commands that take an integer argument, have an
        * incompatible compat layout or fail to encode size
        * and/or direction correctly in cmd
        * This can return ENOIOCTLCMD to fall back to the
        * simple handler for other commands.
        */       
       if (f->ops->ioctl_raw)
                ret = f->ops->ioctl_raw(f, cmd, uarg, arg, compat);
       if (ret != -ENOIOCTLCMD)
                return ret;

       /* No data, so skip the allocation */
       if (_IOC_DIR(cmd) == _IOC_NONE || size = 0)
                return f->ops->ioctl(f, cmd, NULL);

       argp = kzalloc(size, GFP_KERNEL);
       if (!argp)
             return -ENOMEM;

       /* _IOW or _IOWR, so copy data into the kernel */
       if (_IOC_DIR(cmd) & _IOC_WRITE) {
             if (copy_from_user(argp, uarg, size))
                    return -EFAULT;
       }

       ret = f->ops->ioctl(f, cmd, arg);

       /* _IOR or _IOWR, copy back even after command failure */
       if (_IOC_DIR(cmd) & _IOC_READ) {
             if (copy_to_user(uarg, arg, size))
                    return -EFAULT;
       }

       return ret;
}

With this, every driver that has a properly designed
set of ioctl commands can just use the simple .ioctl()
callback, and any commands that need some special case
can be put in the .ioctl_raw() callback.

If this works out for Rust, we can actually put that exact
code into vfs_ioctl() and add back a .ioctl() callback
into struct file_operations next to the C .unlocked_ioctl
and .compat_ioctl handlers.

    Arnd