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

[Juergen Gross]

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@xxxxxxxxxxxxxxx
> Fixes: 68b025c813c2 ("xen-gntdev: Add reference counting to maps")
> Signed-off-by: WenTao Liang <vulab@xxxxxxxxxxx>
> ---
>   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: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature