Re: [PATCH 0/3] Untrusted data abstraction

[Simona Vetter <simona.vetter@ffwll.ch>]

On Fri, Sep 13, 2024 at 11:26:54AM +0000, Benno Lossin wrote:
> Enable marking certain data as untrusted. For example data coming from
> userspace, hardware or any other external data source.
> 
> This idea originates from a discussion with Greg at Kangrejos. As far as
> I understand the rationale, it is to prevent accidentally reading
> untrusted data and using it for *logic* within the kernel. For example
> reading the length from the hardware and not validating that it isn't
> too big. This is a big source for logic bugs that later turn into
> vulnerabilities.
> 
> The API introduced in this series is not a silver bullet, users are
> still able to access the untrusted value (otherwise how would they be
> able to validate it?). But it provides additional guardrails to remind
> users that they ought to validate the value before using it. As already
> stated, they can access the value directly, but to do that, they need to
> explicitly call one of the `untrusted_*` functions signaling to
> reviewers that they are reading untrusted data without validation.
> 
> There are still some things to iron out on the Rust side:
> - allow better handling of `Untrusted<T>`, for example allow comparing
>   `Untrusted<[u8]>` for equality (we should do this via a trait
>   extending `PartialEq`)
> - rebase this on Gary's patch to enable arbitrary self types. This would
>   allow me to remove one `untrusted_mut()` call in `inode.rs`. I would
>   expose the `cap_len` function even if the underlying data is
>   untrusted.
> - get more feedback as to what `Untrusted` should make available
> 
> In addition to adding the abstraction, I also provide very experimental
> RFC patches using the API in tarfs by wedson. They are based on his
> vfs-v2 branch [1] and I will not aim to upstream these patches. They should
> give you some idea as to how the API is intended to be used.
> 
> Open to any suggestions and improvements!
> 
> [1]: https://github.com/wedsonaf/linux/tree/vfs-v2

I forgot to add: If we do this, we should really wrap Untrusted<> around
all the copy_from_user wrappers we have in the uaccess module. That's one
of the main entry points for untrusted garbage into the kernel we have,
and it's already in upstream rust.

There's even a very nice TOCTOU example bug in the docs which would become
flat out impossible if we wrap everything in Untrusted.

Also, while I drop some feature requests: I think it'd be very nice to
have a macro or something to implement FromBytes and AsBytes automatically
and savely for C structs from the bindings which only contain linux uapi
safe types.  And also better contain absolutely no padding, since that
tends to be bugs on the C side too. But no idea whether rust can do that.
-Sima

> 
> Benno Lossin (3):
>   rust: add untrusted data abstraction
>   WIP: rust: fs: mark data returned by inodes untrusted
>   WIP: rust: tarfs: use untrusted data API
> 
>  fs/tarfs/defs.rs        |   1 +
>  fs/tarfs/tar.rs         | 143 +++++++++++++++++++++++++++-------------
>  rust/kernel/fs/inode.rs |  33 ++++++----
>  rust/kernel/types.rs    | 248 ++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 367 insertions(+), 58 deletions(-)
> 
> 
> base-commit: d077242d68a31075ef5f5da041bf8f6fc19aa231
> -- 
> 2.46.0
> 
> 

-- 
Simona Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch