Re: [PATCH 01/70] drm/adp: Switch to drm_atomic_helper_crtc_create_state

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

Pre-existing issues:
- [High] Missing NULL check for `dma_alloc_coherent` and potential integer overflow in `new_size` calculation.
- [High] Calling `dma_alloc_coherent()` with `GFP_KERNEL` during `atomic_flush` violates DRM atomic context constraints.
--

--- Patch [1]: [PATCH 01/70] drm/adp: Switch to drm_atomic_helper_crtc_create_state ---
commit 3f2217690a441eaa2502ebbe2a7e5840385ac1e3
Author: Maxime Ripard <[email protected]>

drm/adp: Switch to drm_atomic_helper_crtc_create_state

The adp crtc implementation uses the deprecated
drm_atomic_helper_crtc_reset() as its reset hook.

Switch to drm_atomic_helper_crtc_create_state() instead.

> diff --git a/drivers/gpu/drm/adp/adp_drv.c b/drivers/gpu/drm/adp/adp_drv.c
> index ce9dd6222634d..fdf0e63161756 100644
> --- a/drivers/gpu/drm/adp/adp_drv.c
> +++ b/drivers/gpu/drm/adp/adp_drv.c
> @@ -351,7 +351,7 @@ static const struct drm_crtc_funcs adp_crtc_funcs = {
>  	.destroy = drm_crtc_cleanup,
>  	.set_config = drm_atomic_helper_set_config,
>  	.page_flip = drm_atomic_helper_page_flip,
> -	.reset = drm_atomic_helper_crtc_reset,
> +	.atomic_create_state = drm_atomic_helper_crtc_create_state,
>  	.atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state,
>  	.atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,
>  	.enable_vblank = adp_crtc_enable_vblank,

[Severity: High]
This is a pre-existing issue, but does calling dma_alloc_coherent() with
GFP_KERNEL in adp_crtc_atomic_flush() violate DRM atomic context constraints?

drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() {
...
		if (new_size != 0) {
			adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size,
							   &adp->mask_iova, GFP_KERNEL);

adp_crtc_atomic_flush() executes during the hardware programming phase of
drm_atomic_helper_commit_tail(), which is an atomic context where sleeping
is forbidden.

dma_alloc_coherent() with GFP_KERNEL can sleep, potentially causing kernel
warnings and deadlocks.

[Severity: High]
This isn't a bug introduced by this patch, but what happens if
dma_alloc_coherent() returns NULL?

drivers/gpu/drm/adp/adp_drv.c:adp_crtc_atomic_flush() {
...
		if (new_size != 0) {
			adp->mask_buf = dma_alloc_coherent(crtc->dev->dev, new_size,
							   &adp->mask_iova, GFP_KERNEL);
			memset(adp->mask_buf, 0xFF, new_size);

The immediate memset() would dereference the NULL pointer.

Additionally, does the calculation of new_size in adp_crtc_atomic_flush() run
the risk of a 32-bit integer overflow for maliciously large resolutions?

	u64 new_size = ALIGN(new_state->mode.hdisplay *
			     new_state->mode.vdisplay * 4, PAGE_SIZE);

Is there a chance hdisplay * vdisplay * 4 could overflow before being cast
to u64?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1