linux-kernel - Re: [PATCH v18 22/26] drm/shmem-helper: Don't free refcounted GEM

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: <20231113105438.60896fdf@collabora.com>
Date:   Mon, 13 Nov 2023 10:54:38 +0100
From:   Boris Brezillon <boris.brezillon@...labora.com>
To:     Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc:     David Airlie <airlied@...il.com>,
        Gerd Hoffmann <kraxel@...hat.com>,
        Gurchetan Singh <gurchetansingh@...omium.org>,
        Chia-I Wu <olvaffe@...il.com>, Daniel Vetter <daniel@...ll.ch>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Christian König <christian.koenig@....com>,
        Qiang Yu <yuq825@...il.com>,
        Steven Price <steven.price@....com>,
        Emma Anholt <emma@...olt.net>, Melissa Wen <mwen@...lia.com>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        kernel@...labora.com, virtualization@...ts.linux-foundation.org
Subject: Re: [PATCH v18 22/26] drm/shmem-helper: Don't free refcounted GEM

On Mon, 30 Oct 2023 02:02:01 +0300
Dmitry Osipenko <dmitry.osipenko@...labora.com> wrote:

> Don't free refcounted shmem object to prevent use-after-free bug that
> is worse than a memory leak.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@...labora.com>
> ---
>  drivers/gpu/drm/drm_gem_shmem_helper.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c
> index 6dd087f19ea3..4253c367dc07 100644
> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c
> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c
> @@ -203,9 +203,10 @@ void drm_gem_shmem_free(struct drm_gem_shmem_object *shmem)
>  	if (obj->import_attach)
>  		drm_prime_gem_destroy(obj, shmem->sgt);
>  
> -	drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count));
> -	drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count));
> -	drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count));
> +	if (drm_WARN_ON(obj->dev, refcount_read(&shmem->vmap_use_count)) ||
> +	    drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_use_count)) ||
> +	    drm_WARN_ON(obj->dev, refcount_read(&shmem->pages_pin_count)))
> +		return;

I guess you're worried about ->sgt being referenced by the driver after
the GEM is destroyed. If we assume drivers don't cache the sgt and
always call get_pages_sgt() when they need it that shouldn't be an
issue. What we really don't want to release is the pages themselves,
but the GPU MMU might still have active mappings pointing to these
pages.

In any case, I'm not against leaking the GEM object when any of these
counters are not zero, but can we at least have a comment in the
code explaining why we're doing that, so people don't have to go look
at the git history to figure it out.

>  
>  	drm_gem_object_release(obj);
>  	kfree(shmem);