| Message ID | 20180424234506.22630-1-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers |
Received: from vger.kernel.org ([209.132.180.67]) by www.linuxtv.org with esmtp (Exim 4.84_2) (envelope-from <linux-media-owner@vger.kernel.org>) id 1fB7d2-0008VN-Q3; Tue, 24 Apr 2018 23:45:45 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751195AbeDXXpm (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Tue, 24 Apr 2018 19:45:42 -0400 Received: from vsp-unauthed02.binero.net ([195.74.38.227]:59707 "EHLO vsp-unauthed02.binero.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750766AbeDXXpl (ORCPT <rfc822;linux-media@vger.kernel.org>); Tue, 24 Apr 2018 19:45:41 -0400 X-Halon-ID: 8e365633-4819-11e8-9bdf-005056917f90 Authorized-sender: niklas@soderlund.pp.se Received: from bismarck.berto.se (unknown [89.233.230.99]) by bin-vsp-out-02.atm.binero.net (Halon) with ESMTPA id 8e365633-4819-11e8-9bdf-005056917f90; Wed, 25 Apr 2018 01:45:33 +0200 (CEST) From: =?UTF-8?q?Niklas=20S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se> To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>, Hans Verkuil <hverkuil@xs4all.nl>, linux-media@vger.kernel.org Cc: linux-renesas-soc@vger.kernel.org, =?UTF-8?q?Niklas=20S=C3=B6derlund?= <niklas.soderlund+renesas@ragnatech.se> Subject: [PATCH] rcar-vin: fix null pointer dereference in rvin_group_get() Date: Wed, 25 Apr 2018 01:45:06 +0200 Message-Id: <20180424234506.22630-1-niklas.soderlund+renesas@ragnatech.se> X-Mailer: git-send-email 2.17.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Niklas Söderlund
April 24, 2018, 11:45 p.m. UTC
Store the group pointer before disassociating the VIN from the group.
Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
Comments
On Wed, Apr 25, 2018 at 01:45:06AM +0200, Niklas Söderlund wrote: > Store the group pointer before disassociating the VIN from the group. > > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions") > Reported-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 7bc2774a11232362..d3072e166a1ca24f 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) > > static void rvin_group_put(struct rvin_dev *vin) > { > - mutex_lock(&vin->group->lock); > + struct rvin_group *group = vin->group; > + > + mutex_lock(&group->lock); Hi Niklas, its not clear to me why moving the lock is safe. Could you explain the locking scheme a little? > > vin->group = NULL; > vin->v4l2_dev.mdev = NULL; > > - if (WARN_ON(vin->group->vin[vin->id] != vin)) > + if (WARN_ON(group->vin[vin->id] != vin)) > goto out; > > - vin->group->vin[vin->id] = NULL; > + group->vin[vin->id] = NULL; > out: > - mutex_unlock(&vin->group->lock); > + mutex_unlock(&group->lock); > > - kref_put(&vin->group->refcount, rvin_group_release); > + kref_put(&group->refcount, rvin_group_release); > } > > /* ----------------------------------------------------------------------------- > -- > 2.17.0 >
On Wed, Apr 25, 2018 at 1:45 AM, Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > Store the group pointer before disassociating the VIN from the group. s/get/put/ in one-line summary? > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions") > Reported-by: Colin Ian King <colin.king@canonical.com> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > --- > drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > index 7bc2774a11232362..d3072e166a1ca24f 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) > > static void rvin_group_put(struct rvin_dev *vin) > { > - mutex_lock(&vin->group->lock); > + struct rvin_group *group = vin->group; Gr{oetje,eeting}s, Geert
Hi Simon, Thanks for your feedback. On 2018-04-25 09:18:51 +0200, Simon Horman wrote: > On Wed, Apr 25, 2018 at 01:45:06AM +0200, Niklas Söderlund wrote: > > Store the group pointer before disassociating the VIN from the group. > > > > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions") > > Reported-by: Colin Ian King <colin.king@canonical.com> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index 7bc2774a11232362..d3072e166a1ca24f 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) > > > > static void rvin_group_put(struct rvin_dev *vin) > > { > > - mutex_lock(&vin->group->lock); > > + struct rvin_group *group = vin->group; > > + > > + mutex_lock(&group->lock); > > Hi Niklas, its not clear to me why moving the lock is safe. > Could you explain the locking scheme a little? The lock here protects the members of the group struct and not any of the members of the vin struct. The intent of the rvin_group_put() function is: 1. Disassociate the vin struct from the group struct. This is done by removing the pointer to the vin from the group->vin array and removing the pointer from vin->group to the group struct. Here the lock is needed to protect access to the group->vin array. 2. Decrease the refcount of the struct group and if we are the last one out release the group. The problem with the original code is that I first disassociate group from the vin 'vin->group = NULL' but still use the pointer stored in the vin struct when I try to disassociate the vin from the group 'vin->group->vin[vin->id]'. AFIK can tell the locking here is fine, the problem was that I pulled the rug from under my own feet in how I access the lock in order to not having to declare a variable to store the pointer in ;-) Do this explanation help put you at ease? > > > > > vin->group = NULL; > > vin->v4l2_dev.mdev = NULL; > > > > - if (WARN_ON(vin->group->vin[vin->id] != vin)) > > + if (WARN_ON(group->vin[vin->id] != vin)) > > goto out; > > > > - vin->group->vin[vin->id] = NULL; > > + group->vin[vin->id] = NULL; > > out: > > - mutex_unlock(&vin->group->lock); > > + mutex_unlock(&group->lock); > > > > - kref_put(&vin->group->refcount, rvin_group_release); > > + kref_put(&group->refcount, rvin_group_release); > > } > > > > /* ----------------------------------------------------------------------------- > > -- > > 2.17.0 > >
Hi Geert, Thanks for your feedback. On 2018-04-25 09:25:56 +0200, Geert Uytterhoeven wrote: > On Wed, Apr 25, 2018 at 1:45 AM, Niklas Söderlund > <niklas.soderlund+renesas@ragnatech.se> wrote: > > Store the group pointer before disassociating the VIN from the group. > > s/get/put/ in one-line summary? Yes, silly copy paste error, must have copied function name from the @@ context line and not from the diff itself. Thanks for noticing. Will send a v2 after I have checked with Simon that the is happy with the change itself. > > > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions") > > Reported-by: Colin Ian King <colin.king@canonical.com> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > > drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++----- > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > index 7bc2774a11232362..d3072e166a1ca24f 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) > > > > static void rvin_group_put(struct rvin_dev *vin) > > { > > - mutex_lock(&vin->group->lock); > > + struct rvin_group *group = vin->group; > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > In personal conversations with technical people, I call myself a hacker. But > when I'm talking to journalists I just say "programmer" or something like that. > -- Linus Torvalds
On Thu, Apr 26, 2018 at 05:20:05PM +0200, Niklas Söderlund wrote: > Hi Simon, > > Thanks for your feedback. > > On 2018-04-25 09:18:51 +0200, Simon Horman wrote: > > On Wed, Apr 25, 2018 at 01:45:06AM +0200, Niklas Söderlund wrote: > > > Store the group pointer before disassociating the VIN from the group. > > > > > > Fixes: 3bb4c3bc85bf77a7 ("media: rcar-vin: add group allocator functions") > > > Reported-by: Colin Ian King <colin.king@canonical.com> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > --- > > > drivers/media/platform/rcar-vin/rcar-core.c | 12 +++++++----- > > > 1 file changed, 7 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c > > > index 7bc2774a11232362..d3072e166a1ca24f 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) > > > > > > static void rvin_group_put(struct rvin_dev *vin) > > > { > > > - mutex_lock(&vin->group->lock); > > > + struct rvin_group *group = vin->group; > > > + > > > + mutex_lock(&group->lock); > > > > Hi Niklas, its not clear to me why moving the lock is safe. > > Could you explain the locking scheme a little? > > The lock here protects the members of the group struct and not any of > the members of the vin struct. The intent of the rvin_group_put() > function is: > > 1. Disassociate the vin struct from the group struct. This is done by > removing the pointer to the vin from the group->vin array and > removing the pointer from vin->group to the group struct. Here the > lock is needed to protect access to the group->vin array. > > 2. Decrease the refcount of the struct group and if we are the last one > out release the group. > > The problem with the original code is that I first disassociate group > from the vin 'vin->group = NULL' but still use the pointer stored in the > vin struct when I try to disassociate the vin from the group > 'vin->group->vin[vin->id]'. > > AFIK can tell the locking here is fine, the problem was that I pulled > the rug from under my own feet in how I access the lock in order to not > having to declare a variable to store the pointer in ;-) > > Do this explanation help put you at ease? Thanks, I am completely relaxed now :) Reviewed-by: Simon Horman <horms+renesas@verge.net.au> > > > vin->group = NULL; > > > vin->v4l2_dev.mdev = NULL; > > > > > > - if (WARN_ON(vin->group->vin[vin->id] != vin)) > > > + if (WARN_ON(group->vin[vin->id] != vin)) > > > goto out; > > > > > > - vin->group->vin[vin->id] = NULL; > > > + group->vin[vin->id] = NULL; > > > out: > > > - mutex_unlock(&vin->group->lock); > > > + mutex_unlock(&group->lock); > > > > > > - kref_put(&vin->group->refcount, rvin_group_release); > > > + kref_put(&group->refcount, rvin_group_release); > > > } > > > > > > /* ----------------------------------------------------------------------------- > > > -- > > > 2.17.0 > > > > > -- > Regards, > Niklas Söderlund >
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 7bc2774a11232362..d3072e166a1ca24f 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -338,19 +338,21 @@ static int rvin_group_get(struct rvin_dev *vin) static void rvin_group_put(struct rvin_dev *vin) { - mutex_lock(&vin->group->lock); + struct rvin_group *group = vin->group; + + mutex_lock(&group->lock); vin->group = NULL; vin->v4l2_dev.mdev = NULL; - if (WARN_ON(vin->group->vin[vin->id] != vin)) + if (WARN_ON(group->vin[vin->id] != vin)) goto out; - vin->group->vin[vin->id] = NULL; + group->vin[vin->id] = NULL; out: - mutex_unlock(&vin->group->lock); + mutex_unlock(&group->lock); - kref_put(&vin->group->refcount, rvin_group_release); + kref_put(&group->refcount, rvin_group_release); } /* -----------------------------------------------------------------------------