Re: [PATCH AUTOSEL 4.14 3/6] drm/crtc: Fix uninit-value bug in drm_mode_setcrtc

[Jani Nikula]

On Mon, 18 Dec 2023, Sasha Levin <sashal@xxxxxxxxxx> wrote:
> From: Ziqi Zhao <astrajoan@xxxxxxxxx>
>
> [ Upstream commit 3823119b9c2b5f9e9b760336f75bc989b805cde6 ]
>
> The connector_set contains uninitialized values when allocated with
> kmalloc_array. However, in the "out" branch, the logic assumes that any
> element in connector_set would be equal to NULL if failed to
> initialize, which causes the bug reported by Syzbot. The fix is to use
> an extra variable to keep track of how many connectors are initialized
> indeed, and use that variable to decrease any refcounts in the "out"
> branch.
>
> Reported-by: syzbot+4fad2e57beb6397ab2fc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Ziqi Zhao <astrajoan@xxxxxxxxx>
> Reported-and-tested-by: syzbot+4fad2e57beb6397ab2fc@xxxxxxxxxxxxxxxxxxxxxxxxx
> Tested-by: Harshit Mogalapalli <harshit.m.mogalapalli@xxxxxxxxxx>
> Link: https://lore.kernel.org/r/20230721161446.8602-1-astrajoan@xxxxxxxxx
> Signed-off-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>

This commit fixes an uninitialized value, but introduces a new
one. Please backport 6e455f5dcdd1 ("drm/crtc: fix uninitialized variable
use") from v6.7-rc6 to go with it.

Thanks,
Jani.

> ---
>  drivers/gpu/drm/drm_crtc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 5af25ce5bf7c2..5ae3adfbc5e80 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -556,8 +556,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	struct drm_mode_set set;
>  	uint32_t __user *set_connectors_ptr;
>  	struct drm_modeset_acquire_ctx ctx;
> -	int ret;
> -	int i;
> +	int ret, i, num_connectors;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
> @@ -672,6 +671,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> +		num_connectors = 0;
>  		for (i = 0; i < crtc_req->count_connectors; i++) {
>  			connector_set[i] = NULL;
>  			set_connectors_ptr = (uint32_t __user *)(unsigned long)crtc_req->set_connectors_ptr;
> @@ -692,6 +692,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  					connector->name);
>  
>  			connector_set[i] = connector;
> +			num_connectors++;
>  		}
>  	}
>  
> @@ -700,7 +701,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  	set.y = crtc_req->y;
>  	set.mode = mode;
>  	set.connectors = connector_set;
> -	set.num_connectors = crtc_req->count_connectors;
> +	set.num_connectors = num_connectors;
>  	set.fb = fb;
>  	ret = __drm_mode_set_config_internal(&set, &ctx);
>  
> @@ -709,7 +710,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  		drm_framebuffer_put(fb);
>  
>  	if (connector_set) {
> -		for (i = 0; i < crtc_req->count_connectors; i++) {
> +		for (i = 0; i < num_connectors; i++) {
>  			if (connector_set[i])
>  				drm_connector_put(connector_set[i]);
>  		}

-- 
Jani Nikula, Intel