Hi Alistair, > On Dec 15, 2025, at 8:43 AM, Alistair Popple <[email protected]> wrote: > > On 2025-12-12 at 19:10 +1100, Dirk Behme <[email protected]> wrote... >>> On 12.12.25 08:59, Joel Fernandes wrote: >>> Hi Alex, >>> >>>> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <[email protected]> >>>> wrote: >>>> >>>> The size of messages' payload is miscalculated, leading to extra data >>>> passed to the message handler. While this is not a problem with our >>>> current set of commands, others with a variable-length payload may >>>> misbehave. Fix this. >>>> >>>> Signed-off-by: Alexandre Courbot <[email protected]> >>>> --- >>>> drivers/gpu/nova-core/gsp/cmdq.rs | 11 +++++++---- >>>> drivers/gpu/nova-core/gsp/fw.rs | 2 +- >>>> 2 files changed, 8 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs >>>> b/drivers/gpu/nova-core/gsp/cmdq.rs >>>> index 6f946d14868a..dab73377c526 100644 >>>> --- a/drivers/gpu/nova-core/gsp/cmdq.rs >>>> +++ b/drivers/gpu/nova-core/gsp/cmdq.rs >>>> @@ -588,21 +588,24 @@ fn wait_for_msg(&self, timeout: Delta) -> >>>> Result<GspMessage<'_>> { >>>> header.length(), >>>> ); >>>> >>>> + // The length of the message that follows the header. >>>> + let msg_length = header.length() - size_of::<GspMsgElement>(); >>> >>> Is this immune to under flow without one of the checked subtraction >>> wrappers? Either way, we should not tolerate the underflow I think. Which >>> means it can panic when the rust overflow checks are enabled. Since the >>> header length comes from firmware, this cannot be guaranteed to not >>> underflow in the event of a malformed message. > > I think we're guaranteed not to underflow here - check out the implementation > for header.length(): > > /// Returns the total length of the message. > pub(crate) fn length(&self) -> usize { > // `rpc.length` includes the length of the GspRpcHeader but not the > message header. > size_of::<Self>() - size_of::<bindings::rpc_message_header_v>() > + num::u32_as_usize(self.inner.rpc.length) > } > > So the above calculation expands to: > > msg_length = size_of::<Self>() - size_of::<bindings::rpc_message_header_v>() > + num::u32_as_usize(self.inner.rpc.length) - > size_of::<GspMsgElement>(); > > Where self.inner.rpc.length is guaranteed to be >= > size_of::<rpc_message_header_v>() by the construction of the type.
But this length field is coming from the firmware, correct? The guarantee is provided by firmware, not by Rust code calculating the length. Maybe Rust validating that the length matches, or is greater than or equal to, the message header would be one way to avoid doing the checked subtraction. I would still be comfortable doing the checked subtraction in case the firmware payload in the message buffer is corrupted and the length field is corrupted. I think Rust cannot trust fields coming from the firmware and needs to check them to prevent undefined behavior. Or maybe the policy is to include safety comments, like we do when expecting C code to behave in a certain way. I do not know. But we should identify the policy for this and stick to it for future such issues. I think one way to verify that there is a Rust guarantee about the length field is to do the unchecked subtraction, compile the code, and then see if the generated code has any panics in it (With the overflow checking config enabled.) If it does, then dead code elimination could not determine at compile time that the subtraction would not overflow. Right? > >> Would this be a possible use case for the untrusted data proposal >> >> https://lwn.net/Articles/1034603/ >> >> ? > > Responding here because Joel appears to have sent a HTML only response ;-) Sorry. :) > > I agree with Joel's points - this does sound useful but as a separate project. > I'd imagine we'd want to it one layer lower though - ie. in the construction > of > the GspMsgElement. Agreed, thanks. - Joel > >> Cheers >> >> Dirk
