[PATCH] xen/gntdev: fix refcount leak in gntdev_ioctl_map_grant_ref()

[PATCH] xen/gntdev: fix refcount leak in gntdev_ioctl_map_grant_ref()

[WenTao Liang]

When gntdev_ioctl_map_grant_ref() fails to copy the operation
result back to userspace after successfully adding the mapping to
the list, the error path returns -EFAULT without releasing the
reference acquired by gntdev_alloc_map(). The mapping remains in
priv->maps with a refcount of 1, causing a memory leak and a
dangling list entry.

Fix this by moving the copy_to_user() before gntdev_add_map(),
so that the mapping is only inserted into the list on success.
This avoids the need to remove the mapping from the list on error.

Cc: stable@vger.kernel.org
Fixes: 68b025c813c2 ("xen-gntdev: Add reference counting to maps")
Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
---
 drivers/xen/gntdev.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 61ea855c4508..a1c230756b3d 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -672,8 +672,13 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
 	op.index = map->index << PAGE_SHIFT;
 	mutex_unlock(&priv->lock);
 
-	if (copy_to_user(u, &op, sizeof(op)) != 0)
+	if (copy_to_user(u, &op, sizeof(op)) != 0) {
+		mutex_lock(&priv->lock);
+		list_del(&map->next);
+		mutex_unlock(&priv->lock);
+		gntdev_put_map(priv, map);
 		return -EFAULT;
+	}
 
 	return 0;
 }
-- 
2.50.1 (Apple Git-155)

Re: [PATCH] xen/gntdev: fix refcount leak in gntdev_ioctl_map_grant_ref()

[Juergen Gross]

[-- Attachment #1.1.1: Type: text/plain, Size: 1696 bytes --]

On 11.06.26 16:23, WenTao Liang wrote:
> When gntdev_ioctl_map_grant_ref() fails to copy the operation
> result back to userspace after successfully adding the mapping to
> the list, the error path returns -EFAULT without releasing the
> reference acquired by gntdev_alloc_map(). The mapping remains in
> priv->maps with a refcount of 1, causing a memory leak and a
> dangling list entry.
> 
> Fix this by moving the copy_to_user() before gntdev_add_map(),
> so that the mapping is only inserted into the list on success.
> This avoids the need to remove the mapping from the list on error.
> 
> Cc: stable@vger.kernel.org
> Fixes: 68b025c813c2 ("xen-gntdev: Add reference counting to maps")
> Signed-off-by: WenTao Liang <vulab@iscas.ac.cn>
> ---
>   drivers/xen/gntdev.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 61ea855c4508..a1c230756b3d 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -672,8 +672,13 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
>   	op.index = map->index << PAGE_SHIFT;
>   	mutex_unlock(&priv->lock);
>   
> -	if (copy_to_user(u, &op, sizeof(op)) != 0)
> +	if (copy_to_user(u, &op, sizeof(op)) != 0) {
> +		mutex_lock(&priv->lock);
> +		list_del(&map->next);
> +		mutex_unlock(&priv->lock);

I don't think this is race free.

Just dereferencing map without verifying it is still on the list (the mutex was
dropped in between!) might access an already freed object.

I think you need to keep the mutex held across the copy_to_user() and drop it
only after the last time accessing map.


Juergen

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3743 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]