Re: [PATCH v2 4/5] drm/nouveau/gsp/r570: Start saving comptag backing stores

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] nouveau: gsp: buffer overflow in r535_fbsr_memlist() due to unaligned compbit_store_size
- [Medium] nouveau: gsp: integer truncation of size leads to tiny DMA allocation
--

commit 2f071ac78fdb305696ed659788ff3cf83d6adc73
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/gsp/r570: Start saving comptag backing stores
    
One of the portions of OpenRM's fbsr process that we never implemented is
the saving and restoring of comptag backing stores. This isn't strictly
necessary for fbsr to work (as long as we don't specify bEnteringGcOff =
1), but implementing it brings us much closer to matching OpenRM's fbsr
process - which means we can rely on things being well tested on Nvidia's
side.

> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> index 569732320f3dd..feef5104a6241 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/gsp/rm/r570/fbsr.c

[ ... ]

> @@ -174,6 +197,8 @@ r570_fbsr_suspend(struct nvkm_gsp *gsp)
>  	size  = gsp->fb.heap.size;
>  	size += gsp->fb.rsvd_size;
>  	size += gsp->fb.bios.vga_workspace.size;
> +	size += compbit_store_size;
> +

[Severity: High]
Does adding the unaligned compbit_store_size directly to size here lead to an
out-of-bounds write in r535_fbsr_memlist()?

When this unaligned size is passed to nvkm_gsp_sg(), it rounds it up using
DIV_ROUND_UP to allocate memory pages. However, when r535_fbsr_memlist()
constructs the memory list, it computes the required RPC array size by
truncation:

    const u32 pages = size / GSP_PAGE_SIZE;

This allocates an undersized array for the PTEs. Then the loop inside
r535_fbsr_memlist() iterates based on the rounded-up scatterlist length:

    for (int i = 0; i < sg_dma_len(sgl) / GSP_PAGE_SIZE; i++)

This mismatch causes the pte++ index to write 8 bytes (or more) past the end
of the allocated rpc buffer, resulting in kernel heap memory corruption if a
buggy or compromised firmware returns a non-page-aligned size.


[Severity: Medium]
Could an extremely large compbit_store_size from the GSP firmware cause an
integer truncation when passed to sg_alloc_table()?

If size exceeds the 32-bit limit, DIV_ROUND_UP(size, PAGE_SIZE) in
nvkm_gsp_sg() overflows the 32-bit unsigned int nents parameter in
sg_alloc_table(). This truncates the value, resulting in a tiny DMA
allocation.

Meanwhile, this huge size is still passed to the GSP as the length field.
When the GSP attempts to save its state, it will DMA out of the bounds of the
mapped physical pages, potentially leading to a GSP MMU or IOMMU fault.

>  	nvkm_debug(subdev, "fbsr: size: 0x%llx bytes\n", size);
>  
>  	ret = nvkm_gsp_sg(device, size, &gsp->sr.fbsr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260915210647.695187-1-lyude@redhat.com?part=4