Re: [PATCH v4 02/14] drm/panic: Test address from kmap op for NULL

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Potential NULL pointer dereference in `kmap_local_page_try_from_panic()` due to missing NULL checks for page pointers in `drm_panic_fill_page()` and `drm_panic_blit_page()`.
--

commit ddaf01e3dc4138e21af0a9818e11a3a8b5244eae
Author: Thomas Zimmermann <tzimmermann@suse.de>

drm/panic: Test address from kmap op for NULL

This patch adds a NULL check for the return value of
kmap_local_page_try_from_panic() to avoid dereferencing a failed mapping
during kernel panic screen rendering.

> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> index ba21df4be3380..d28ce8878e055 100644
> --- a/drivers/gpu/drm/drm_panic.c
> +++ b/drivers/gpu/drm/drm_panic.c
> @@ -178,6 +178,8 @@ static void drm_panic_write_pixel24_xpage(void *vaddr, struct page *next_page,
>  	u8 *p = vaddr + offset;
>  
>  	vaddr2 = kmap_local_page_try_from_panic(next_page);

[Severity: High]
This isn't a bug introduced by this patch, but could next_page itself 
be NULL here?

If a 24-bit pixel crosses a page boundary and the next page in the pages 
array is NULL, passing it into kmap_local_page_try_from_panic() can cause 
a NULL pointer dereference via PageHighMem(). If CONFIG_HIGHMEM is disabled, 
page_to_virt(NULL) yields a garbage pointer which also crashes upon 
dereference. This would cause a double fault during a kernel panic, 
preventing the panic screen from rendering.

It looks like similar unchecked page pointers are passed elsewhere in 
drivers/gpu/drm/drm_panic.c, such as in drm_panic_fill_page():

drm_panic_fill_page() {
    ...
    vaddr = kmap_local_page_try_from_panic(pages[page]);
    ...
}

And in drm_panic_blit_page(), pages[page + 1] is passed without a NULL check 
even though pages[new_page] is explicitly checked earlier in the function:

drm_panic_blit_page() {
    ...
    drm_panic_write_pixel24_xpage(vaddr, pages[page + 1], offset, fg32);
    ...
}

Would it be safer to verify that next_page and pages[page] are not NULL 
before attempting to map them, in order to safely handle unmapped or sparse 
pages in the display buffer?

> +	if (!vaddr2)
> +		return;
>  
>  	*p++ = color & 0xff;
>  	color >>= 8;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909085826.64529-1-tzimmermann@suse.de?part=2