| Message ID | 20191120122217.845-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] media: v4l2-core: fix a use-after-free bug of sd->devnode | expand |
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
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 >
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 >>
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 >>> >
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
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);
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(+)