linux-kernel - Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <DEYH6HB5A8GV.EYH4ZAMXIFUC@nvidia.com>
Date: Mon, 15 Dec 2025 12:37:53 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Alistair Popple" <apopple@...dia.com>, "Dirk Behme"
 <dirk.behme@...il.com>
Cc: "Joel Fernandes" <joelagnelf@...dia.com>, "Alexandre Courbot"
 <acourbot@...dia.com>, "Danilo Krummrich" <dakr@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Simona Vetter" <simona@...ll.ch>, "Miguel Ojeda"
 <ojeda@...nel.org>, "Alex Gaynor" <alex.gaynor@...il.com>, "Boqun Feng"
 <boqun.feng@...il.com>, "Gary Guo" <gary@...yguo.net>,
 Björn Roy Baron <bjorn3_gh@...tonmail.com>, "Benno Lossin"
 <lossin@...nel.org>, "Andreas Hindborg" <a.hindborg@...nel.org>, "Trevor
 Gross" <tmgross@...ch.edu>, "nouveau@...ts.freedesktop.org"
 <nouveau@...ts.freedesktop.org>, "dri-devel@...ts.freedesktop.org"
 <dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
 <linux-kernel@...r.kernel.org>, "rust-for-linux@...r.kernel.org"
 <rust-for-linux@...r.kernel.org>
Subject: Re: [PATCH 2/4] gpu: nova-core: gsp: Fix length of received
 messages

On Mon Dec 15, 2025 at 8:43 AM JST, Alistair Popple wrote:
> On 2025-12-12 at 19:10 +1100, Dirk Behme <dirk.behme@...il.com> wrote...
>> On 12.12.25 08:59, Joel Fernandes wrote:
>> > Hi Alex,
>> > 
>> >> On Nov 22, 2025, at 12:00 AM, Alexandre Courbot <acourbot@...dia.com> 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 <acourbot@...dia.com>
>> >> ---
>> >> 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.