drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

[PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

Posted by T.J. Mercier 1 year, 5 months ago

A syncobj reference is taken in drm_syncobj_find, but not released if
eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
paths.

Reported-by: Xingyu Jin <xingyuj@google.com>
Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
Signed-off-by: T.J. Mercier <tjmercier@google.com>
---
 drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
index a0e94217b511..4fcfc0b9b386 100644
--- a/drivers/gpu/drm/drm_syncobj.c
+++ b/drivers/gpu/drm/drm_syncobj.c
@@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
 	struct drm_syncobj *syncobj;
 	struct eventfd_ctx *ev_fd_ctx;
 	struct syncobj_eventfd_entry *entry;
+	int ret;
 
 	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
 		return -EOPNOTSUPP;
@@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
 		return -ENOENT;
 
 	ev_fd_ctx = eventfd_ctx_fdget(args->fd);
-	if (IS_ERR(ev_fd_ctx))
-		return PTR_ERR(ev_fd_ctx);
+	if (IS_ERR(ev_fd_ctx)) {
+		ret = PTR_ERR(ev_fd_ctx);
+		goto err_fdget;
+	}
 
 	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
 	if (!entry) {
-		eventfd_ctx_put(ev_fd_ctx);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err_kzalloc;
 	}
 	entry->syncobj = syncobj;
 	entry->ev_fd_ctx = ev_fd_ctx;
@@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
 	drm_syncobj_put(syncobj);
 
 	return 0;
+
+err_kzalloc:
+	eventfd_ctx_put(ev_fd_ctx);
+err_fdget:
+	drm_syncobj_put(syncobj);
+	return ret;
 }
 
 int
-- 
2.46.0.598.g6f2099f65c-goog

Re: [PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

Posted by Tvrtko Ursulin 1 year, 5 months ago

On 09/09/2024 21:53, T.J. Mercier wrote:
> A syncobj reference is taken in drm_syncobj_find, but not released if
> eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
> paths.
> 
> Reported-by: Xingyu Jin <xingyuj@google.com>
> Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> ---
>   drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
> index a0e94217b511..4fcfc0b9b386 100644
> --- a/drivers/gpu/drm/drm_syncobj.c
> +++ b/drivers/gpu/drm/drm_syncobj.c
> @@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
>   	struct drm_syncobj *syncobj;
>   	struct eventfd_ctx *ev_fd_ctx;
>   	struct syncobj_eventfd_entry *entry;
> +	int ret;
>   
>   	if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>   		return -EOPNOTSUPP;
> @@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
>   		return -ENOENT;
>   
>   	ev_fd_ctx = eventfd_ctx_fdget(args->fd);
> -	if (IS_ERR(ev_fd_ctx))
> -		return PTR_ERR(ev_fd_ctx);
> +	if (IS_ERR(ev_fd_ctx)) {
> +		ret = PTR_ERR(ev_fd_ctx);
> +		goto err_fdget;
> +	}
>   
>   	entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>   	if (!entry) {
> -		eventfd_ctx_put(ev_fd_ctx);
> -		return -ENOMEM;
> +		ret = -ENOMEM;
> +		goto err_kzalloc;
>   	}
>   	entry->syncobj = syncobj;
>   	entry->ev_fd_ctx = ev_fd_ctx;
> @@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device *dev, void *data,
>   	drm_syncobj_put(syncobj);
>   
>   	return 0;
> +
> +err_kzalloc:
> +	eventfd_ctx_put(ev_fd_ctx);
> +err_fdget:
> +	drm_syncobj_put(syncobj);
> +	return ret;
>   }
>   
>   int

Easy enough to review while browsing the list:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

