Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [Low] The commit modifies import statements without migrating them to the kernel's vertical import style. Pre-existing issues: - [High] FSP queue registers are defined as arrays of 8 elements without a `stride` parameter, defaulting to a 4-byte stride which silently overlaps with adjacent registers. - [High] Unbounded allocation in `recv_msg` based on untrusted hardware input can trigger an Out-of-Memory (OOM) condition or kernel panic. -- commit 2170d35504c32c252c52eca31b351aad8e873b56 Author: Antonin Malzieu Ridolfi <[email protected]> gpu: nova-core: falcon: Move PFSP register This patch moves the PFSP register definitions into the falcon module and updates the registers' visibility. > diff --git a/drivers/gpu/nova-core/falcon/fsp.rs > b/drivers/gpu/nova-core/falcon/fsp.rs > index 52cdb84ef0e80..547843e6483c6 100644 > --- a/drivers/gpu/nova-core/falcon/fsp.rs > +++ b/drivers/gpu/nova-core/falcon/fsp.rs > @@ -26,10 +26,10 @@ > Falcon, > FalconEngine, > PFalcon2Base, > - PFalconBase, // > + PFalconBase, > + regs, // > }, > - num, > - regs, // > + num, // > }; [Severity: Low] Does this import block need to be updated to match the kernel vertical import style? The Rust subsystem coding guidelines require flattening nested imports when they are modified. [ ... ] > diff --git a/drivers/gpu/nova-core/falcon/regs.rs > b/drivers/gpu/nova-core/falcon/regs.rs > index 4d4e69b235fc6..81530abfb54c3 100644 > --- a/drivers/gpu/nova-core/falcon/regs.rs > +++ b/drivers/gpu/nova-core/falcon/regs.rs [ ... ] > +register! { > + pub(super) NV_PFSP_QUEUE_HEAD(u32)[8] @ 0x008f2c00 { > + 31:0 address => u32; > + } > + > + pub(super) NV_PFSP_QUEUE_TAIL(u32)[8] @ 0x008f2c04 { > + 31:0 address => u32; > + } [Severity: High] This is a pre-existing issue, but does NV_PFSP_QUEUE_HEAD need a stride parameter? Since the elements are 4 bytes, omitting the stride defaults it to 4 bytes. This would place index 1 of NV_PFSP_QUEUE_HEAD at 0x008f2c04, which overlaps with NV_PFSP_QUEUE_TAIL. Should this be defined as [8, stride = 8] to prevent accessing the wrong register for indices > 0? [ ... ] [Severity: High] This isn't a bug introduced by this patch, but is there an unbounded allocation risk in recv_msg()? Looking at fsp.rs, poll_msgq() derives the message size directly from the hardware MMIO registers without bounds checking: tail.saturating_sub(head).saturating_add(4) This untrusted size is then used to allocate kernel memory: drivers/gpu/nova-core/falcon/fsp.rs:recv_msg() { ... let mut buffer = KVec::<u8>::new(); buffer.resize(msg_size, 0, GFP_KERNEL)?; ... } Could a compromised or misbehaving FSP firmware provide a massive message size, exhausting kernel memory and triggering an OOM condition? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
