Re: [PATCH 0/3] Untrusted data abstraction

[Benno Lossin <benno.lossin@proton.me>]

On 13.09.24 22:43, Simona Vetter wrote:
> 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

Will do with the next version.

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

Well you technically can still be very stupid and just call `validate()`
twice (I hope that this doesn't happen in practice, but you never
know...). But it's another big improvement.

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

We would probably need some changes to bindgen. But I also think that
it's a good idea.

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

So padding bytes are not allowed for types implementing `AsBytes`. For
`FromBytes` they are.

---
Cheers,
Benno