Joel Fernandes
2025-Dec-12 07:59 UTC
[PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages
Hi Alex,> On Nov 22, 2025, at 12:00?AM, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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. thanks, - Joel> + > // Check that the driver read area is large enough for the message. > - if slice_1.len() + slice_2.len() < header.length() { > + if slice_1.len() + slice_2.len() < msg_length { > return Err(EIO); > } > > // Cut the message slices down to the actual length of the message. > - let (slice_1, slice_2) = if slice_1.len() > header.length() { > + let (slice_1, slice_2) = if slice_1.len() > msg_length { > // PANIC: we checked above that `slice_1` is at least as long as `msg_header.length()`. > - (slice_1.split_at(header.length()).0, &slice_2[0..0]) > + (slice_1.split_at(msg_length).0, &slice_2[0..0]) > } else { > ( > slice_1, > // PANIC: we checked above that `slice_1.len() + slice_2.len()` is at least as > // large as `msg_header.length()`. > - slice_2.split_at(header.length() - slice_1.len()).0, > + slice_2.split_at(msg_length - slice_1.len()).0, > ) > }; > > diff --git a/drivers/gpu/nova-core/gsp/fw.rs b/drivers/gpu/nova-core/gsp/fw.rs > index abffd6beec65..7fcba5afb0a3 100644 > --- a/drivers/gpu/nova-core/gsp/fw.rs > +++ b/drivers/gpu/nova-core/gsp/fw.rs > @@ -853,7 +853,7 @@ pub(crate) fn set_checksum(&mut self, checksum: u32) { > self.inner.checkSum = checksum; > } > > - /// Returns the total length of the message. > + /// Returns the total length of the message, message and RPC headers included. > 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>() > > -- > 2.51.2 >
Dirk Behme
2025-Dec-12 08:10 UTC
[PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages
On 12.12.25 08:59, Joel Fernandes wrote:> Hi Alex, > >> On Nov 22, 2025, at 12:00?AM, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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.Would this be a possible use case for the untrusted data proposal https://lwn.net/Articles/1034603/ ? Cheers Dirk
Joel Fernandes
2025-Dec-12 09:23 UTC
[PATCH 2/4] gpu: nova-core: gsp: Fix length of received messages
On Dec 12, 2025, at 5:10?PM, Dirk Behme <dirk.behme at gmail.com> wrote:
.12.25 08:59, Joel Fernandes wrote:
Hi Alex,
On Nov 22, 2025, at 12:00?AM, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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.
Show Quoted Content
On 12.12.25 08:59, Joel Fernandes wrote:
Hi Alex,
On Nov 22, 2025, at 12:00?AM, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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.
On 12.12.25 08:59, Joel Fernandes wrote:
Hi Alex,
On Nov 22, 2025, at 12:00?AM, Alexandre Courbot <acourbot at nvidia.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 at nvidia.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.
Would this be a possible use case for the untrusted data proposal
https://lwn.net/Articles/1034603/
Yeah, I think so. Thanks. What do you think Alex?
My opinion is that would be a larger/separate effort than what Alex is doing
with this patch. But it sounds promising (I.e forcing validation of untrusted
sources).
thanks,
- Joel
?
Cheers
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20251212/ebf9c31b/attachment-0001.htm>