[PATCH 4/14] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[PATCH 4/14] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Peter Senna Tschudin]

From: Peter Senna Tschudin <peter.senna@gmail.com>

Convert a nonnegative error return code to a negative one, as returned
elsewhere in the function.

A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)

// <smpl>
(
if@p1 (\(ret < 0\|ret != 0\))
 { ... return ret; }
|
ret@p1 = 0
)
... when != ret = e1
    when != &ret
*if(...)
{
  ... when != ret = e2
      when forall
 return ret;
}

// </smpl>

Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>

---
 drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
index 4da3df6..f6bc240 100644
--- a/drivers/media/v4l2-core/videobuf2-core.c
+++ b/drivers/media/v4l2-core/videobuf2-core.c
@@ -1876,8 +1876,10 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
 	 */
 	for (i = 0; i < q->num_buffers; i++) {
 		fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
-		if (fileio->bufs[i].vaddr = NULL)
+		if (fileio->bufs[i].vaddr = NULL) {
+			ret = -EFAULT;
 			goto err_reqbufs;
+		}
 		fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
 	}
 

Re: [PATCH 4/14] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Mauro Carvalho Chehab]

Em Thu,  6 Sep 2012 17:23:57 +0200
Peter Senna Tschudin <peter.senna@gmail.com> escreveu:

> From: Peter Senna Tschudin <peter.senna@gmail.com>
> 
> Convert a nonnegative error return code to a negative one, as returned
> elsewhere in the function.
> 
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
> 
> // <smpl>
> (
> if@p1 (\(ret < 0\|ret != 0\))
>  { ... return ret; }
> |
> ret@p1 = 0
> )
> ... when != ret = e1
>     when != &ret
> *if(...)
> {
>   ... when != ret = e2
>       when forall
>  return ret;
> }
> 
> // </smpl>
> 
> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
> 
> ---
>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 4da3df6..f6bc240 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
> @@ -1876,8 +1876,10 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>  	 */
>  	for (i = 0; i < q->num_buffers; i++) {
>  		fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
> -		if (fileio->bufs[i].vaddr = NULL)
> +		if (fileio->bufs[i].vaddr = NULL) {
> +			ret = -EFAULT;
>  			goto err_reqbufs;
> +		}

Had you test this patch? I suspect it breaks the driver, as there are failures under
streaming handling that are acceptable, as it may indicate that userspace was not
able to handle all queued frames in time. On such cases, what the Kernel does is to
just discard the frame. Userspace is able to detect it, by looking inside the timestamp
added on each frame.

>  		fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html




Cheers,
Mauro

Re: [PATCH 4/14] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Peter Senna Tschudin]

On Sat, Oct 6, 2012 at 1:17 PM, Mauro Carvalho Chehab
<mchehab@infradead.org> wrote:
> Em Thu,  6 Sep 2012 17:23:57 +0200
> Peter Senna Tschudin <peter.senna@gmail.com> escreveu:
>
>> From: Peter Senna Tschudin <peter.senna@gmail.com>
>>
>> Convert a nonnegative error return code to a negative one, as returned
>> elsewhere in the function.
>>
>> A simplified version of the semantic match that finds this problem is as
>> follows: (http://coccinelle.lip6.fr/)
>>
>> // <smpl>
>> (
>> if@p1 (\(ret < 0\|ret != 0\))
>>  { ... return ret; }
>> |
>> ret@p1 = 0
>> )
>> ... when != ret = e1
>>     when != &ret
>> *if(...)
>> {
>>   ... when != ret = e2
>>       when forall
>>  return ret;
>> }
>>
>> // </smpl>
>>
>> Signed-off-by: Peter Senna Tschudin <peter.senna@gmail.com>
>>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c |    4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>> index 4da3df6..f6bc240 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -1876,8 +1876,10 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>>        */
>>       for (i = 0; i < q->num_buffers; i++) {
>>               fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
>> -             if (fileio->bufs[i].vaddr = NULL)
>> +             if (fileio->bufs[i].vaddr = NULL) {
>> +                     ret = -EFAULT;
>>                       goto err_reqbufs;
>> +             }
>
> Had you test this patch? I suspect it breaks the driver, as there are failures under
> streaming handling that are acceptable, as it may indicate that userspace was not
> able to handle all queued frames in time. On such cases, what the Kernel does is to
> just discard the frame. Userspace is able to detect it, by looking inside the timestamp
> added on each frame.

No, I have not tested it. This was the only place the function was
returning non negative value for error path, so looked as a bug to me.
May I add a comment about returning non-negative value is intended
there?

>
>>               fileio->bufs[i].size = vb2_plane_size(q->bufs[i], 0);
>>       }
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-media" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>
>
>
> Cheers,
> Mauro



-- 
Peter

Re: [PATCH 4/14] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Sylwester Nawrocki]

Hi Peter,

