Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] A TOCTOU double-fetch vulnerability in GSP message parsing allows compromised or malicious GSP firmware to corrupt the CPU's message queue read pointer, causing desynchronization and potential denial of service. -- commit eaaa0723c196a2c00643733ca548b4c1354725ff Author: Alexandre Courbot <[email protected]> gpu: nova-core: use kernel lossless integer conversion module The kernel crate now features a replacement for our lossless integer conversion routines. Switch to the kernel version and remove our own. > diff --git a/drivers/gpu/nova-core/gsp/cmdq.rs > b/drivers/gpu/nova-core/gsp/cmdq.rs > index 6da728201281b..f85fde09aa6e3 100644 > --- a/drivers/gpu/nova-core/gsp/cmdq.rs > +++ b/drivers/gpu/nova-core/gsp/cmdq.rs [ ... ] > @@ -162,7 +162,7 @@ fn read( > #[repr(C, align(0x1000))] > #[derive(Debug)] > struct MsgqData { > - data: [[u8; GSP_PAGE_SIZE]; num::u32_as_usize(MSGQ_NUM_PAGES)], > + data: [[u8; GSP_PAGE_SIZE]; casts::u32_as_usize(MSGQ_NUM_PAGES)], > } [Severity: High] This isn't a bug introduced by this patch, but while reviewing cmdq.rs, I noticed a pre-existing double-fetch issue that could allow a compromised GSP firmware to desynchronize the message queue. Could the GSP modify the message length in DMA memory after it has been validated, causing the host to corrupt its CPU read pointer? In CmdqInner::wait_for_msg(), the message length is fetched from DMA memory and validated against the available buffer size to ensure it is safe: CmdqInner::wait_for_msg() { ... let payload_length = header.payload_length(); // Check that the driver read area is large enough for the message. if slice_1.len() + slice_2.len() < payload_length { return Err(EIO); } ... } However, upon returning to CmdqInner::receive_msg(), the length is fetched a second time directly from the shared memory when advancing the queue read pointer: CmdqInner::receive_msg() { ... let message = self.wait_for_msg(timeout)?; ... // Advance the read pointer past this message. self.gsp_mem.advance_cpu_read_ptr(u32::try_from( message.header.length().div_ceil(GSP_PAGE_SIZE), )?); ... } Because message.header is a reference into DMA memory, this second fetch uses an unvalidated value. If the GSP modifies the length field in DMA memory between these two reads, the kernel will advance its read pointer by an unvalidated amount. This could cause the kernel to misinterpret queue memory, fail subsequent validations, and lose communication with the GSP. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
