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

[Greg KH <gregkh@linuxfoundation.org>]

On Sat, Aug 16, 2025 at 12:22:04PM +0200, Benno Lossin wrote:
> On Fri Aug 15, 2025 at 4:19 PM CEST, Greg KH wrote:
> > 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.)
> 
> I'm guessing that the kernel doesn't usually read these? (if we don't
> have a way to read them from the Rust side, we don't need `Untrusted`)

Correct.

> >> >> 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.
> 
> So if I understand the current API correctly, the `arg` and `cmd`
> parameters are copied from userspace (or rather they are direct
> parameters of the syscall), so you can't have the copy-after-validation
> bug there. Then in the `ioctl` function one currently just looks at
> `cmd` & then decides what to do with `arg`, possibly doing a
> `copy_from_user`.
> 
> In my suggestion for the API, we just change the first part of the
> current approach, combining the two parameters to `ioctl` into a single
> struct & making people parse it using the untrusted API.
> 
> The API that protects you from the copy-after-validation bug is the
> `UserPtr<T>` abstraction. And there I also will add the untrusted API.
> For example the `UserPtr::read_all` function would become:
> 
>     pub fn read_all<A: Allocator>(self, buf: &mut Untrusted<Vec<u8, A>>, flags: Flags) -> Result;
> 
> This means that you can only read the data into an `Untrusted<Vec<u8>>`
> which ensures that the data is validated before use. This API also makes
> it harder to have the copy-after-validation bug, since you have to
> explicitly call `clone_reader`.

Ok, that sounds good!