Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
* [Patch V3] v4l2-core: fix use-after-free error
@ 2019-04-15 15:36 Sumit Gupta
  2019-04-24 11:07 ` Hans Verkuil
  0 siblings, 1 reply; 3+ messages in thread
From: Sumit Gupta @ 2019-04-15 15:36 UTC (permalink / raw)
  To: mchehab, hverkuil-cisco, sakari.ailus, paul.kocialkowski, tfiga,
	keiichiw, sumitg, bbasu
  Cc: linux-media, linux-kernel

From: sumitg <sumitg@nvidia.com>

Fixing use-after-free within __v4l2_ctrl_handler_setup().
Memory is being freed with kfree(new_ref) for duplicate
control reference entry but ctrl->cluster pointer is still
referring to freed duplicate entry resulting in error on
access. Change done to update cluster pointer only when new
control reference is added. Also, added check to add new
ctrl to handler only if the cluster points to an entry.

 ==================================================================
 BUG: KASAN: use-after-free in __v4l2_ctrl_handler_setup+0x388/0x428
 Read of size 8 at addr ffffffc324e78618 by task systemd-udevd/312

 Allocated by task 312:

 Freed by task 312:

 The buggy address belongs to the object at ffffffc324e78600
  which belongs to the cache kmalloc-64 of size 64
 The buggy address is located 24 bytes inside of
  64-byte region [ffffffc324e78600, ffffffc324e78640)
 The buggy address belongs to the page:
 page:ffffffbf0c939e00 count:1 mapcount:0 mapping:
					(null) index:0xffffffc324e78f80
 flags: 0x4000000000000100(slab)
 raw: 4000000000000100 0000000000000000 ffffffc324e78f80 000000018020001a
 raw: 0000000000000000 0000000100000001 ffffffc37040fb80 0000000000000000
 page dumped because: kasan: bad access detected

 Memory state around the buggy address:
  ffffffc324e78500: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
  ffffffc324e78580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
 >ffffffc324e78600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
                             ^
  ffffffc324e78680: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
  ffffffc324e78700: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
 ==================================================================

Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
---

v3:
* update ctrl->cluster only when new control reference is added.
* add new ctrl to handler only if the cluster points to an entry.

v2:
* update ctrl->cluster only when new control reference is added.
* check ctrl->ncontrols to avoid illegal access when cluster has zero controls.

 drivers/media/v4l2-core/v4l2-ctrls.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 5e3806f..877c2ab 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2154,15 +2154,6 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	if (size_extra_req)
 		new_ref->p_req.p = &new_ref[1];
 
-	if (ctrl->handler == hdl) {
-		/* By default each control starts in a cluster of its own.
-		   new_ref->ctrl is basically a cluster array with one
-		   element, so that's perfect to use as the cluster pointer.
-		   But only do this for the handler that owns the control. */
-		ctrl->cluster = &new_ref->ctrl;
-		ctrl->ncontrols = 1;
-	}
-
 	INIT_LIST_HEAD(&new_ref->node);
 
 	mutex_lock(hdl->lock);
@@ -2195,6 +2186,15 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
 	hdl->buckets[bucket] = new_ref;
 	if (ctrl_ref)
 		*ctrl_ref = new_ref;
+	if (ctrl->handler == hdl) {
+		/* By default each control starts in a cluster of its own.
+		 * new_ref->ctrl is basically a cluster array with one
+		 * element, so that's perfect to use as the cluster pointer.
+		 * But only do this for the handler that owns the control.
+		 */
+		ctrl->cluster = &new_ref->ctrl;
+		ctrl->ncontrols = 1;
+	}
 
 unlock:
 	mutex_unlock(hdl->lock);
@@ -2346,9 +2346,11 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		kvfree(ctrl);
 		return NULL;
 	}
-	mutex_lock(hdl->lock);
-	list_add_tail(&ctrl->node, &hdl->ctrls);
-	mutex_unlock(hdl->lock);
+	if (ctrl->cluster) {
+		mutex_lock(hdl->lock);
+		list_add_tail(&ctrl->node, &hdl->ctrls);
+		mutex_unlock(hdl->lock);
+	}
 	return ctrl;
 }
 
-- 
2.7.4


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Patch V3] v4l2-core: fix use-after-free error
  2019-04-15 15:36 [Patch V3] v4l2-core: fix use-after-free error Sumit Gupta
@ 2019-04-24 11:07 ` Hans Verkuil
  2019-05-17 13:26   ` sumitg
  0 siblings, 1 reply; 3+ messages in thread
From: Hans Verkuil @ 2019-04-24 11:07 UTC (permalink / raw)
  To: Sumit Gupta, mchehab, sakari.ailus, paul.kocialkowski, tfiga,
	keiichiw, bbasu
  Cc: linux-media, linux-kernel

On 4/15/19 5:36 PM, Sumit Gupta wrote:
> From: sumitg <sumitg@nvidia.com>
> 
> Fixing use-after-free within __v4l2_ctrl_handler_setup().
> Memory is being freed with kfree(new_ref) for duplicate
> control reference entry but ctrl->cluster pointer is still
> referring to freed duplicate entry resulting in error on
> access. Change done to update cluster pointer only when new
> control reference is added. Also, added check to add new
> ctrl to handler only if the cluster points to an entry.
> 
>  ==================================================================
>  BUG: KASAN: use-after-free in __v4l2_ctrl_handler_setup+0x388/0x428
>  Read of size 8 at addr ffffffc324e78618 by task systemd-udevd/312
> 
>  Allocated by task 312:
> 
>  Freed by task 312:
> 
>  The buggy address belongs to the object at ffffffc324e78600
>   which belongs to the cache kmalloc-64 of size 64
>  The buggy address is located 24 bytes inside of
>   64-byte region [ffffffc324e78600, ffffffc324e78640)
>  The buggy address belongs to the page:
>  page:ffffffbf0c939e00 count:1 mapcount:0 mapping:
> 					(null) index:0xffffffc324e78f80
>  flags: 0x4000000000000100(slab)
>  raw: 4000000000000100 0000000000000000 ffffffc324e78f80 000000018020001a
>  raw: 0000000000000000 0000000100000001 ffffffc37040fb80 0000000000000000
>  page dumped because: kasan: bad access detected
> 
>  Memory state around the buggy address:
>   ffffffc324e78500: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>   ffffffc324e78580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>  >ffffffc324e78600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>                              ^
>   ffffffc324e78680: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
>   ffffffc324e78700: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>  ==================================================================
> 
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
> ---
> 
> v3:
> * update ctrl->cluster only when new control reference is added.
> * add new ctrl to handler only if the cluster points to an entry.
> 
> v2:
> * update ctrl->cluster only when new control reference is added.
> * check ctrl->ncontrols to avoid illegal access when cluster has zero controls.
> 
>  drivers/media/v4l2-core/v4l2-ctrls.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 5e3806f..877c2ab 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2154,15 +2154,6 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
>  	if (size_extra_req)
>  		new_ref->p_req.p = &new_ref[1];
>  
> -	if (ctrl->handler == hdl) {
> -		/* By default each control starts in a cluster of its own.
> -		   new_ref->ctrl is basically a cluster array with one
> -		   element, so that's perfect to use as the cluster pointer.
> -		   But only do this for the handler that owns the control. */
> -		ctrl->cluster = &new_ref->ctrl;
> -		ctrl->ncontrols = 1;
> -	}
> -
>  	INIT_LIST_HEAD(&new_ref->node);
>  
>  	mutex_lock(hdl->lock);
> @@ -2195,6 +2186,15 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
>  	hdl->buckets[bucket] = new_ref;
>  	if (ctrl_ref)
>  		*ctrl_ref = new_ref;
> +	if (ctrl->handler == hdl) {
> +		/* By default each control starts in a cluster of its own.
> +		 * new_ref->ctrl is basically a cluster array with one
> +		 * element, so that's perfect to use as the cluster pointer.
> +		 * But only do this for the handler that owns the control.
> +		 */
> +		ctrl->cluster = &new_ref->ctrl;
> +		ctrl->ncontrols = 1;
> +	}

This is good.

>  
>  unlock:
>  	mutex_unlock(hdl->lock);
> @@ -2346,9 +2346,11 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  		kvfree(ctrl);
>  		return NULL;
>  	}
> -	mutex_lock(hdl->lock);
> -	list_add_tail(&ctrl->node, &hdl->ctrls);
> -	mutex_unlock(hdl->lock);
> +	if (ctrl->cluster) {
> +		mutex_lock(hdl->lock);
> +		list_add_tail(&ctrl->node, &hdl->ctrls);
> +		mutex_unlock(hdl->lock);
> +	}

But why change this? ctrl->cluster can never be NULL here.

If ctrl->cluster really is NULL for you, then something else
is wrong.

Regards,

	Hans

>  	return ctrl;
>  }
>  
> 


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Patch V3] v4l2-core: fix use-after-free error
  2019-04-24 11:07 ` Hans Verkuil
