linux-kernel - Re: [PATCH RESEND] drm/lease: fix potential race in fill_object

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200317170243.GR2363188@phenom.ffwll.local>
Date:   Tue, 17 Mar 2020 18:02:43 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Qiujun Huang <hqjagain@...il.com>
Cc:     maarten.lankhorst@...ux.intel.com, mripard@...nel.org,
        airlied@...ux.ie, daniel@...ll.ch, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND] drm/lease: fix potential race in fill_object_idr

On Mon, Mar 16, 2020 at 03:18:23PM +0800, Qiujun Huang wrote:
> We should hold idr_mutex for idr_alloc.
> 
> Signed-off-by: Qiujun Huang <hqjagain@...il.com>

I've not seen the first version of this anywhere in my inbox, not sure
where that got lost.

Anyway, this seems like a false positive - I'm assuming this was caught
with KCSAN. The commit message really should mention that.

fill_object_idr creates the idr, which yes is only access later on under
the idr_mutex. But here it's not yet visible to any other thread, and
hence lockless access is safe and correct.

No idea what the KCSAN complains about safe access like this best practice
is.
-Daniel

> ---
>  drivers/gpu/drm/drm_lease.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_lease.c b/drivers/gpu/drm/drm_lease.c
> index b481caf..427ee21 100644
> --- a/drivers/gpu/drm/drm_lease.c
> +++ b/drivers/gpu/drm/drm_lease.c
> @@ -418,6 +418,7 @@ static int fill_object_idr(struct drm_device *dev,
>  		goto out_free_objects;
>  	}
>  
> +	mutex_lock(&dev->mode_config.idr_mutex);
>  	/* add their IDs to the lease request - taking into account
>  	   universal planes */
>  	for (o = 0; o < object_count; o++) {
> @@ -437,7 +438,7 @@ static int fill_object_idr(struct drm_device *dev,
>  		if (ret < 0) {
>  			DRM_DEBUG_LEASE("Object %d cannot be inserted into leases (%d)\n",
>  					object_id, ret);
> -			goto out_free_objects;
> +			goto out_unlock;
>  		}
>  		if (obj->type == DRM_MODE_OBJECT_CRTC && !universal_planes) {
>  			struct drm_crtc *crtc = obj_to_crtc(obj);
> @@ -445,20 +446,22 @@ static int fill_object_idr(struct drm_device *dev,
>  			if (ret < 0) {
>  				DRM_DEBUG_LEASE("Object primary plane %d cannot be inserted into leases (%d)\n",
>  						object_id, ret);
> -				goto out_free_objects;
> +				goto out_unlock;
>  			}
>  			if (crtc->cursor) {
>  				ret = idr_alloc(leases, &drm_lease_idr_object, crtc->cursor->base.id, crtc->cursor->base.id + 1, GFP_KERNEL);
>  				if (ret < 0) {
>  					DRM_DEBUG_LEASE("Object cursor plane %d cannot be inserted into leases (%d)\n",
>  							object_id, ret);
> -					goto out_free_objects;
> +					goto out_unlock;
>  				}
>  			}
>  		}
>  	}
>  
>  	ret = 0;
> +out_unlock:
> +	mutex_unlock(&dev->mode_config.idr_mutex);
>  out_free_objects:
>  	for (o = 0; o < object_count; o++) {
>  		if (objects[o])
> -- 
> 1.8.3.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch