Re: [PATCH v4 0/4] Untrusted Data API

["Benno Lossin" <lossin@kernel.org>]

On Thu Aug 14, 2025 at 8:26 PM CEST, Greg KH wrote:
> On Thu, Aug 14, 2025 at 07:23:45PM +0200, Benno Lossin wrote:
>> On Thu Aug 14, 2025 at 5:42 PM CEST, Greg KH wrote:
>> > On Thu, Aug 14, 2025 at 05:22:57PM +0200, Benno Lossin wrote:
>> >> On Thu Aug 14, 2025 at 4:37 PM CEST, Greg KH wrote:
>> >> > On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>> >> >> I didn't have too much time to spend on this API, so this is mostly a
>> >> >> resend of v3. There are some changes in the last commit, updating to the
>> >> >> latest version of Alice's iov_iter patche series [1] & rebasing on top
>> >> >> of v6.17-rc1.
>> >> >> 
>> >> >> I think we should just merge the first two patches this cycle in order
>> >> >> to get the initial, bare-bones API into the kernel and have people
>> >> >> experiment with it. The validation logic in the third patch still needs
>> >> >> some work and I'd need to find some time to work on that (no idea when I
>> >> >> find it though).
>> >> >
>> >> > Nice, thanks for reviving this!
>> >> >
>> >> > And we should at least add an example using it, otherwise it's not going
>> >> > to help out much here.  Add it to the misc device driver api?
>> >> 
>> >> You mean `rust/kernel/miscdevice.rs`? What parts of that API are
>> >> untrusted? 
>> >
>> > mmap() is, but you can't do anything about that...
>> 
>> Which parameter is untrusted there and why can't I do anything about it?
>
> The whole memory range is untrusted as to what is written there, sorry,
> it was a bad attempt at a joke, the kernel never gets a chance to know
> what is happening.

Ahh that flew over my head :) So we'd either build `Untrusted` directly
into the `VmaNew`/`VmaRef` abstractions -- or if there is a way to have
a trusted `VmaNew`, we can wrap it in `mmap`.

>> > ioctl() is the callback that is taking untrusted data from userspace.
>> > That's one place we have had more kernel buffer overflows then I can
>> > count and ALWAYS needs to be properly verified before anything can be
>> > done with the data there.
>> 
>> Are the `cmd` & `arg` parameters the untrusted part?
>
> Yes, especially as `arg` is usually a pointer to "something".
>
>> If so we probably
>> should have a single parameter so users can verify them at the same
>> time. Or am I thinking of the wrong thing to verify? (`file` should be
>> already in kernel memory, right?)
>
> Both are usually verified at different places, first `cmd` tells what
> `arg` is going to be, and then the code goes off and parses whatever
> `arg` points to (or contains for simple ioctls).
>
> And for some, `arg` is just a place to write something back, so `arg`
> needs no verification for them, it depends on what `cmd` is.

I still think grouping them together makes sense, since in the
validation function you'd want access to both, right? And these use
cases seem perfect for enums:

    enum MyIoctlArgs {
        WriteFoo(UserPtr<Foo>),
        VerifyBar(UserPtr<Bar>),
        // ...
    }

And then in the validation function you can do:

    const WRITE_FOO_CMD: u32 = ...;

    fn validate(cmd: u32, arg: usize) -> Result<MyIoctlArgs> {
        Ok(match cmd {
            WRITE_FOO_CMD => MyIoctlArgs::WriteFoo(UserPtr::from_addr(arg)),
            VERIFY_BAR_CMD => MyIoctlArgs::VerifyBar(UserPtr::from_addr(arg)),
            _ => return Err(EINVAL),
        })
    }

So when wrapping them together we could have:

    pub struct IoctlArgs {
        pub cmd: u32,
        pub arg: usize,
    }

And then change the `MiscDevice::ioctl` function to:

    fn ioctl(
        _device: <Self::Ptr as ForeignOwnable>::Borrowed<'_>,
        _file: &File,
        _args: Untrusted<IoctlArgs>,
    ) -> Result<isize>

---
Cheers,
Benno