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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Aug 15, 2025 at 09:28:59AM +0200, Benno Lossin wrote:
> 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`.

Nah, I wouldn't worry about that, mmap() doesn't seem to cause security
issues as by definition it is just allowing userspace to read/write
anything to that device or memory, and the kernel doesn't even see it.
Because of that, the kernel can't really be "broken" with invalid data
there (hardware can, of course, but that's userspace's fault the kernel
is just setting up a pipe here.)

> >> 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>

I think the problem with this is you now end up with the "I verified
this is ok, so now I will copy it" bug, where userspace will race with
the kernel and modify the data after verification but before copying.

Note, Windows is full of these types of bugs as they don't do a call to
copy_from_user(), but usually just poke at the data directly.  Linux at
least forces a copy_from_user() call, but it doesn't always work in that
you still have to validate the data is correct and people forget.

So as long as we can copy the data from userspace first, and then
validate it, and keep that validated copy around to use, that's great.
I can't really determine above if that is the case or not, sorry.

thanks,

greg k-h