[PATCH] media: staging: atomisp: fix a potential missing-check bug

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Dan Carpenter]

On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
> At the end of atomisp_subdev_set_selection(), the function
> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
> this function may return a NULL pointer, it is firstly invoked to check
> the returned pointer. If the returned pointer is not NULL, then the
> function is invoked again to obtain the pointer and the memory content
> at the location of the returned pointer is copied to the memory location of
> r. In most cases, the pointers returned by the two invocations are same.
> However, given that the pointer returned by the function
> atomisp_subdev_get_rect() is not a constant, it is possible that the two
> invocations return two different pointers. For example, another thread may
> race to modify the related pointers during the two invocations.

You're assuming a very serious race condition exists.


> In that
> case, even if the first returned pointer is not null, the second returned
> pointer might be null, which will cause issues such as null pointer
> dereference.

And then complaining that if a really serious bug exists then this very
minor bug would exist too...  If there were really a race condition like
that then we'd want to fix it instead.  In other words, this is not a
real life bug fix.

But it would be fine as a readability or static checker fix so that's
fine.

regards,
dan carpenter

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
>> At the end of atomisp_subdev_set_selection(), the function
>> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
>> this function may return a NULL pointer, it is firstly invoked to check
>> the returned pointer. If the returned pointer is not NULL, then the
>> function is invoked again to obtain the pointer and the memory content
>> at the location of the returned pointer is copied to the memory location of
>> r. In most cases, the pointers returned by the two invocations are same.
>> However, given that the pointer returned by the function
>> atomisp_subdev_get_rect() is not a constant, it is possible that the two
>> invocations return two different pointers. For example, another thread may
>> race to modify the related pointers during the two invocations.
>
> You're assuming a very serious race condition exists.
>
>
>> In that
>> case, even if the first returned pointer is not null, the second returned
>> pointer might be null, which will cause issues such as null pointer
>> dereference.
>
> And then complaining that if a really serious bug exists then this very
> minor bug would exist too...  If there were really a race condition like
> that then we'd want to fix it instead.  In other words, this is not a
> real life bug fix.
>
> But it would be fine as a readability or static checker fix so that's
> fine.

Thanks for your response. From the performance perspective, this bug
should also be fixed, as the second invocation is redundant if it is
expected to return a same pointer as the first one.

Wenwen
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
>> At the end of atomisp_subdev_set_selection(), the function
>> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
>> this function may return a NULL pointer, it is firstly invoked to check
>> the returned pointer. If the returned pointer is not NULL, then the
>> function is invoked again to obtain the pointer and the memory content
>> at the location of the returned pointer is copied to the memory location of
>> r. In most cases, the pointers returned by the two invocations are same.
>> However, given that the pointer returned by the function
>> atomisp_subdev_get_rect() is not a constant, it is possible that the two
>> invocations return two different pointers. For example, another thread may
>> race to modify the related pointers during the two invocations.
>
> You're assuming a very serious race condition exists.
>
>
>> In that
>> case, even if the first returned pointer is not null, the second returned
>> pointer might be null, which will cause issues such as null pointer
>> dereference.
>
> And then complaining that if a really serious bug exists then this very
> minor bug would exist too...  If there were really a race condition like
> that then we'd want to fix it instead.  In other words, this is not a
> real life bug fix.
>
> But it would be fine as a readability or static checker fix so that's
> fine.

Thanks for your response. From the performance perspective, this bug
should also be fixed, as the second invocation is redundant if it is
expected to return a same pointer as the first one.

Wenwen

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Sakari Ailus]

On Tue, May 08, 2018 at 08:04:54AM -0500, Wenwen Wang wrote:
> On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
> >> At the end of atomisp_subdev_set_selection(), the function
> >> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
> >> this function may return a NULL pointer, it is firstly invoked to check
> >> the returned pointer. If the returned pointer is not NULL, then the
> >> function is invoked again to obtain the pointer and the memory content
> >> at the location of the returned pointer is copied to the memory location of
> >> r. In most cases, the pointers returned by the two invocations are same.
> >> However, given that the pointer returned by the function
> >> atomisp_subdev_get_rect() is not a constant, it is possible that the two
> >> invocations return two different pointers. For example, another thread may
> >> race to modify the related pointers during the two invocations.
> >
> > You're assuming a very serious race condition exists.
> >
> >
> >> In that
> >> case, even if the first returned pointer is not null, the second returned
> >> pointer might be null, which will cause issues such as null pointer
> >> dereference.
> >
> > And then complaining that if a really serious bug exists then this very
> > minor bug would exist too...  If there were really a race condition like
> > that then we'd want to fix it instead.  In other words, this is not a
> > real life bug fix.
> >
> > But it would be fine as a readability or static checker fix so that's
> > fine.
> 
> Thanks for your response. From the performance perspective, this bug
> should also be fixed, as the second invocation is redundant if it is
> expected to return a same pointer as the first one.

The arguments are unchanged so the function returns the same pointer.

Btw. this driver is being removed; please see discussion here:

<URL:https://www.spinics.net/lists/linux-media/msg133223.html>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Sakari Ailus]

On Tue, May 08, 2018 at 08:04:54AM -0500, Wenwen Wang wrote:
> On Tue, May 8, 2018 at 7:16 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Wed, May 02, 2018 at 05:38:49PM -0500, Wenwen Wang wrote:
> >> At the end of atomisp_subdev_set_selection(), the function
> >> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
> >> this function may return a NULL pointer, it is firstly invoked to check
> >> the returned pointer. If the returned pointer is not NULL, then the
> >> function is invoked again to obtain the pointer and the memory content
> >> at the location of the returned pointer is copied to the memory location of
> >> r. In most cases, the pointers returned by the two invocations are same.
> >> However, given that the pointer returned by the function
> >> atomisp_subdev_get_rect() is not a constant, it is possible that the two
> >> invocations return two different pointers. For example, another thread may
> >> race to modify the related pointers during the two invocations.
> >
> > You're assuming a very serious race condition exists.
> >
> >
> >> In that
> >> case, even if the first returned pointer is not null, the second returned
> >> pointer might be null, which will cause issues such as null pointer
> >> dereference.
> >
> > And then complaining that if a really serious bug exists then this very
> > minor bug would exist too...  If there were really a race condition like
> > that then we'd want to fix it instead.  In other words, this is not a
> > real life bug fix.
> >
> > But it would be fine as a readability or static checker fix so that's
> > fine.
> 
> Thanks for your response. From the performance perspective, this bug
> should also be fixed, as the second invocation is redundant if it is
> expected to return a same pointer as the first one.

The arguments are unchanged so the function returns the same pointer.

Btw. this driver is being removed; please see discussion here:

<URL:https://www.spinics.net/lists/linux-media/msg133223.html>

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Andy Shevchenko]

On Fri, 2018-05-04 at 02:29 -0500, Wenwen Wang wrote:
> At the end of atomisp_subdev_set_selection(), the function
> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect.
> Since
> this function may return a NULL pointer, it is firstly invoked to
> check
> the returned pointer. If the returned pointer is not NULL, then the
> function is invoked again to obtain the pointer and the memory content
> at the location of the returned pointer is copied to the memory
> location of
> r. In most cases, the pointers returned by the two invocations are
> same.
> However, given that the pointer returned by the function
> atomisp_subdev_get_rect() is not a constant, it is possible that the
> two
> invocations return two different pointers. For example, another thread
> may
> race to modify the related pointers during the two invocations. In
> that
> case, even if the first returned pointer is not null, the second
> returned
> pointer might be null, which will cause issues such as null pointer
> dereference.
> 
> This patch saves the pointer returned by the first invocation and
> removes
> the second invocation. If the returned pointer is not NULL, the memory
> content is copied according to the original code.
> 

The driver will be gone soon, don't bother with it anymore.
Thanks!

> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++-
> -
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> index 49a9973..d5fa513 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> @@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>  	unsigned int i;
>  	unsigned int padding_w = pad_w;
>  	unsigned int padding_h = pad_h;
> +	struct v4l2_rect *p;
>  
>  	stream_id = atomisp_source_pad_to_stream_id(isp_sd,
> vdev_pad);
>  
> @@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>  		ffmt[pad]->height = comp[pad]->height;
>  	}
>  
> -	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
> +	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> +	if (!p)
>  		return -EINVAL;
> -	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> +	*r = *p;
>  
>  	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
>  		r->left, r->top, r->width, r->height);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH] media: staging: atomisp: fix a potential missing-check bug

[Andy Shevchenko]

On Fri, 2018-05-04 at 02:29 -0500, Wenwen Wang wrote:
> At the end of atomisp_subdev_set_selection(), the function
> atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect.
> Since
> this function may return a NULL pointer, it is firstly invoked to
> check
> the returned pointer. If the returned pointer is not NULL, then the
> function is invoked again to obtain the pointer and the memory content
> at the location of the returned pointer is copied to the memory
> location of
> r. In most cases, the pointers returned by the two invocations are
> same.
> However, given that the pointer returned by the function
> atomisp_subdev_get_rect() is not a constant, it is possible that the
> two
> invocations return two different pointers. For example, another thread
> may
> race to modify the related pointers during the two invocations. In
> that
> case, even if the first returned pointer is not null, the second
> returned
> pointer might be null, which will cause issues such as null pointer
> dereference.
> 
> This patch saves the pointer returned by the first invocation and
> removes
> the second invocation. If the returned pointer is not NULL, the memory
> content is copied according to the original code.
> 

The driver will be gone soon, don't bother with it anymore.
Thanks!

> Signed-off-by: Wenwen Wang <wang6495@umn.edu>
> ---
>  drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++-
> -
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git
> a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> index 49a9973..d5fa513 100644
> --- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> +++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
> @@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>  	unsigned int i;
>  	unsigned int padding_w = pad_w;
>  	unsigned int padding_h = pad_h;
> +	struct v4l2_rect *p;
>  
>  	stream_id = atomisp_source_pad_to_stream_id(isp_sd,
> vdev_pad);
>  
> @@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct
> v4l2_subdev *sd,
>  		ffmt[pad]->height = comp[pad]->height;
>  	}
>  
> -	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
> +	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> +	if (!p)
>  		return -EINVAL;
> -	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
> +	*r = *p;
>  
>  	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
>  		r->left, r->top, r->width, r->height);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4

_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

[PATCH] media: staging: atomisp: fix a potential missing-check bug

[Wenwen Wang]

At the end of atomisp_subdev_set_selection(), the function
atomisp_subdev_get_rect() is invoked to get the pointer to v4l2_rect. Since
this function may return a NULL pointer, it is firstly invoked to check
the returned pointer. If the returned pointer is not NULL, then the
function is invoked again to obtain the pointer and the memory content
at the location of the returned pointer is copied to the memory location of
r. In most cases, the pointers returned by the two invocations are same.
However, given that the pointer returned by the function
atomisp_subdev_get_rect() is not a constant, it is possible that the two
invocations return two different pointers. For example, another thread may
race to modify the related pointers during the two invocations. In that
case, even if the first returned pointer is not null, the second returned
pointer might be null, which will cause issues such as null pointer
dereference.

This patch saves the pointer returned by the first invocation and removes
the second invocation. If the returned pointer is not NULL, the memory
content is copied according to the original code.

Signed-off-by: Wenwen Wang <wang6495@umn.edu>
---
 drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
index 49a9973..d5fa513 100644
--- a/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
+++ b/drivers/staging/media/atomisp/pci/atomisp2/atomisp_subdev.c
@@ -366,6 +366,7 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 	unsigned int i;
 	unsigned int padding_w = pad_w;
 	unsigned int padding_h = pad_h;
+	struct v4l2_rect *p;
 
 	stream_id = atomisp_source_pad_to_stream_id(isp_sd, vdev_pad);
 
@@ -536,9 +537,10 @@ int atomisp_subdev_set_selection(struct v4l2_subdev *sd,
 		ffmt[pad]->height = comp[pad]->height;
 	}
 
-	if (!atomisp_subdev_get_rect(sd, cfg, which, pad, target))
+	p = atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	if (!p)
 		return -EINVAL;
-	*r = *atomisp_subdev_get_rect(sd, cfg, which, pad, target);
+	*r = *p;
 
 	dev_dbg(isp->dev, "sel actual: l %d t %d w %d h %d\n",
 		r->left, r->top, r->width, r->height);
-- 
2.7.4