Re: [PATCH v5 5/6] gpu: nova-core: set RMSetSriovMode for vGPU

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The `size` field in the `PACKED_REGISTRY_TABLE` header is initialized incorrectly, causing the string data offsets to exceed the advertised table size.
--

--- Patch [5]: [PATCH v5 5/6] gpu: nova-core: set RMSetSriovMode for vGPU ---
commit 7a8618e853f5431c98f463e1cf9330f35e524af1
Author: Zhi Wang <[email protected]>

gpu: nova-core: set RMSetSriovMode for vGPU
    
The GSP registry setup needs to advertise SR-IOV mode when nova-core boots
GSP for an enabled vGPU configuration. Without the registry entry, GSP-RM
is not told to initialize in the mode required by NVIDIA vGPU.

> diff --git a/drivers/gpu/nova-core/gsp/commands.rs b/drivers/gpu/nova-core/gsp/commands.rs
> index 08380de39048a..134f34b191744 100644
> --- a/drivers/gpu/nova-core/gsp/commands.rs
> +++ b/drivers/gpu/nova-core/gsp/commands.rs
[ ... ]
> @@ -104,6 +105,17 @@ pub(crate) fn new() -> Result<Self> {
>              GFP_KERNEL,
>          )?;
>  
> +        if matches!(vgpu_state, VgpuState::Enabled { .. }) {
> +            // RMSetSriovMode - required when vGPU is enabled.
> +            entries.push(
> +                RegistryEntry {
> +                    key: "RMSetSriovMode",
> +                    value: 1,
> +                },
> +                GFP_KERNEL,
> +            )?;
> +        }
> +
>          Ok(Self { entries })
>      }
>  }

[Severity: High]
This isn't a bug introduced by this patch, but the size field in the
PACKED_REGISTRY_TABLE appears to be initialized incorrectly, which might cause
the newly added RMSetSriovMode entry to silently fail or cause out-of-bounds
reads.

Looking at SetRegistry::init() in this same file:

drivers/gpu/nova-core/gsp/commands.rs:SetRegistry::init() {
    fn init(&self) -> impl Init<Self::Command, Self::InitError> {
        Self::Command::init(
            self.entries.len() as u32,
            self.variable_payload_len() as u32,
        )
    }
}

Does this initialize the size using only the payload length, omitting the
8-byte table header itself? 

Since the string offsets calculated in init_variable_payload() correctly
include the 8-byte header size, won't the end of the string data exceed the
advertised table size by exactly 8 bytes?

Could this cause the GSP firmware to truncate the registry entries or perform
an out-of-bounds read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5