Re: [PATCH v2] gpu: nova-core: gsp: fix undefined behavior in command queue code

[Alexandre Courbot]

On Thu Mar 26, 2026 at 9:03 PM JST, Gary Guo wrote:
> On Thu Mar 26, 2026 at 4:51 AM GMT, Alexandre Courbot wrote:
>> On Thu Mar 26, 2026 at 1:30 PM JST, Alexandre Courbot wrote:
>>> On Wed Mar 25, 2026 at 12:15 AM JST, Gary Guo wrote:
>>>> On Tue Mar 24, 2026 at 2:44 PM GMT, Alexandre Courbot wrote:
>>>>> 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.
>>>>
>>>> I want to avoid big changes back and forth. We could use raw pointer projection
>>>> today, which could be fairly easy to convert to I/O projection:
>>>
>>> Thanks for the diff. I have adapted it to work on top of Danilo's
>>> suggestion to compute the end indices first as it works just as well and
>>> is cleaner. I have been running into a link error with this conversion
>>> applied though - let's discuss that on v3.
>>
>> Mmm, I guess this was because the optimizer could not prove that the
>> slices were within the bounds of the command queue as the expressions
>> passed to `ptr::project` were too complex with that version and this
>> makes the `ProjectIndex` check fail. I have better luck when doing
>> something closer to the diff you pasted.
>
> I'm considering switching the projectiong `[]` syntax to become panicking
> instead, given that the slicing use case quite often is indeed hard to prove
> (and also, we already have panicking comments).
>
> One option is to just change `[]` to do that, another option is adding a new
> `[]!` syntax to denote panicking projections. I'm more inclined to just the
> first one to keep consistency with Rust slicing syntax, but the second one is
> okay to me too.
>
> Thoughts?

If the slice's validity is hard to prove, then the caller should
probably rework their code towards something simpler (like we did with
this patch). Allowing a potentially invalid slice to build is just
inserting a kernel panic mine, and as you might have noticed from LPC I
am not a huge fan of those. :)

I think hammering the point about slice validity in the documentation
should be enough. We *want* build to fail if the slice can be invalid.