Re: [PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

Posted by Christian König 1 year, 5 months ago

Am 10.09.24 um 09:26 schrieb Tvrtko Ursulin:
>
> On 09/09/2024 21:53, T.J. Mercier wrote:
>> A syncobj reference is taken in drm_syncobj_find, but not released if
>> eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
>> paths.
>>
>> Reported-by: Xingyu Jin <xingyuj@google.com>
>> Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
>>   1 file changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c 
>> b/drivers/gpu/drm/drm_syncobj.c
>> index a0e94217b511..4fcfc0b9b386 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device 
>> *dev, void *data,
>>       struct drm_syncobj *syncobj;
>>       struct eventfd_ctx *ev_fd_ctx;
>>       struct syncobj_eventfd_entry *entry;
>> +    int ret;
>>         if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>>           return -EOPNOTSUPP;
>> @@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device 
>> *dev, void *data,
>>           return -ENOENT;
>>         ev_fd_ctx = eventfd_ctx_fdget(args->fd);
>> -    if (IS_ERR(ev_fd_ctx))
>> -        return PTR_ERR(ev_fd_ctx);
>> +    if (IS_ERR(ev_fd_ctx)) {
>> +        ret = PTR_ERR(ev_fd_ctx);
>> +        goto err_fdget;
>> +    }
>>         entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>>       if (!entry) {
>> -        eventfd_ctx_put(ev_fd_ctx);
>> -        return -ENOMEM;
>> +        ret = -ENOMEM;
>> +        goto err_kzalloc;
>>       }
>>       entry->syncobj = syncobj;
>>       entry->ev_fd_ctx = ev_fd_ctx;
>> @@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device 
>> *dev, void *data,
>>       drm_syncobj_put(syncobj);
>>         return 0;
>> +
>> +err_kzalloc:
>> +    eventfd_ctx_put(ev_fd_ctx);
>> +err_fdget:
>> +    drm_syncobj_put(syncobj);
>> +    return ret;
>>   }
>>     int
>
> Easy enough to review while browsing the list:
>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Looks reasonable to me as well.

Reviewed-by. Christian König <christian.koenig@amd.com>
CC: stable?

Let me know when you need someone to push it to drm-misc-fixes.

Regards,
Christian.

>
> Regards,
>
> Tvrtko

Re: [PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

Posted by T.J. Mercier 1 year, 4 months ago

On Tue, Sep 10, 2024 at 12:30 AM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 10.09.24 um 09:26 schrieb Tvrtko Ursulin:
> >
> > On 09/09/2024 21:53, T.J. Mercier wrote:
> >> A syncobj reference is taken in drm_syncobj_find, but not released if
> >> eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
> >> paths.
> >>
> >> Reported-by: Xingyu Jin <xingyuj@google.com>
> >> Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
> >> Signed-off-by: T.J. Mercier <tjmercier@google.com>
> >> ---
> >>   drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
> >>   1 file changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_syncobj.c
> >> b/drivers/gpu/drm/drm_syncobj.c
> >> index a0e94217b511..4fcfc0b9b386 100644
> >> --- a/drivers/gpu/drm/drm_syncobj.c
> >> +++ b/drivers/gpu/drm/drm_syncobj.c
> >> @@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> >> *dev, void *data,
> >>       struct drm_syncobj *syncobj;
> >>       struct eventfd_ctx *ev_fd_ctx;
> >>       struct syncobj_eventfd_entry *entry;
> >> +    int ret;
> >>         if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
> >>           return -EOPNOTSUPP;
> >> @@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> >> *dev, void *data,
> >>           return -ENOENT;
> >>         ev_fd_ctx = eventfd_ctx_fdget(args->fd);
> >> -    if (IS_ERR(ev_fd_ctx))
> >> -        return PTR_ERR(ev_fd_ctx);
> >> +    if (IS_ERR(ev_fd_ctx)) {
> >> +        ret = PTR_ERR(ev_fd_ctx);
> >> +        goto err_fdget;
> >> +    }
> >>         entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> >>       if (!entry) {
> >> -        eventfd_ctx_put(ev_fd_ctx);
> >> -        return -ENOMEM;
> >> +        ret = -ENOMEM;
> >> +        goto err_kzalloc;
> >>       }
> >>       entry->syncobj = syncobj;
> >>       entry->ev_fd_ctx = ev_fd_ctx;
> >> @@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device
> >> *dev, void *data,
> >>       drm_syncobj_put(syncobj);
> >>         return 0;
> >> +
> >> +err_kzalloc:
> >> +    eventfd_ctx_put(ev_fd_ctx);
> >> +err_fdget:
> >> +    drm_syncobj_put(syncobj);
> >> +    return ret;
> >>   }
> >>     int
> >
> > Easy enough to review while browsing the list:
> >
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>
> Looks reasonable to me as well.
>
> Reviewed-by. Christian König <christian.koenig@amd.com>

