Re: [PATCH v4 0/4] Untrusted Data API

[Alistair <alistair23@gmail.com>]

On 17/5/26 05:58, Gary Guo wrote:
> (Resend to reply-all, oops!)
> 
> On Sat May 16, 2026 at 2:21 PM BST, Greg KH wrote:> On Thu, Aug 14, 2025 at 02:44:12PM +0200, Benno Lossin wrote:
>>> I didn't have too much time to spend on this API, so this is mostly a
>>> resend of v3. There are some changes in the last commit, updating to the
>>> latest version of Alice's iov_iter patche series [1] & rebasing on top
>>> of v6.17-rc1.
>>>
>>> I think we should just merge the first two patches this cycle in order
>>> to get the initial, bare-bones API into the kernel and have people
>>> experiment with it. The validation logic in the third patch still needs
>>> some work and I'd need to find some time to work on that (no idea when I
>>> find it though).
>>>
>>> I also think that field projections are necessary to make `Untrusted`
>>> reasonably useful, but I'm open to adding a stop gap solution in the
>>> meantime. There has been some movement at upstream rust on field
>>> projections. I submitted a project goal for 2025H2 [2] and it most
>>> likely will be accpeted. I also opened a tracking issue [3] for the
>>> language experiment that will drive the design of the feature.
>>
>> Ok, I finally carved out a bit of time for this, and moved the user
>> data pointer rust bindings over to use untrusted, which looks like this:
>>
>> [snip]
>>
>> Now, this obviously blows up the build as everywhere we are attempting
>> to read from userspace data, the buffers are marked "untrusted".
>> Ideally this would be simple to just go and make all readers implement a
>> Validate trait, BUT we have fun things like the debugfs bindings that
>> attempt to do automatic conversions of any type being read from userspace:
>>
>> impl<T: FromStr + Unpin> Reader for Mutex<T> {
>>      fn read_from_slice(&self, reader: &mut UserSliceReader) -> Result {
>>          let mut buf = [0u8; 128];
>>          if reader.len() > buf.len() {
>>              return Err(EINVAL);
>>          }
>>          let n = reader.len();
>>          reader.read_slice(&mut buf[..n])?;
>>
>>          let s = core::str::from_utf8(&buf[..n]).map_err(|_| EINVAL)?;
>>          let val = s.trim().parse::<T>().map_err(|_| EINVAL)?;
>>          *self.lock() = val;
>>          Ok(())
>>      }
>> }
>>
>> So, converting the data to the "correct" type is a fine idea, but then we
>> really want to make the data in that type as "Untrusted", right?  But how?
>> Force the caller to make the type definition as untrusted?  Something else?
> 
> Forcing the data to be "Untrusted" can be done like this:
> 
> impl<T: FromStr + Unpin> Reader for Mutex<Untrusted<T>> {
>      ...
> }
> 
> Although I suppose for debugfs you would want to validate immediately, so
> something like:
> 
> impl<T: FromStr + Validate<Self> + Unpin> Reader for Mutex<T> {}
> 
> Although this does force `T: Validate<Self>` which does not allow type-changing
> during validation.
> 
>>
>> I thought about a "blind" movement from untrusted->validated in the buffer
>> here, but that feels to circumvent the real idea that the data coming from
>> userspace is "untrusted" and must be checked before acted on.
>>
>> I have run into the wall of my rust knowledge here, am I missing something
>> simple?
>>
>> Also, the one user of this trait so far in the SPDM patchset:
>> 	https://lore.kernel.org/r/20260211032935.2705841-1-alistair.francis@wdc.com
>> is doing just "this is a C structure, so all is good" type of validation:

That's not always the case. For `GetVersionRsp` for example we can do 
some validation that the data returned matches what we expect.

As part of the `GetCapabilitiesRsp` we also have to ensure that the 
length of the data returned (which is dynamic based on the version 
support) is long enough to fit in the type. If it isn't we have to 
expand the underlying vector to avoid soundness issues. As that is still 
a spec compliant response, but would leave unallocated memory at the end 
of the struct.

One issue with doing more validation is that we don't have the full SPDM 
context in the `validate()` function. For example `validate()` doesn't 
know the version or algorithms that were previously negotiated, if it 
did we could compare against that.

I wanted to do more validation in the `validate()` functions, but don't 
have any good ideas of what else to check for.

One other idea, was that considering SPDM data is generally pretty small 
and not a performance bottleneck. The structs could be converted to 
non-packed structs and the data can be verified, copied out and the 
endianess updated. So the actual Rust struct output is just ready to go. 
But the overhead is a bit high

>>
>> impl Validate<&mut Unvalidated<KVec<u8>>> for &mut ChallengeRsp {
>>      type Err = Error;
>>
>>      fn validate(unvalidated: &mut Unvalidated<KVec<u8>>) -> Result<Self, Self::Err> {
>>          let raw = unvalidated.raw_mut();
>>          if raw.len() < mem::size_of::<ChallengeRsp>() {
>>              return Err(EINVAL);
>>          }
>>
>>          let ptr = raw.as_mut_ptr();
>>          // CAST: `ChallengeRsp` only contains integers and has `repr(C)`.
>>          let ptr = ptr.cast::<ChallengeRsp>();
>>          // SAFETY: `ptr` came from a reference and the cast above is valid.
>>          let rsp: &mut ChallengeRsp = unsafe { &mut *ptr };
>>
>>          // rsp.opaque_data_len = rsp.opaque_data_len.to_le();
>>
>>          Ok(rsp)
>>      }
>> }
> 
> Yeah, this use is confusing between two things: invariant of types and whether
> things are trusted.
> 
> Given a raw chunk of memory say `[u8]`, it may not be allowed to cast this chunk
> of memory to different type, say `T`, if `T` has some special assumptions on the
> data. For example, `T` may be `NonZero<u32>`, then it's invalid to convert a
> all-zero memory to it, this is known as validity invariant.
> 
> There's also safety invariant, so e.g. `[u8]` must not be turned to `str` if the
> representation is not valid UTF-8. Or it must not be turned into `CStr` if it
> contains interior NUL.

The SPDM case only converts `[u8]` to `u32`, `u16` or `u8`. So both of 
these should be satisfied.

> 
> We've already have a `FromBytes` and `IntoBytes` trait to capture both. Plain
> old structures can implement these traits and the type system catches you doing
> a cast (or transmutation, in Rust term) incorrectly.

So the SPDM should be updated to use the `FromBytes` trait?

 From a quick look at `FromBytes`, it does seem to be doing more or less 
the same thing as the SPDM implementation though.

> 
> But `Untrusted` is on top of that. It's a marker to indicate that this comes
> from user. Regardless if the marker exists, it must still uphold the type
> invariants. So in some sense, the code snippet is basically unconditionally
> discard the marker, because the only thing it checks is the validity invariants.

I do think we check for both, unless I'm missing something

Alistair

> 
> FWIW, I think our `User` API is not optimal even without considering the
> "Untrusted" markers, because it is completely untyped. Taking a IOCTL for
> example, you know what the type, so it should really be a `User<SpecificStruct>`
> and then you can read out `SpecificStruct` or `Untrusted<SpecificStruct>`. I
> have been thinking about doing that for a while but haven't had time to tackle
> it yet.
> 
> Best,
> Gary