On 10/10/2012 06:47 PM, Peter Senna Tschudin wrote:
>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
>>> index 4da3df6..f6bc240 100644
>>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>>> @@ -1876,8 +1876,10 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read)
>>>        */
>>>       for (i = 0; i < q->num_buffers; i++) {
>>>               fileio->bufs[i].vaddr = vb2_plane_vaddr(q->bufs[i], 0);
>>> -             if (fileio->bufs[i].vaddr = NULL)
>>> +             if (fileio->bufs[i].vaddr = NULL) {
>>> +                     ret = -EFAULT;
>>>                       goto err_reqbufs;
>>> +             }
>>
>> Had you test this patch? I suspect it breaks the driver, as there are failures under
>> streaming handling that are acceptable, as it may indicate that userspace was not
>> able to handle all queued frames in time. On such cases, what the Kernel does is to
>> just discard the frame. Userspace is able to detect it, by looking inside the timestamp
>> added on each frame.
> 
> No, I have not tested it. This was the only place the function was
> returning non negative value for error path, so looked as a bug to me.
> May I add a comment about returning non-negative value is intended
> there?

There are several drivers depending on core modules like videobuf2. By making
random changes for something that _looks like_ a bug to you and not verifying
it by testing with at least one driver you are potentially causing trouble to
developers that are already busy fixing real bugs or working on new features.

I appreciate your help but I also don't want to see _any_ untested, not trivial
patches for core modules like videobuf2 being applied.

--
Thanks,
Sylwester

Re: [PATCH V2] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Ezequiel Garcia]

On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
<peter.senna@gmail.com> wrote:
> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
> The NULL pointer deference happens at videobuf2-core.c:
>
> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>                 loff_t *ppos, int nonblock, int read)
> {
> ...
>         if (!q->fileio) {
>                 ret = __vb2_init_fileio(q, read);
>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>                 if (ret)
>                         return ret;
>         }
>         fileio = q->fileio; // NULL pointer deference here
> ...
> }
>
> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
> The OOPS happened when I've artificially forced the error by commenting the line:
>         if (fileio->bufs[i].vaddr = NULL)
>

... but if you manually changed the original source, how
can this be a real BUG?

Or am I missing something here ?

    Ezequiel

Re: [PATCH V2] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Peter Senna Tschudin]

On Thu, Oct 18, 2012 at 5:28 PM, Ezequiel Garcia <elezegarcia@gmail.com> wrote:
> On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
> <peter.senna@gmail.com> wrote:
>> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
>> The NULL pointer deference happens at videobuf2-core.c:
>>
>> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>>                 loff_t *ppos, int nonblock, int read)
>> {
>> ...
>>         if (!q->fileio) {
>>                 ret = __vb2_init_fileio(q, read);
>>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>>                 if (ret)
>>                         return ret;
>>         }
>>         fileio = q->fileio; // NULL pointer deference here
>> ...
>> }
>>
>> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
>> The OOPS happened when I've artificially forced the error by commenting the line:
>>         if (fileio->bufs[i].vaddr = NULL)
>>
>
> ... but if you manually changed the original source, how
> can this be a real BUG?

It is supposed that under some circumstances, (fileio->bufs[i].vaddr
= NULL) can be true. 'While testing', my change forced the scenario
in which (fileio->bufs[i].vaddr = NULL) is true...

>
> Or am I missing something here ?
>
>     Ezequiel



-- 
Peter

Re: [PATCH V2] drivers/media/v4l2-core/videobuf2-core.c: fix error return code

[Sylwester Nawrocki]

On 10/18/2012 05:28 PM, Ezequiel Garcia wrote:
> On Thu, Oct 18, 2012 at 11:47 AM, Peter Senna Tschudin
> <peter.senna@gmail.com> wrote:
>> This patch fixes a NULL pointer dereference bug at __vb2_init_fileio().
>> The NULL pointer deference happens at videobuf2-core.c:
>>
>> static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_t count,
>>                 loff_t *ppos, int nonblock, int read)
>> {
>> ...
>>         if (!q->fileio) {
>>                 ret = __vb2_init_fileio(q, read);
>>                 dprintk(3, "file io: vb2_init_fileio result: %d\n", ret);
>>                 if (ret)
>>                         return ret;
>>         }
>>         fileio = q->fileio; // NULL pointer deference here
>> ...
>> }
>>
>> It was tested with vivi driver and qv4l2 for selecting read() as capture method.
>> The OOPS happened when I've artificially forced the error by commenting the line:
>>         if (fileio->bufs[i].vaddr = NULL)
>>
> 
> ... but if you manually changed the original source, how
> can this be a real BUG?
> 
> Or am I missing something here ?

He just commented out this line to trigger the bug, i.e. to simulate
a situation where fileio->bufs[i].vaddr is NULL. Which is now not
handled properly.

--
Thanks,
Sylwester