From b36b505965e374b284166c2e6b9c1d369d663ea9 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Mon, 24 Oct 2011 05:03:27 -0300 Subject: [media] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL Signed-off-by: Hans de Goede Acked-by: Laurent Pinchart Acked-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/v4l2-event.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'drivers/media/video/v4l2-event.c') diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c index 53b190cf225e..9f56f18d509f 100644 --- a/drivers/media/video/v4l2-event.c +++ b/drivers/media/video/v4l2-event.c @@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh, unsigned long flags; unsigned i; + if (sub->type == V4L2_EVENT_ALL) + return -EINVAL; + if (elems < 1) elems = 1; if (sub->type == V4L2_EVENT_CTRL) { -- cgit v1.2.3 From 78c87e863bb3350426fecd14912fd0a546c58ec0 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 26 Oct 2011 05:40:27 -0300 Subject: [media] v4l2-event: Remove pending events from fh event queue when unsubscribing The kev pointers inside the pending events queue (the available queue) of the fh point to data inside the sev, unsubscribing frees the sev, thus making these pointers point to freed memory! This patch fixes these dangling pointers in the available queue by removing all matching pending events on unsubscription. Signed-off-by: Hans de Goede Acked-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/v4l2-event.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'drivers/media/video/v4l2-event.c') diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c index 9f56f18d509f..4d01f17497f6 100644 --- a/drivers/media/video/v4l2-event.c +++ b/drivers/media/video/v4l2-event.c @@ -285,6 +285,7 @@ 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); @@ -295,6 +296,11 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, 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); sev->fh = NULL; } -- cgit v1.2.3 From e3e72f39b68ec2563d8ef22f9704a66b7f71b638 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 26 Oct 2011 05:52:47 -0300 Subject: [media] v4l2-event: Don't set sev->fh to NULL on unsubscribe Setting sev->fh to NULL causes problems for the del op added in the next patch of this series, since this op needs a way to get to its own data structures, and typically this will be done by using container_of on an embedded v4l2_fh struct. The reason the original code is setting sev->fh to NULL is to signal to users of the event framework that the unsubscription has happened, but since their is no shared lock between the event framework and users of it, this is inherently racy, and it also turns out to be unnecessary as long as both the event framework and the user of the framework do their own locking properly and the user guarantees that it holds no references to the subcribed_event structure after its del operation has been called. This is best explained by looking at the only code currently checking for sev->fh being set to NULL on unsubscribe, which is the v4l2-ctrls.c send_event function. Here is the relevant code from v4l2-ctrls: send_event(): if (sev->fh && (sev->fh != fh || (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK))) v4l2_event_queue_fh(sev->fh, &ev); Now lets say that v4l2_event_unsubscribe and v4l2-ctrls: send_event() race on the same sev, then the following could happens: 1) send_event checks sev->fh, finds it is not NULL 2) v4l2_event_unsubscribe sets sev->fh NULL 3) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks as the thread calling send_event holds the ctrl_lock 4) send_event calls v4l2_event_queue_fh(sev->fh, &ev) which not is equivalent to calling: v4l2_event_queue_fh(NULL, &ev) 5) oops, NULL pointer deref. Now again without setting sev->fh to NULL in v4l2_event_unsubscribe and without the (now senseless since always true) sev->fh != NULL check in 1) send_event is about to call v4l2_event_queue_fh(sev->fh, &ev) 2) v4l2_event_unsubscribe removes sev->list from the fh->subscribed list 3) send_event calls v4l2_event_queue_fh(sev->fh, &ev) 4) v4l2_event_queue_fh blocks on the fh_lock spinlock 5) v4l2_event_unsubscribe unlocks the fh_lock spinlock 6) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks as the thread calling send_event holds the ctrl_lock 8) v4l2_event_queue_fh takes the fh_lock 7) v4l2_event_queue_fh calls v4l2_event_subscribed, does not find it since sev->list has been removed from fh->subscribed already -> does nothing 9) v4l2_event_queue_fh releases the fh_lock 10) the caller of send_event releases the ctrl lock (mutex) 11) v4l2_ctrls del_event takes the ctrl lock 12) v4l2_ctrls del_event removes sev->node from the ev_subs list 13) v4l2_ctrls del_event releases the ctrl lock 14) v4l2_event_unsubscribe frees the sev, to which no references are being held anymore Signed-off-by: Hans de Goede Acked-by: Hans Verkuil Signed-off-by: Mauro Carvalho Chehab --- drivers/media/video/v4l2-ctrls.c | 4 ++-- drivers/media/video/v4l2-event.c | 1 - 2 files changed, 2 insertions(+), 3 deletions(-) (limited to 'drivers/media/video/v4l2-event.c') diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c index 5552f8137571..c58c91bbcfbd 100644 --- a/drivers/media/video/v4l2-ctrls.c +++ b/drivers/media/video/v4l2-ctrls.c @@ -820,8 +820,8 @@ static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes) fill_event(&ev, ctrl, changes); list_for_each_entry(sev, &ctrl->ev_subs, node) - if (sev->fh && (sev->fh != fh || - (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK))) + if (sev->fh != fh || + (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)) v4l2_event_queue_fh(sev->fh, &ev); } diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c index 4d01f17497f6..3d93251f292e 100644 --- a/drivers/media/video/v4l2-event.c +++ b/drivers/media/video/v4l2-event.c @@ -302,7 +302,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh, fh->navailable--; } list_del(&sev->list); - sev->fh = NULL; } spin_unlock_irqrestore(&fh->vdev->fh_lock, flags); -- cgit v1.2.3