On Dec 12, 2025, at 5:10 PM, Dirk Behme <[email protected]> wrote:
.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. Show Quoted Content 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. 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. 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
