Re: [PATCH 0/3] Untrusted data abstraction

[Greg KH <gregkh@linuxfoundation.org>]

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.

As you say, this is NOT a "silver bullet", but it IS something that will
help squash so many kernel bugs it's not funny.  I've been wanting
something like this for C code for a while (much like we have __user
markings), and it will help call out exactly where we are doing "is this
data valid or not" calls, and where we are NOT doing those calls, which
is also important to know (as many data streams are not touched by the
kernel.)

It will also help with the detection of where user and hardware data can
influence code logic, much like the spectre work has highlighted needs
to be tracked in order to help catch "gadgets" and other places where
it is not intended for userspace to be able to control cpu registers in
malicious ways, but it happens too often.

Thanks so much for doing this work, and I can't wait to tie this into
many driver subsystems going forward.  I'm sure the "vfio" people will
want this, given their preoccupation with the marketing idea of
"hardening" a vfio driver :)

> 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!

I like it!  I'll see about tieing it into the miscdev patches and any
future code that has ioctl handling once they show up on the list for
inclusion.

thanks again!

greg k-h