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

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

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

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

---
Cheers,
Benno