Re: [PATCH 0/5] Rust support for `struct iov_iter`

[Andreas Hindborg <a.hindborg@kernel.org>]

"Greg Kroah-Hartman" <gregkh@linuxfoundation.org> writes:

> On Wed, Mar 19, 2025 at 12:10:03PM +0100, Andreas Hindborg wrote:
>> "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> writes:
>>
>> > On Tue, Mar 18, 2025 at 09:57:55PM +0100, Andreas Hindborg wrote:
>> >> "Greg Kroah-Hartman" <gregkh@linuxfoundation.org> writes:
>> >>
>> >> > On Tue, Mar 11, 2025 at 02:25:11PM +0000, Alice Ryhl wrote:
>> >> >> This series adds support for the `struct iov_iter` type. This type
>> >> >> represents an IO buffer for reading or writing, and can be configured
>> >> >> for either direction of communication.
>> >> >>
>> >> >> In Rust, we define separate types for reading and writing. This will
>> >> >> ensure that you cannot mix them up and e.g. call copy_from_iter in a
>> >> >> read_iter syscall.
>> >> >>
>> >> >> To use the new abstractions, miscdevices are given new methods read_iter
>> >> >> and write_iter that can be used to implement the read/write syscalls on
>> >> >> a miscdevice. The miscdevice sample is updated to provide read/write
>> >> >> operations.
>> >> >
>> >> > Nice, this is good to have, but what's the odds of tieing in the
>> >> > "untrusted buffer" logic here so that all misc drivers HAVE to properly
>> >> > validate the data sent to them before they can touch it:
>> >> > 	https://lore.kernel.org/r/20240925205244.873020-1-benno.lossin@proton.me
>> >> >
>> >> > I'd like to force drivers to do this, otherwise it's just going to force
>> >> > us to audit all paths from userspace->kernel that happen.
>> >> >
>> >>
>> >> I think that for user backed iterators (`user_backed_iter(iter) != 0`)
>> >> we will have the same problems as discussed in [1]. To validate, we
>> >> would have to copy the data to another buffer and then validate it
>> >> there, in a race free place. But the copying is apparently a problem.
>> >
>> > We already copy all data first, that's not an issue.  Validate it after
>> > it has been copied before you do something with it, just like we do
>> > today for normal ioctl C code.  Same goes for data coming from hardware,
>> > it's already been copied into a buffer that you can use, no need to copy
>> > it again, just "validate" it before using it.
>>
>> I guess that depends on the user of the `iov_iter`? At least when it is
>> used for direct IO, the operation is zero copy with pages mapped into
>> kernel and IO performed directly from those pages.
>
> Great, and then, before you actually do something with that data, you
> have to validate it that it is correct, right?  If this is just a
> "pass-through" then no need to do anything to it.  But if you have to
> inspect/act-on-it, then just inspect it in the verifier portion.
>
> I'm not trying to add any additional overhead here, as you normally
> would have to verify the data is correct before doing something with it,
> but rather just forcing that validation to be done in an obvious place
> where everyone can see that it is being done, or not being done.
>
> In other words, make it obvious to reviewers that this is happening,
> instead of forcing them to dig through code paths to hopefully find out
> where it happens.

I would agree.

Just to be clear, I am not objecting to validating data before
interpreting, that is the only sane thing to do. I'm just raising a
concern in relation to reading pages mapped from user space in Rust.
Because apparently it is undefined behavior, as discussed in the thread
I linked.


Best regards,
Andreas Hindborg