Thanks!

> CC: stable?

Yes, I think we should. 6.6 and 6.10

> Let me know when you need someone to push it to drm-misc-fixes.

Anytime is good, no rush for this one.

>
> Regards,
> Christian.
>
> >
> > Regards,
> >
> > Tvrtko
>

Re: [PATCH] drm/syncobj: Fix syncobj leak in drm_syncobj_eventfd_ioctl

Posted by Christian König 1 year, 4 months ago

Am 10.09.24 um 16:47 schrieb T.J. Mercier:
> On Tue, Sep 10, 2024 at 12:30 AM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 10.09.24 um 09:26 schrieb Tvrtko Ursulin:
>>> On 09/09/2024 21:53, T.J. Mercier wrote:
>>>> A syncobj reference is taken in drm_syncobj_find, but not released if
>>>> eventfd_ctx_fdget or kzalloc fails. Put the reference in these error
>>>> paths.
>>>>
>>>> Reported-by: Xingyu Jin <xingyuj@google.com>
>>>> Fixes: c7a472297169 ("drm/syncobj: add IOCTL to register an eventfd")
>>>> Signed-off-by: T.J. Mercier <tjmercier@google.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_syncobj.c | 17 +++++++++++++----
>>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_syncobj.c
>>>> b/drivers/gpu/drm/drm_syncobj.c
>>>> index a0e94217b511..4fcfc0b9b386 100644
>>>> --- a/drivers/gpu/drm/drm_syncobj.c
>>>> +++ b/drivers/gpu/drm/drm_syncobj.c
>>>> @@ -1464,6 +1464,7 @@ drm_syncobj_eventfd_ioctl(struct drm_device
>>>> *dev, void *data,
>>>>        struct drm_syncobj *syncobj;
>>>>        struct eventfd_ctx *ev_fd_ctx;
>>>>        struct syncobj_eventfd_entry *entry;
>>>> +    int ret;
>>>>          if (!drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE))
>>>>            return -EOPNOTSUPP;
>>>> @@ -1479,13 +1480,15 @@ drm_syncobj_eventfd_ioctl(struct drm_device
>>>> *dev, void *data,
>>>>            return -ENOENT;
>>>>          ev_fd_ctx = eventfd_ctx_fdget(args->fd);
>>>> -    if (IS_ERR(ev_fd_ctx))
>>>> -        return PTR_ERR(ev_fd_ctx);
>>>> +    if (IS_ERR(ev_fd_ctx)) {
>>>> +        ret = PTR_ERR(ev_fd_ctx);
>>>> +        goto err_fdget;
>>>> +    }
>>>>          entry = kzalloc(sizeof(*entry), GFP_KERNEL);
>>>>        if (!entry) {
>>>> -        eventfd_ctx_put(ev_fd_ctx);
>>>> -        return -ENOMEM;
>>>> +        ret = -ENOMEM;
>>>> +        goto err_kzalloc;
>>>>        }
>>>>        entry->syncobj = syncobj;
>>>>        entry->ev_fd_ctx = ev_fd_ctx;
>>>> @@ -1496,6 +1499,12 @@ drm_syncobj_eventfd_ioctl(struct drm_device
>>>> *dev, void *data,
>>>>        drm_syncobj_put(syncobj);
>>>>          return 0;
>>>> +
>>>> +err_kzalloc:
>>>> +    eventfd_ctx_put(ev_fd_ctx);
>>>> +err_fdget:
>>>> +    drm_syncobj_put(syncobj);
>>>> +    return ret;
>>>>    }
>>>>      int
>>> Easy enough to review while browsing the list:
>>>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
>> Looks reasonable to me as well.
>>
>> Reviewed-by. Christian König <christian.koenig@amd.com>
> Thanks!
>
>> CC: stable?
> Yes, I think we should. 6.6 and 6.10
>
>> Let me know when you need someone to push it to drm-misc-fixes.
> Anytime is good, no rush for this one.

Done.

Christian.

>
>> Regards,
>> Christian.
>>
>>> Regards,
>>>
>>> Tvrtko