* [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference @ 2012-09-22 7:28 Sachin Kamat 2012-09-25 21:58 ` Sylwester Nawrocki 0 siblings, 1 reply; 3+ messages in thread From: Sachin Kamat @ 2012-09-22 7:28 UTC (permalink / raw) To: linux-media; +Cc: s.nawrocki, mchehab, sachin.kamat, patches It is previously assumed that 'rect' could be NULL. Hence add a check to print the members of 'rect' only when it is not NULL. Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org> --- drivers/media/i2c/s5k6aa.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c index 045ca7f..7531edb 100644 --- a/drivers/media/i2c/s5k6aa.c +++ b/drivers/media/i2c/s5k6aa.c @@ -1177,8 +1177,9 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, mutex_unlock(&s5k6aa->lock); - v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", - rect->left, rect->top, rect->width, rect->height); + if (rect) + v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", + rect->left, rect->top, rect->width, rect->height); return 0; } -- 1.7.4.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference 2012-09-22 7:28 [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference Sachin Kamat @ 2012-09-25 21:58 ` Sylwester Nawrocki 2012-09-26 3:28 ` Sachin Kamat 0 siblings, 1 reply; 3+ messages in thread From: Sylwester Nawrocki @ 2012-09-25 21:58 UTC (permalink / raw) To: Sachin Kamat; +Cc: linux-media, s.nawrocki, mchehab, patches Hi Sachin, On 09/22/2012 09:28 AM, Sachin Kamat wrote: > It is previously assumed that 'rect' could be NULL. > Hence add a check to print the members of 'rect' only when it is not > NULL. > > Signed-off-by: Sachin Kamat<sachin.kamat@linaro.org> > --- > drivers/media/i2c/s5k6aa.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c > index 045ca7f..7531edb 100644 > --- a/drivers/media/i2c/s5k6aa.c > +++ b/drivers/media/i2c/s5k6aa.c > @@ -1177,8 +1177,9 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, > > mutex_unlock(&s5k6aa->lock); > > - v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", > - rect->left, rect->top, rect->width, rect->height); > + if (rect) > + v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", > + rect->left, rect->top, rect->width, rect->height); > > return 0; > } Thank you for the patch. I would attack this problem form slightly different angle though, i.e. I would make sure __s5k6aa_get_crop_rect() always returns valid pointer. There is similar issue in s5k6aa_set_crop(). Since crop->which is already validated in drivers/media/v4l2-core/v4l2-subdev.c and can have only values: V4L2_SUBDEV_FORMAT_TRY, V4L2_SUBDEV_FORMAT_ACTIVE it's safe to do something like: 8<------------------------------------------------------------------------- >From 724aa5f1fefcaca2dee4f75ba960a1f620400e1a Mon Sep 17 00:00:00 2001 From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> Date: Tue, 25 Sep 2012 23:53:27 +0200 Subject: [PATCH] s5k6aa: Fix potential null pointer dereference Make sure __s5k6aa_get_crop_rect() always returns valid pointer, as it is assumed at the callers. crop->which is already validated when subdev set_crop and get_crop callbacks are called from within the v4l2-core. If it ever happens the crop operations are called directly for some reason in kernel space, with incorrect crop->which argument, just log it with WARN and return reference to the TRY crop. Reported-by: Sachin Kamat <sachin.kamat@linaro.org> Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> --- drivers/media/i2c/s5k6aa.c | 11 ++++------- 1 files changed, 4 insertions(+), 7 deletions(-) diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c index 045ca7f..57cd4fa 100644 --- a/drivers/media/i2c/s5k6aa.c +++ b/drivers/media/i2c/s5k6aa.c @@ -1061,10 +1061,9 @@ __s5k6aa_get_crop_rect(struct s5k6aa *s5k6aa, struct v4l2_subdev_fh *fh, { if (which == V4L2_SUBDEV_FORMAT_ACTIVE) return &s5k6aa->ccd_rect; - if (which == V4L2_SUBDEV_FORMAT_TRY) - return v4l2_subdev_get_try_crop(fh, 0); - return NULL; + WARN_ON(which != V4L2_SUBDEV_FORMAT_TRY); + return v4l2_subdev_get_try_crop(fh, 0); } static void s5k6aa_try_format(struct s5k6aa *s5k6aa, @@ -1169,12 +1168,10 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, struct v4l2_rect *rect; memset(crop->reserved, 0, sizeof(crop->reserved)); - mutex_lock(&s5k6aa->lock); + mutex_lock(&s5k6aa->lock); rect = __s5k6aa_get_crop_rect(s5k6aa, fh, crop->which); - if (rect) - crop->rect = *rect; - + crop->rect = *rect; mutex_unlock(&s5k6aa->lock); v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", -- 1.7.4.1 8<------------------------------------------------------------------------- I'm going to queue this patch for 3.7. -- Thanks, Sylwester ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference 2012-09-25 21:58 ` Sylwester Nawrocki @ 2012-09-26 3:28 ` Sachin Kamat 0 siblings, 0 replies; 3+ messages in thread From: Sachin Kamat @ 2012-09-26 3:28 UTC (permalink / raw) To: Sylwester Nawrocki; +Cc: linux-media, s.nawrocki, mchehab, patches Hi Sylwester, On 26 September 2012 03:28, Sylwester Nawrocki <sylvester.nawrocki@gmail.com> wrote: > Hi Sachin, > > On 09/22/2012 09:28 AM, Sachin Kamat wrote: >> It is previously assumed that 'rect' could be NULL. >> Hence add a check to print the members of 'rect' only when it is not >> NULL. >> >> Signed-off-by: Sachin Kamat<sachin.kamat@linaro.org> >> --- >> drivers/media/i2c/s5k6aa.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c >> index 045ca7f..7531edb 100644 >> --- a/drivers/media/i2c/s5k6aa.c >> +++ b/drivers/media/i2c/s5k6aa.c >> @@ -1177,8 +1177,9 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, >> >> mutex_unlock(&s5k6aa->lock); >> >> - v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", >> - rect->left, rect->top, rect->width, rect->height); >> + if (rect) >> + v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", >> + rect->left, rect->top, rect->width, rect->height); >> >> return 0; >> } > > Thank you for the patch. I would attack this problem form slightly > different angle though, i.e. I would make sure __s5k6aa_get_crop_rect() > always returns valid pointer. There is similar issue in s5k6aa_set_crop(). > Since crop->which is already validated in > drivers/media/v4l2-core/v4l2-subdev.c and can have only values: > V4L2_SUBDEV_FORMAT_TRY, V4L2_SUBDEV_FORMAT_ACTIVE it's safe to do > something like: Looks good to me. > > 8<------------------------------------------------------------------------- > From 724aa5f1fefcaca2dee4f75ba960a1f620400e1a Mon Sep 17 00:00:00 2001 > From: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> > Date: Tue, 25 Sep 2012 23:53:27 +0200 > Subject: [PATCH] s5k6aa: Fix potential null pointer dereference > > Make sure __s5k6aa_get_crop_rect() always returns valid pointer, > as it is assumed at the callers. > > crop->which is already validated when subdev set_crop and get_crop > callbacks are called from within the v4l2-core. If it ever happens > the crop operations are called directly for some reason in kernel > space, with incorrect crop->which argument, just log it with WARN > and return reference to the TRY crop. > > Reported-by: Sachin Kamat <sachin.kamat@linaro.org> > Signed-off-by: Sylwester Nawrocki <sylvester.nawrocki@gmail.com> Acked-by: Sachin Kamat <sachin.kamat@linaro.org> > --- > drivers/media/i2c/s5k6aa.c | 11 ++++------- > 1 files changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/s5k6aa.c b/drivers/media/i2c/s5k6aa.c > index 045ca7f..57cd4fa 100644 > --- a/drivers/media/i2c/s5k6aa.c > +++ b/drivers/media/i2c/s5k6aa.c > @@ -1061,10 +1061,9 @@ __s5k6aa_get_crop_rect(struct s5k6aa *s5k6aa, struct v4l2_subdev_fh *fh, > { > if (which == V4L2_SUBDEV_FORMAT_ACTIVE) > return &s5k6aa->ccd_rect; > - if (which == V4L2_SUBDEV_FORMAT_TRY) > - return v4l2_subdev_get_try_crop(fh, 0); > > - return NULL; > + WARN_ON(which != V4L2_SUBDEV_FORMAT_TRY); > + return v4l2_subdev_get_try_crop(fh, 0); > } > > static void s5k6aa_try_format(struct s5k6aa *s5k6aa, > @@ -1169,12 +1168,10 @@ static int s5k6aa_get_crop(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh, > struct v4l2_rect *rect; > > memset(crop->reserved, 0, sizeof(crop->reserved)); > - mutex_lock(&s5k6aa->lock); > > + mutex_lock(&s5k6aa->lock); > rect = __s5k6aa_get_crop_rect(s5k6aa, fh, crop->which); > - if (rect) > - crop->rect = *rect; > - > + crop->rect = *rect; > mutex_unlock(&s5k6aa->lock); > > v4l2_dbg(1, debug, sd, "Current crop rectangle: (%d,%d)/%dx%d\n", > -- > 1.7.4.1 > 8<------------------------------------------------------------------------- > > I'm going to queue this patch for 3.7. > > -- > > Thanks, > Sylwester -- With warm regards, Sachin ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2012-09-26 3:28 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-09-22 7:28 [PATCH] [media] s5k6aa: Fix possible NULL pointer dereference Sachin Kamat 2012-09-25 21:58 ` Sylwester Nawrocki 2012-09-26 3:28 ` Sachin Kamat
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).