linux-kernel - Re: [PATCH] drm/crtc: Fix uninit-value bug in drm_mode

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <87h6kszvx7.fsf@intel.com>
Date:   Fri, 08 Dec 2023 15:05:40 +0200
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     Ziqi Zhao <astrajoan@...oo.com>, astrajoan@...oo.com,
        airlied@...il.com, daniel@...ll.ch,
        dri-devel@...ts.freedesktop.org, ivan.orlov0322@...il.com,
        maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        skhan@...uxfoundation.org, tzimmermann@...e.de
Cc:     syzkaller-bugs@...glegroups.com, linux-kernel@...r.kernel.org,
        christian.koenig@....com, linaro-mm-sig@...ts.linaro.org,
        glider@...gle.com,
        syzbot+4fad2e57beb6397ab2fc@...kaller.appspotmail.com,
        sumit.semwal@...aro.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] drm/crtc: Fix uninit-value bug in drm_mode_setcrtc

On Fri, 21 Jul 2023, Ziqi Zhao <astrajoan@...oo.com> wrote:
> 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.

>From one uninit-value bug to another?

>
> Reported-by: syzbot+4fad2e57beb6397ab2fc@...kaller.appspotmail.com
> Signed-off-by: Ziqi Zhao <astrajoan@...oo.com>
> ---
>  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 df9bf3c9206e..d718c17ab1e9 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -715,8 +715,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;

num_connectors is uninitialized.

>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EOPNOTSUPP;
> @@ -851,6 +850,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  			goto out;
>  		}
>  
> +		num_connectors = 0;

num_connectors is initialized only if crtc_req->count_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;
> @@ -871,6 +871,7 @@ int drm_mode_setcrtc(struct drm_device *dev, void *data,
>  					connector->name);
>  
>  			connector_set[i] = connector;
> +			num_connectors++;
>  		}
>  	}
>  
> @@ -879,7 +880,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;

num_connectors is used uninitialized if crtc_req->count_connectors <= 0.

BR,
Jani.

>  	set.fb = fb;
>  
>  	if (drm_drv_uses_atomic_modeset(dev))
> @@ -892,7 +893,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