linux-kernel - Re: [PATCH v2] drm: vkms: check status of alloc_ordered

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: <20190325083119.GB2665@phenom.ffwll.local>
Date:   Mon, 25 Mar 2019 09:31:19 +0100
From:   Daniel Vetter <daniel@...ll.ch>
To:     Kangjie Lu <kjlu@....edu>
Cc:     pakki001@....edu, Rodrigo Siqueira <rodrigosiqueiramelo@...il.com>,
        Haneen Mohammed <hamohammed.sa@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] drm: vkms: check status of alloc_ordered_workqueue

On Sat, Mar 23, 2019 at 04:42:16PM -0500, Kangjie Lu wrote:
> alloc_ordered_workqueue may fail and return NULL.
> The fix cleans up drm plans and returns ENOMEM when it fails to
> avoid potential NULL pointer dereference.
> 
> Signed-off-by: Kangjie Lu <kjlu@....edu>
> ---
> V2: clean up resources
> ---
>  drivers/gpu/drm/vkms/vkms_crtc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
> index 8a9aeb0a9ea8..018b52dd953a 100644
> --- a/drivers/gpu/drm/vkms/vkms_crtc.c
> +++ b/drivers/gpu/drm/vkms/vkms_crtc.c
> @@ -219,6 +219,17 @@ int vkms_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  	spin_lock_init(&vkms_out->state_lock);
>  
>  	vkms_out->crc_workq = alloc_ordered_workqueue("vkms_crc_workq", 0);
> +	if (!vkms_out->crc_workq) {
> +		ret = -ENOMEM;
> +		goto cleanup;
> +	}
> +
> +	return ret;
>  
> +cleanup:

Style nit for the future, for clarity I'd label this err:, since this path
is only taken for failures, and not to do cleanup for all cases.

Aside from that, I think your v1 was correct, vkms_crtc_init only sets up
the crtc, it doesn't allocate the cursor/planes. That's done from
vkms_output_init, which already has the cleanup code to handle this case.
-Daniel

> +	if (!IS_ERR_OR_NULL(cursor))
> +		drm_plane_cleanup(cursor);
> +	if (!IS_ERR(primary))
> +		drm_plane_cleanup(primary);
>  	return ret;
>  }
> -- 
> 2.17.1
> 

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