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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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.

thanks,

greg k-h