On Mon Dec 15, 2025 at 8:43 AM JST, Alistair Popple 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.
That's correct, although it defers the possibility of underflow to
`length`, albeit using two constants. Still, doing this as a const would
catch any potential issue at build-time:
pub(crate) fn length(&self) -> usize {
// `rpc.length` includes the length of the GspRpcHeader but not the
message header.
const RPC_LENGTH: usize = size_of::<Self>() -
size_of::<bindings::rpc_message_header_v>();
RPC_LENGTH + num::u32_as_usize(self.inner.rpc.length)
}
This length computation has been the subject of debate between me and
Alistair back when we were writing the code, so this part can be subject
to change if doing so limits the amount of potentially panicking
operations.
>
>> 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 ;-)
>
> 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.
How would that be beneficial? We would need to use `unsafe` to access
the data, but then unless we can encode the guarantees that we verified
in the type itself (or its invariants, but that would quickly become
cumbersome to manage) we would still have the same ops ongoing.
IMHO the simple and safe way is to use checked operations with data that
comes from the GSP.
