* [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).