* [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
@ 2019-11-20 12:22 Dafna Hirschfeld
2020-01-16 11:57 ` Hans Verkuil
0 siblings, 1 reply; 6+ messages in thread
From: Dafna Hirschfeld @ 2019-11-20 12:22 UTC (permalink / raw)
To: linux-media
Cc: dafna.hirschfeld, hverkuil, dafna3, helen.koike, ezequiel, stable
sd->devnode is released after calling
v4l2_subdev_release. Therefore it should be set
to NULL so that the subdev won't hold a pointer
to a released object. This fixes a reference
after free bug in function
v4l2_device_unregister_subdev
Cc: stable@vger.kernel.org
Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
---
changes since v2:
- since this is a regresion fix, I added Fixes and Cc to stable tags,
- change the commit title and log to be more clear.
drivers/media/v4l2-core/v4l2-device.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
index 63d6b147b21e..2b3595671d62 100644
--- a/drivers/media/v4l2-core/v4l2-device.c
+++ b/drivers/media/v4l2-core/v4l2-device.c
@@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
{
struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
+ sd->devnode = NULL;
if (sd->internal_ops && sd->internal_ops->release)
sd->internal_ops->release(sd);
module_put(owner);
--
2.20.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
2019-11-20 12:22 [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode Dafna Hirschfeld
@ 2020-01-16 11:57 ` Hans Verkuil
2020-01-17 10:35 ` Dafna Hirschfeld
2020-01-31 15:42 ` Ezequiel Garcia
0 siblings, 2 replies; 6+ messages in thread
From: Hans Verkuil @ 2020-01-16 11:57 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: dafna3, helen.koike, ezequiel, stable
On 11/20/19 1:22 PM, Dafna Hirschfeld wrote:
> sd->devnode is released after calling
> v4l2_subdev_release. Therefore it should be set
> to NULL so that the subdev won't hold a pointer
> to a released object. This fixes a reference
> after free bug in function
> v4l2_device_unregister_subdev
>
> Cc: stable@vger.kernel.org
> Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> ---
> changes since v2:
> - since this is a regresion fix, I added Fixes and Cc to stable tags,
> - change the commit title and log to be more clear.
>
> drivers/media/v4l2-core/v4l2-device.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> index 63d6b147b21e..2b3595671d62 100644
> --- a/drivers/media/v4l2-core/v4l2-device.c
> +++ b/drivers/media/v4l2-core/v4l2-device.c
> @@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
> {
> struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
>
> + sd->devnode = NULL;
> if (sd->internal_ops && sd->internal_ops->release)
> sd->internal_ops->release(sd);
I'd move the sd->devnode = NULL; line here. That way the
sd->internal_ops->release(sd) callback can still use it.
Unless I am missing something?
> module_put(owner);
>
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
2020-01-16 11:57 ` Hans Verkuil
@ 2020-01-17 10:35 ` Dafna Hirschfeld
2020-01-17 10:46 ` Hans Verkuil
2020-01-31 15:42 ` Ezequiel Garcia
1 sibling, 1 reply; 6+ messages in thread
From: Dafna Hirschfeld @ 2020-01-17 10:35 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: dafna3, helen.koike, ezequiel, stable
Hi
On 16.01.20 13:57, Hans Verkuil wrote:
> On 11/20/19 1:22 PM, Dafna Hirschfeld wrote:
>> sd->devnode is released after calling
>> v4l2_subdev_release. Therefore it should be set
>> to NULL so that the subdev won't hold a pointer
>> to a released object. This fixes a reference
>> after free bug in function
>> v4l2_device_unregister_subdev
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>> ---
>> changes since v2:
>> - since this is a regresion fix, I added Fixes and Cc to stable tags,
>> - change the commit title and log to be more clear.
>>
>> drivers/media/v4l2-core/v4l2-device.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>> index 63d6b147b21e..2b3595671d62 100644
>> --- a/drivers/media/v4l2-core/v4l2-device.c
>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>> @@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
>> {
>> struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
>>
>> + sd->devnode = NULL;
>> if (sd->internal_ops && sd->internal_ops->release)
>> sd->internal_ops->release(sd);
>
> I'd move the sd->devnode = NULL; line here. That way the
> sd->internal_ops->release(sd) callback can still use it.
>
> Unless I am missing something?
It makes sense although none of the drivers uses this callback since
the subdevice should be released in the v4l2_device's release so it
seems that this callback can (should?) be removed.
Dafna
>
>> module_put(owner);
>>
>
> Regards,
>
> Hans
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
2020-01-17 10:35 ` Dafna Hirschfeld
@ 2020-01-17 10:46 ` Hans Verkuil
2020-01-17 10:52 ` Dafna Hirschfeld
0 siblings, 1 reply; 6+ messages in thread
From: Hans Verkuil @ 2020-01-17 10:46 UTC (permalink / raw)
To: Dafna Hirschfeld, linux-media; +Cc: dafna3, helen.koike, ezequiel, stable
On 1/17/20 11:35 AM, Dafna Hirschfeld wrote:
> Hi
>
> On 16.01.20 13:57, Hans Verkuil wrote:
>> On 11/20/19 1:22 PM, Dafna Hirschfeld wrote:
>>> sd->devnode is released after calling
>>> v4l2_subdev_release. Therefore it should be set
>>> to NULL so that the subdev won't hold a pointer
>>> to a released object. This fixes a reference
>>> after free bug in function
>>> v4l2_device_unregister_subdev
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>> ---
>>> changes since v2:
>>> - since this is a regresion fix, I added Fixes and Cc to stable tags,
>>> - change the commit title and log to be more clear.
>>>
>>> drivers/media/v4l2-core/v4l2-device.c | 1 +
>>> 1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>>> index 63d6b147b21e..2b3595671d62 100644
>>> --- a/drivers/media/v4l2-core/v4l2-device.c
>>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>>> @@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
>>> {
>>> struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
>>>
>>> + sd->devnode = NULL;
>>> if (sd->internal_ops && sd->internal_ops->release)
>>> sd->internal_ops->release(sd);
>>
>> I'd move the sd->devnode = NULL; line here. That way the
>> sd->internal_ops->release(sd) callback can still use it.
>>
>> Unless I am missing something?
> It makes sense although none of the drivers uses this callback since
> the subdevice should be released in the v4l2_device's release so it
> seems that this callback can (should?) be removed.
If nobody uses it, then I agree that it is better to remove it.
Regards,
Hans
>
> Dafna
>
>>
>>> module_put(owner);
>>>
>>
>> Regards,
>>
>> Hans
>>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
2020-01-17 10:46 ` Hans Verkuil
@ 2020-01-17 10:52 ` Dafna Hirschfeld
0 siblings, 0 replies; 6+ messages in thread
From: Dafna Hirschfeld @ 2020-01-17 10:52 UTC (permalink / raw)
To: Hans Verkuil, linux-media; +Cc: dafna3, helen.koike, ezequiel, stable
On 17.01.20 12:46, Hans Verkuil wrote:
> On 1/17/20 11:35 AM, Dafna Hirschfeld wrote:
>> Hi
>>
>> On 16.01.20 13:57, Hans Verkuil wrote:
>>> On 11/20/19 1:22 PM, Dafna Hirschfeld wrote:
>>>> sd->devnode is released after calling
>>>> v4l2_subdev_release. Therefore it should be set
>>>> to NULL so that the subdev won't hold a pointer
>>>> to a released object. This fixes a reference
>>>> after free bug in function
>>>> v4l2_device_unregister_subdev
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
>>>> Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
>>>> ---
>>>> changes since v2:
>>>> - since this is a regresion fix, I added Fixes and Cc to stable tags,
>>>> - change the commit title and log to be more clear.
>>>>
>>>> drivers/media/v4l2-core/v4l2-device.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
>>>> index 63d6b147b21e..2b3595671d62 100644
>>>> --- a/drivers/media/v4l2-core/v4l2-device.c
>>>> +++ b/drivers/media/v4l2-core/v4l2-device.c
>>>> @@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
>>>> {
>>>> struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
>>>>
>>>> + sd->devnode = NULL;
>>>> if (sd->internal_ops && sd->internal_ops->release)
>>>> sd->internal_ops->release(sd);
>>>
>>> I'd move the sd->devnode = NULL; line here. That way the
>>> sd->internal_ops->release(sd) callback can still use it.
>>>
>>> Unless I am missing something?
>> It makes sense although none of the drivers uses this callback since
>> the subdevice should be released in the v4l2_device's release so it
>> seems that this callback can (should?) be removed.
>
> If nobody uses it, then I agree that it is better to remove it.
ok, currently only vimc implements this callback
this would change after the patchset `media: vimc: race condition fixes`
will be accepted. Then I can send a patch removing it.
Dafna
>
> Regards,
>
> Hans
>
>>
>> Dafna
>>
>>>
>>>> module_put(owner);
>>>>
>>>
>>> Regards,
>>>
>>> Hans
>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode
2020-01-16 11:57 ` Hans Verkuil
2020-01-17 10:35 ` Dafna Hirschfeld
@ 2020-01-31 15:42 ` Ezequiel Garcia
1 sibling, 0 replies; 6+ messages in thread
From: Ezequiel Garcia @ 2020-01-31 15:42 UTC (permalink / raw)
To: Hans Verkuil, Dafna Hirschfeld, linux-media; +Cc: dafna3, helen.koike, stable
On Thu, 2020-01-16 at 12:57 +0100, Hans Verkuil wrote:
> On 11/20/19 1:22 PM, Dafna Hirschfeld wrote:
> > sd->devnode is released after calling
> > v4l2_subdev_release. Therefore it should be set
> > to NULL so that the subdev won't hold a pointer
> > to a released object. This fixes a reference
> > after free bug in function
> > v4l2_device_unregister_subdev
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 0e43734d4c46e ("media: v4l2-subdev: add release() internal op")
> > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> > Reviewed-by: Ezequiel Garcia <ezequiel@collabora.com>
> > ---
> > changes since v2:
> > - since this is a regresion fix, I added Fixes and Cc to stable tags,
> > - change the commit title and log to be more clear.
> >
> > drivers/media/v4l2-core/v4l2-device.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-device.c b/drivers/media/v4l2-core/v4l2-device.c
> > index 63d6b147b21e..2b3595671d62 100644
> > --- a/drivers/media/v4l2-core/v4l2-device.c
> > +++ b/drivers/media/v4l2-core/v4l2-device.c
> > @@ -177,6 +177,7 @@ static void v4l2_subdev_release(struct v4l2_subdev *sd)
> > {
> > struct module *owner = !sd->owner_v4l2_dev ? sd->owner : NULL;
> >
> > + sd->devnode = NULL;
> > if (sd->internal_ops && sd->internal_ops->release)
> > sd->internal_ops->release(sd);
>
> I'd move the sd->devnode = NULL; line here. That way the
> sd->internal_ops->release(sd) callback can still use it.
>
Hi everyone,
Please note this fix is useful to fix a kernel oops
when rkisp1 driver is removed.
Can we get a v4 addressing Hans' feedback?
Thanks,
Ezequiel
> Unless I am missing something?
>
> > module_put(owner);
> >
>
> Regards,
>
> Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-31 15:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 12:22 [PATCH v3] media: v4l2-core: fix a use-after-free bug of sd->devnode Dafna Hirschfeld
2020-01-16 11:57 ` Hans Verkuil
2020-01-17 10:35 ` Dafna Hirschfeld
2020-01-17 10:46 ` Hans Verkuil
2020-01-17 10:52 ` Dafna Hirschfeld
2020-01-31 15:42 ` Ezequiel Garcia
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).