rust/kernel/alloc/kvec.rs | 27 ++++ rust/kernel/iov.rs | 308 +++++++++++++++++++++++++++++++++++++++ rust/kernel/lib.rs | 1 + rust/kernel/miscdevice.rs | 97 +++++++++++- samples/rust/rust_misc_device.rs | 37 ++++- 5 files changed, 467 insertions(+), 3 deletions(-)
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.
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Alice Ryhl (4):
rust: iov: add iov_iter abstractions for ITER_SOURCE
rust: iov: add iov_iter abstractions for ITER_DEST
rust: miscdevice: Provide additional abstractions for iov_iter and kiocb structures
rust: alloc: add Vec::clear
Lee Jones (1):
samples: rust_misc_device: Expand the sample to support read()ing from userspace
rust/kernel/alloc/kvec.rs | 27 ++++
rust/kernel/iov.rs | 308 +++++++++++++++++++++++++++++++++++++++
rust/kernel/lib.rs | 1 +
rust/kernel/miscdevice.rs | 97 +++++++++++-
samples/rust/rust_misc_device.rs | 37 ++++-
5 files changed, 467 insertions(+), 3 deletions(-)
---
base-commit: 046cc01be6b9d139b49dfc396b7201c633ff1a26
change-id: 20250311-iov-iter-c984aea07d18
Best regards,
--
Alice Ryhl <aliceryhl@google.com>
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. thanks, greg k-h
"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. Best regards, Andreas Hindborg [1] https://lore.kernel.org/all/ab8fd525-9a63-46e2-a443-b9d94eed6004@ralfj.de/
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. thanks, greg k-h
"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. Best regards, Andreas Hindborg
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
"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
On Tue Mar 11, 2025 at 3:37 PM CET, Greg Kroah-Hartman wrote: > 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 have started to work on that again, just needed to get through several things in my backlog... Are there any drivers or abstractions in mainline that I can use for creating the interface? Or are those still out of tree? I don't think that I can use tarfs for that as I did back when I started with this patch set, as it will probably be hopelessly out of date. --- Cheers, Benno > 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. > > thanks, > > greg k-h
On Wed, Mar 12, 2025 at 02:16:43AM +0000, Benno Lossin wrote: > On Tue Mar 11, 2025 at 3:37 PM CET, Greg Kroah-Hartman wrote: > > 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 have started to work on that again, just needed to get through several > things in my backlog... > > Are there any drivers or abstractions in mainline that I can use for > creating the interface? Or are those still out of tree? I don't think > that I can use tarfs for that as I did back when I started with this > patch set, as it will probably be hopelessly out of date. You can use the misc device api as a start, bindings for it are in the tree, and this series here has an example that would need it directly. thanks, greg k-h
© 2016 - 2025 Red Hat, Inc.