@ 2019-05-17 13:26   ` sumitg
  0 siblings, 0 replies; 3+ messages in thread
From: sumitg @ 2019-05-17 13:26 UTC (permalink / raw)
  To: Hans Verkuil, mchehab, sakari.ailus, paul.kocialkowski, tfiga,
	keiichiw, bbasu
  Cc: linux-media, linux-kernel


On 24/04/19 4:37 PM, Hans Verkuil wrote:
> On 4/15/19 5:36 PM, Sumit Gupta wrote:
>> From: sumitg <sumitg@nvidia.com>
>>
>> Fixing use-after-free within __v4l2_ctrl_handler_setup().
>> Memory is being freed with kfree(new_ref) for duplicate
>> control reference entry but ctrl->cluster pointer is still
>> referring to freed duplicate entry resulting in error on
>> access. Change done to update cluster pointer only when new
>> control reference is added. Also, added check to add new
>> ctrl to handler only if the cluster points to an entry.
>>
>>   ==================================================================
>>   BUG: KASAN: use-after-free in __v4l2_ctrl_handler_setup+0x388/0x428
>>   Read of size 8 at addr ffffffc324e78618 by task systemd-udevd/312
>>
>>   Allocated by task 312:
>>
>>   Freed by task 312:
>>
>>   The buggy address belongs to the object at ffffffc324e78600
>>    which belongs to the cache kmalloc-64 of size 64
>>   The buggy address is located 24 bytes inside of
>>    64-byte region [ffffffc324e78600, ffffffc324e78640)
>>   The buggy address belongs to the page:
>>   page:ffffffbf0c939e00 count:1 mapcount:0 mapping:
>> 					(null) index:0xffffffc324e78f80
>>   flags: 0x4000000000000100(slab)
>>   raw: 4000000000000100 0000000000000000 ffffffc324e78f80 000000018020001a
>>   raw: 0000000000000000 0000000100000001 ffffffc37040fb80 0000000000000000
>>   page dumped because: kasan: bad access detected
>>
>>   Memory state around the buggy address:
>>    ffffffc324e78500: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>>    ffffffc324e78580: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>>   >ffffffc324e78600: fb fb fb fb fb fb fb fb fc fc fc fc fc fc fc fc
>>                               ^
>>    ffffffc324e78680: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
>>    ffffffc324e78700: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
>>   ==================================================================
>>
>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> Signed-off-by: Sumit Gupta <sumitg@nvidia.com>
>> ---
>>
>> v3:
>> * update ctrl->cluster only when new control reference is added.
>> * add new ctrl to handler only if the cluster points to an entry.
>>
>> v2:
>> * update ctrl->cluster only when new control reference is added.
>> * check ctrl->ncontrols to avoid illegal access when cluster has zero controls.
>>
>>   drivers/media/v4l2-core/v4l2-ctrls.c | 26 ++++++++++++++------------
>>   1 file changed, 14 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
>> index 5e3806f..877c2ab 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
>> @@ -2154,15 +2154,6 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
>>   	if (size_extra_req)
>>   		new_ref->p_req.p = &new_ref[1];
>>   
>> -	if (ctrl->handler == hdl) {
>> -		/* By default each control starts in a cluster of its own.
>> -		   new_ref->ctrl is basically a cluster array with one
>> -		   element, so that's perfect to use as the cluster pointer.
>> -		   But only do this for the handler that owns the control. */
>> -		ctrl->cluster = &new_ref->ctrl;
>> -		ctrl->ncontrols = 1;
>> -	}
>> -
>>   	INIT_LIST_HEAD(&new_ref->node);
>>   
>>   	mutex_lock(hdl->lock);
>> @@ -2195,6 +2186,15 @@ static int handler_new_ref(struct v4l2_ctrl_handler *hdl,
>>   	hdl->buckets[bucket] = new_ref;
>>   	if (ctrl_ref)
>>   		*ctrl_ref = new_ref;
>> +	if (ctrl->handler == hdl) {
>> +		/* By default each control starts in a cluster of its own.
>> +		 * new_ref->ctrl is basically a cluster array with one
>> +		 * element, so that's perfect to use as the cluster pointer.
>> +		 * But only do this for the handler that owns the control.
>> +		 */
>> +		ctrl->cluster = &new_ref->ctrl;
>> +		ctrl->ncontrols = 1;
>> +	}
> This is good.
>
>>   
>>   unlock:
>>   	mutex_unlock(hdl->lock);
>> @@ -2346,9 +2346,11 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>>   		kvfree(ctrl);
>>   		return NULL;
>>   	}
>> -	mutex_lock(hdl->lock);
>> -	list_add_tail(&ctrl->node, &hdl->ctrls);
>> -	mutex_unlock(hdl->lock);
>> +	if (ctrl->cluster) {
>> +		mutex_lock(hdl->lock);
>> +		list_add_tail(&ctrl->node, &hdl->ctrls);
>> +		mutex_unlock(hdl->lock);
>> +	}
> But why change this? ctrl->cluster can never be NULL here.
>
> If ctrl->cluster really is NULL for you, then something else
> is wrong.

I checked it again but issue is not visible now in my setup.

So, as suspected by you there might be some other issue which

was causing this problem earlier. But now that looks fixed. Sending

first patch as code improvement to avoid use-after-free problem.

>
> Regards,
>
> 	Hans
>
>>   	return ctrl;
>>   }
>>   
>>

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, back to index

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-15 15:36 [Patch V3] v4l2-core: fix use-after-free error Sumit Gupta
2019-04-24 11:07 ` Hans Verkuil
2019-05-17 13:26   ` sumitg

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox