[PATCH v3.16 0/2] V4L2 event subscription fixes

[PATCH v3.16 0/2] V4L2 event subscription fixes

[Sakari Ailus]

Hi Ben,

The two patches fix a use-after-free issue in V4L2 event handling. The
first patch that fixes that issue is already in the other stable trees (as
well as Linus's tree) whereas the second that fixes a bug in the first
one, is in the media tree only as of yet.

<URL:https://git.linuxtv.org/media_tree.git/commit/?id=92539d3eda2c090b382699bbb896d4b54e9bdece>

Sakari Ailus (2):
  v4l: event: Prevent freeing event subscriptions while accessed
  v4l: event: Add subscription to list before calling "add" operation

 drivers/media/v4l2-core/v4l2-event.c | 63 ++++++++++++++++++++----------------
 drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
 include/media/v4l2-fh.h              |  1 +
 3 files changed, 38 insertions(+), 28 deletions(-)

-- 
2.11.0

[PATCH v3.16 1/2] v4l: event: Prevent freeing event subscriptions while accessed

[Sakari Ailus]

[ upstream commit ad608fbcf166fec809e402d548761768f602702c ]

The event subscriptions are added to the subscribed event list while
holding a spinlock, but that lock is subsequently released while still
accessing the subscription object. This makes it possible to unsubscribe
the event --- and freeing the subscription object's memory --- while
the subscription object is simultaneously accessed.

Prevent this by adding a mutex to serialise the event subscription and
unsubscription. This also gives a guarantee to the callback ops that the
add op has returned before the del op is called.

This change also results in making the elems field less special:
subscriptions are only added to the event list once they are fully
initialised.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: stable@vger.kernel.org # for 4.14 and up
Fixes: c3b5b0241f62 ("V4L/DVB: V4L: Events: Add backend")
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/v4l2-core/v4l2-event.c | 38 +++++++++++++++++++-----------------
 drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
 include/media/v4l2-fh.h              |  1 +
 3 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index 8761aab99de95..fcf65f131a2ac 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -119,14 +119,6 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
 	if (sev == NULL)
 		return;
 
-	/*
-	 * If the event has been added to the fh->subscribed list, but its
-	 * add op has not completed yet elems will be 0, treat this as
-	 * not being subscribed.
-	 */
-	if (!sev->elems)
-		return;
-
 	/* Increase event sequence number on fh. */
 	fh->sequence++;
 
@@ -209,6 +201,7 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	struct v4l2_subscribed_event *sev, *found_ev;
 	unsigned long flags;
 	unsigned i;
+	int ret = 0;
 
 	if (sub->type == V4L2_EVENT_ALL)
 		return -EINVAL;
@@ -226,31 +219,36 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->flags = sub->flags;
 	sev->fh = fh;
 	sev->ops = ops;
+	sev->elems = elems;
+
+	mutex_lock(&fh->subscribe_lock);
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
-	if (!found_ev)
-		list_add(&sev->list, &fh->subscribed);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
 	if (found_ev) {
+		/* Already listening */
 		kfree(sev);
-		return 0; /* Already listening */
+		goto out_unlock;
 	}
 
 	if (sev->ops && sev->ops->add) {
-		int ret = sev->ops->add(sev, elems);
+		ret = sev->ops->add(sev, elems);
 		if (ret) {
-			sev->ops = NULL;
-			v4l2_event_unsubscribe(fh, sub);
-			return ret;
+			kfree(sev);
+			goto out_unlock;
 		}
 	}
 
-	/* Mark as ready for use */
-	sev->elems = elems;
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_add(&sev->list, &fh->subscribed);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
-	return 0;
+out_unlock:
+	mutex_unlock(&fh->subscribe_lock);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
 
@@ -289,6 +287,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 		return 0;
 	}
 
+	mutex_lock(&fh->subscribe_lock);
+
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -306,6 +306,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	if (sev && sev->ops && sev->ops->del)
 		sev->ops->del(sev);
 
+	mutex_unlock(&fh->subscribe_lock);
+
 	kfree(sev);
 
 	return 0;
diff --git a/drivers/media/v4l2-core/v4l2-fh.c b/drivers/media/v4l2-core/v4l2-fh.c
index e57c002b41506..573ec2f192d44 100644
--- a/drivers/media/v4l2-core/v4l2-fh.c
+++ b/drivers/media/v4l2-core/v4l2-fh.c
@@ -42,6 +42,7 @@ void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 	INIT_LIST_HEAD(&fh->available);
 	INIT_LIST_HEAD(&fh->subscribed);
 	fh->sequence = -1;
+	mutex_init(&fh->subscribe_lock);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_init);
 
@@ -88,6 +89,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 	if (fh->vdev == NULL)
 		return;
 	v4l2_event_unsubscribe_all(fh);
+	mutex_destroy(&fh->subscribe_lock);
 	fh->vdev = NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 803516775162d..a2777a324e083 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -41,6 +41,7 @@ struct v4l2_fh {
 
 	/* Events */
 	wait_queue_head_t	wait;
+	struct mutex		subscribe_lock;
 	struct list_head	subscribed; /* Subscribed events */
 	struct list_head	available; /* Dequeueable event */
 	unsigned int		navailable;
-- 
2.11.0

[PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Sakari Ailus]

[ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]

Patch ad608fbcf166 changed how events were subscribed to address an issue
elsewhere. As a side effect of that change, the "add" callback was called
before the event subscription was added to the list of subscribed events,
causing the first event queued by the add callback (and possibly other
events arriving soon afterwards) to be lost.

Fix this by adding the subscription to the list before calling the "add"
callback, and clean up afterwards if that fails.

Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")

Reported-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Tested-by: Dave Stevenson <dave.stevenson@raspberrypi.org>
Reviewed-by: Hans Verkuil <hans.verkuil@cisco.com>
Tested-by: Hans Verkuil <hans.verkuil@cisco.com>
Cc: stable@vger.kernel.org (for 4.14 and up)
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/v4l2-core/v4l2-event.c | 43 ++++++++++++++++++++----------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
index fcf65f131a2ac..35901f5b3971b 100644
--- a/drivers/media/v4l2-core/v4l2-event.c
+++ b/drivers/media/v4l2-core/v4l2-event.c
@@ -194,6 +194,22 @@ int v4l2_event_pending(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
+static void __v4l2_event_unsubscribe(struct v4l2_subscribed_event *sev)
+{
+	struct v4l2_fh *fh = sev->fh;
+	unsigned int i;
+
+	lockdep_assert_held(&fh->subscribe_lock);
+	assert_spin_locked(&fh->vdev->fh_lock);
+
+	/* Remove any pending events for this subscription */
+	for (i = 0; i < sev->in_use; i++) {
+		list_del(&sev->events[sev_pos(sev, i)].list);
+		fh->navailable--;
+	}
+	list_del(&sev->list);
+}
+
 int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 const struct v4l2_event_subscription *sub, unsigned elems,
 			 const struct v4l2_subscribed_event_ops *ops)
@@ -225,27 +241,23 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
+	if (!found_ev)
+		list_add(&sev->list, &fh->subscribed);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
 	if (found_ev) {
 		/* Already listening */
 		kfree(sev);
-		goto out_unlock;
-	}
-
-	if (sev->ops && sev->ops->add) {
+	} else if (sev->ops && sev->ops->add) {
 		ret = sev->ops->add(sev, elems);
 		if (ret) {
+			spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+			__v4l2_event_unsubscribe(sev);
+			spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 			kfree(sev);
-			goto out_unlock;
 		}
 	}
 
-	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-	list_add(&sev->list, &fh->subscribed);
-	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
-out_unlock:
 	mutex_unlock(&fh->subscribe_lock);
 
 	return ret;
@@ -280,7 +292,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 {
 	struct v4l2_subscribed_event *sev;
 	unsigned long flags;
-	int i;
 
 	if (sub->type == V4L2_EVENT_ALL) {
 		v4l2_event_unsubscribe_all(fh);
@@ -292,14 +303,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
-	if (sev != NULL) {
-		/* Remove any pending events for this subscription */
-		for (i = 0; i < sev->in_use; i++) {
-			list_del(&sev->events[sev_pos(sev, i)].list);
-			fh->navailable--;
-		}
-		list_del(&sev->list);
-	}
+	if (sev != NULL)
+		__v4l2_event_unsubscribe(sev);
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
-- 
2.11.0

Re: [PATCH v3.16 0/2] V4L2 event subscription fixes

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1037 bytes --]

On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
> Hi Ben,
> 
> The two patches fix a use-after-free issue in V4L2 event handling. The
> first patch that fixes that issue is already in the other stable trees (as
> well as Linus's tree) whereas the second that fixes a bug in the first
> one, is in the media tree only as of yet.

Thanks.  I'll apply the first now and hold the second until the
corresponding commit gets into Linus's tree.

Ben.

> <URL:https://git.linuxtv.org/media_tree.git/commit/?id=92539d3eda2c090b382699bbb896d4b54e9bdece>
> 
> Sakari Ailus (2):
>   v4l: event: Prevent freeing event subscriptions while accessed
>   v4l: event: Add subscription to list before calling "add" operation
> 
>  drivers/media/v4l2-core/v4l2-event.c | 63 ++++++++++++++++++++----------------
>  drivers/media/v4l2-core/v4l2-fh.c    |  2 ++
>  include/media/v4l2-fh.h              |  1 +
>  3 files changed, 38 insertions(+), 28 deletions(-)
> 
-- 
Ben Hutchings
Knowledge is power.  France is bacon.



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3.16 1/2] v4l: event: Prevent freeing event subscriptions while accessed

[Greg KH]

On Thu, Nov 08, 2018 at 02:03:49PM +0200, Sakari Ailus wrote:
> [ upstream commit ad608fbcf166fec809e402d548761768f602702c ]

This is already in 3.18.124.

thanks,

greg k-h

Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Greg KH]

On Thu, Nov 08, 2018 at 02:03:50PM +0200, Sakari Ailus wrote:
> [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> 
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event queued by the add callback (and possibly other
> events arriving soon afterwards) to be lost.
> 
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
> 
> Fixes: ad608fbcf166 ("media: v4l: event: Prevent freeing event subscriptions while accessed")

Now applied, thanks.

greg k-h

Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 773 bytes --]

On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
> [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> 
> Patch ad608fbcf166 changed how events were subscribed to address an issue
> elsewhere. As a side effect of that change, the "add" callback was called
> before the event subscription was added to the list of subscribed events,
> causing the first event queued by the add callback (and possibly other
> events arriving soon afterwards) to be lost.
> 
> Fix this by adding the subscription to the list before calling the "add"
> callback, and clean up afterwards if that fails.
[...]

I've queued this up for the next update, thanks.

Ben.

-- 
Ben Hutchings
Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Yi Qingliang]

hello, I sent a email about 'can't wake problem' 4 days ago.

Is this problem related with mine?

> epoll and vb2_poll: can't wake_up

> Sun, Dec 30, 2018, 6:17 PM (4 days ago)
> to linux-kernel
> Hello, I encountered a "can't wake_up" problem when use camera on imx6.
>
> if delay some time after 'streamon' the /dev/video0, then add fd
> through epoll_ctl, then the process can't be waken_up after some time.
>
> I checked both the epoll / vb2_poll(videobuf2_core.c) code.
>
> epoll will pass 'poll_table' structure to vb2_poll, but it only
> contain valid function pointer when inserting fd.
>
> in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
> after that, every call to vb2_poll will not contain valid poll_table,
> which will result in all calling to poll_wait will not work.
>
> so if app can process frames quickly, and found frame data when
> inserting fd (i.e. poll_wait will not be called or not contain valid
> function pointer), it will not found valid frame in 'vb2_poll' finally
> at some time, then call 'poll_wait' to expect be waken up at following
> vb2_buffer_done, but no good luck.
>
> I also checked the 'videobuf-core.c', there is no this problem.
>
> of course, both epoll and vb2_poll are right by itself side, but the
> result is we can't get new frames.
>
> I think by epoll's implementation, the user should always call poll_wait.
>
> and it's better to split the two actions: 'wait' and 'poll' both for
> epoll framework and all epoll users, for example, v4l2.
>
> am I right?

On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings <ben@decadent.org.uk> wrote:
>
> On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
> > [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> >
> > Patch ad608fbcf166 changed how events were subscribed to address an issue
> > elsewhere. As a side effect of that change, the "add" callback was called
> > before the event subscription was added to the list of subscribed events,
> > causing the first event queued by the add callback (and possibly other
> > events arriving soon afterwards) to be lost.
> >
> > Fix this by adding the subscription to the list before calling the "add"
> > callback, and clean up afterwards if that fails.
> [...]
>
> I've queued this up for the next update, thanks.
>
> Ben.
>
> --
> Ben Hutchings
> Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
>
>

Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Hans Verkuil]

On 01/03/2019 01:58 AM, Yi Qingliang wrote:
> hello, I sent a email about 'can't wake problem' 4 days ago.
> 
> Is this problem related with mine?

No, it's unrelated.

I'll take a look at vb2_poll next week.

Regards,

	Hans

> 
>> epoll and vb2_poll: can't wake_up
> 
>> Sun, Dec 30, 2018, 6:17 PM (4 days ago)
>> to linux-kernel
>> Hello, I encountered a "can't wake_up" problem when use camera on imx6.
>>
>> if delay some time after 'streamon' the /dev/video0, then add fd
>> through epoll_ctl, then the process can't be waken_up after some time.
>>
>> I checked both the epoll / vb2_poll(videobuf2_core.c) code.
>>
>> epoll will pass 'poll_table' structure to vb2_poll, but it only
>> contain valid function pointer when inserting fd.
>>
>> in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
>> after that, every call to vb2_poll will not contain valid poll_table,
>> which will result in all calling to poll_wait will not work.
>>
>> so if app can process frames quickly, and found frame data when
>> inserting fd (i.e. poll_wait will not be called or not contain valid
>> function pointer), it will not found valid frame in 'vb2_poll' finally
>> at some time, then call 'poll_wait' to expect be waken up at following
>> vb2_buffer_done, but no good luck.
>>
>> I also checked the 'videobuf-core.c', there is no this problem.
>>
>> of course, both epoll and vb2_poll are right by itself side, but the
>> result is we can't get new frames.
>>
>> I think by epoll's implementation, the user should always call poll_wait.
>>
>> and it's better to split the two actions: 'wait' and 'poll' both for
>> epoll framework and all epoll users, for example, v4l2.
>>
>> am I right?
> 
> On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings <ben@decadent.org.uk> wrote:
>>
>> On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
>>> [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
>>>
>>> Patch ad608fbcf166 changed how events were subscribed to address an issue
>>> elsewhere. As a side effect of that change, the "add" callback was called
>>> before the event subscription was added to the list of subscribed events,
>>> causing the first event queued by the add callback (and possibly other
>>> events arriving soon afterwards) to be lost.
>>>
>>> Fix this by adding the subscription to the list before calling the "add"
>>> callback, and clean up afterwards if that fails.
>> [...]
>>
>> I've queued this up for the next update, thanks.
>>
>> Ben.
>>
>> --
>> Ben Hutchings
>> Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
>>
>>

Re: [PATCH v3.16 2/2] v4l: event: Add subscription to list before calling "add" operation

[Yi Qingliang]

Ok, thanks a lot.

On Thu, Jan 3, 2019 at 6:15 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 01/03/2019 01:58 AM, Yi Qingliang wrote:
> > hello, I sent a email about 'can't wake problem' 4 days ago.
> >
> > Is this problem related with mine?
>
> No, it's unrelated.
>
> I'll take a look at vb2_poll next week.
>
> Regards,
>
>         Hans
>
> >
> >> epoll and vb2_poll: can't wake_up
> >
> >> Sun, Dec 30, 2018, 6:17 PM (4 days ago)
> >> to linux-kernel
> >> Hello, I encountered a "can't wake_up" problem when use camera on imx6.
> >>
> >> if delay some time after 'streamon' the /dev/video0, then add fd
> >> through epoll_ctl, then the process can't be waken_up after some time.
> >>
> >> I checked both the epoll / vb2_poll(videobuf2_core.c) code.
> >>
> >> epoll will pass 'poll_table' structure to vb2_poll, but it only
> >> contain valid function pointer when inserting fd.
> >>
> >> in vb2_poll, if found new data in done list, it will not call 'poll_wait'.
> >> after that, every call to vb2_poll will not contain valid poll_table,
> >> which will result in all calling to poll_wait will not work.
> >>
> >> so if app can process frames quickly, and found frame data when
> >> inserting fd (i.e. poll_wait will not be called or not contain valid
> >> function pointer), it will not found valid frame in 'vb2_poll' finally
> >> at some time, then call 'poll_wait' to expect be waken up at following
> >> vb2_buffer_done, but no good luck.
> >>
> >> I also checked the 'videobuf-core.c', there is no this problem.
> >>
> >> of course, both epoll and vb2_poll are right by itself side, but the
> >> result is we can't get new frames.
> >>
> >> I think by epoll's implementation, the user should always call poll_wait.
> >>
> >> and it's better to split the two actions: 'wait' and 'poll' both for
> >> epoll framework and all epoll users, for example, v4l2.
> >>
> >> am I right?
> >
> > On Thu, Jan 3, 2019 at 4:17 AM Ben Hutchings <ben@decadent.org.uk> wrote:
> >>
> >> On Thu, 2018-11-08 at 14:03 +0200, Sakari Ailus wrote:
> >>> [ upstream commit 92539d3eda2c090b382699bbb896d4b54e9bdece ]
> >>>
> >>> Patch ad608fbcf166 changed how events were subscribed to address an issue
> >>> elsewhere. As a side effect of that change, the "add" callback was called
> >>> before the event subscription was added to the list of subscribed events,
> >>> causing the first event queued by the add callback (and possibly other
> >>> events arriving soon afterwards) to be lost.
> >>>
> >>> Fix this by adding the subscription to the list before calling the "add"
> >>> callback, and clean up afterwards if that fails.
> >> [...]
> >>
> >> I've queued this up for the next update, thanks.
> >>
> >> Ben.
> >>
> >> --
> >> Ben Hutchings
> >> Absolutum obsoletum. (If it works, it's out of date.) - Stafford Beer
> >>
> >>
>