Re: [PATCH v2] gpu: nova-core: gsp: fix undefined behavior in command queue code
From: Alexandre Courbot
Date: Tue Mar 24 2026 - 10:47:13 EST
On Tue Mar 24, 2026 at 1:44 AM JST, Gary Guo wrote:
> On Mon Mar 23, 2026 at 5:40 AM GMT, Alexandre Courbot wrote:
>> `driver_read_area` and `driver_write_area` are internal methods that
>> return slices containing the area of the command queue buffer that the
>> driver has exclusive read or write access, respectively.
>>
>> While their returned value is correct and safe to use, internally they
>> temporarily create a reference to the whole command-buffer slice,
>> including GSP-owned regions. These regions can change without notice,
>> and thus creating a slice to them is undefined behavior.
>>
>> Fix this by replacing the slice logic with pointer arithmetic and
>> creating slices to valid regions only. It adds unsafe code, but should
>> be mostly replaced by `IoView` and `IoSlice` once they land.
>>
>> Fixes: 75f6b1de8133 ("gpu: nova-core: gsp: Add GSP command queue bindings and handling")
>> Reported-by: Danilo Krummrich <dakr@xxxxxxxxxx>
>> Closes: https://lore.kernel.org/all/DH47AVPEKN06.3BERUSJIB4M1R@xxxxxxxxxx/
>> Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
>> ---
>> I didn't apply Eliot's Reviewed-by because the code has changed
>> drastically. The logic should remain identical though.
>> ---
>> Changes in v2:
>> - Use `u32_as_usize` consistently.
>> - Reduce the number of `unsafe` blocks by computing the end offset of
>> the returned slices and creating them at the end, in one step.
>> - Take advantage of the fact that both slices have the same start index
>> regardless of the branch chosen.
>> - Improve safety comments.
>> - Link to v1: https://patch.msgid.link/20260319-cmdq-ub-fix-v1-1-0f9f6e8f3ce3@xxxxxxxxxx
>
> Here's the diff that fixes the issue using I/O projection
> https://lore.kernel.org/rust-for-linux/20260323153807.1360705-1-gary@xxxxxxxxxx/
Should we apply or drop this patch meanwhile? I/O projections are still
undergoing review, but I'm fine with dropping it if Danilo thinks we can
live a bit longer with that UB. It's not like the driver is actively
doing anything useful yet anyway.