[PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Thiago Farina]

This fix the following warning:
drivers/media/video/v4l2-compat-ioctl32.c: In function ‘get_microcode32’:
drivers/media/video/v4l2-compat-ioctl32.c:209: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index e30e8df..55825ec 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -206,7 +206,9 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
 	 * user address is invalid, the native ioctl will do
 	 * the error handling for us
 	 */
-	(void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
+	if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
+		return NULL;
+
 	(void) put_user(kp->datasize, &up->datasize);
 	(void) put_user(compat_ptr(kp->data), &up->data);
 	return up;
-- 
1.7.3.2.343.g7d43d

Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Arnd Bergmann]

On Tuesday 21 December 2010 02:18:06 Thiago Farina wrote:
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index e30e8df..55825ec 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -206,7 +206,9 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
>          * user address is invalid, the native ioctl will do
>          * the error handling for us
>          */
> -       (void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
> +       if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
> +               return NULL;
> +
>         (void) put_user(kp->datasize, &up->datasize);
>         (void) put_user(compat_ptr(kp->data), &up->data);
>         return up;

Did you read the comment above the code you changed?

You can probably change this function to look at the return code of
copy_to_user, but then you need to treat the put_user return code
the same, and change the comment.

	Arnd

Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Thiago Farina]

On Tue, Dec 21, 2010 at 4:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 21 December 2010 02:18:06 Thiago Farina wrote:
>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
>> index e30e8df..55825ec 100644
>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> @@ -206,7 +206,9 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
>>          * user address is invalid, the native ioctl will do
>>          * the error handling for us
>>          */
>> -       (void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
>> +       if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
>> +               return NULL;
>> +
>>         (void) put_user(kp->datasize, &up->datasize);
>>         (void) put_user(compat_ptr(kp->data), &up->data);
>>         return up;
>
> Did you read the comment above the code you changed?
>
Yes, I read, but I went ahead.

> You can probably change this function to look at the return code of
> copy_to_user, but then you need to treat the put_user return code
> the same, and change the comment.
>

Right, I will do the same with put_user, but I'm afraid of changing the comment.

Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Arnd Bergmann]

On Tuesday 21 December 2010 19:34:04 Thiago Farina wrote:
> > You can probably change this function to look at the return code of
> > copy_to_user, but then you need to treat the put_user return code
> > the same, and change the comment.
> >
> 
> Right, I will do the same with put_user, but I'm afraid of changing the comment.

The comment makes no sense when the code doesn't do what is explained
there.

	Arnd

[PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Thiago Farina]

This fix the following warning:
drivers/media/video/v4l2-compat-ioctl32.c: In function ‘get_microcode32’:
drivers/media/video/v4l2-compat-ioctl32.c:209: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result

Signed-off-by: Thiago Farina <tfransosi@gmail.com>
---
 Changes from v1:
 - Check the return code of put_user too.
 - Remove the obsolete comment.

 drivers/media/video/v4l2-compat-ioctl32.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index e30e8df..6f2a022 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -201,14 +201,12 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
 
 	up = compat_alloc_user_space(sizeof(*up));
 
-	/*
-	 * NOTE! We don't actually care if these fail. If the
-	 * user address is invalid, the native ioctl will do
-	 * the error handling for us
-	 */
-	(void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
-	(void) put_user(kp->datasize, &up->datasize);
-	(void) put_user(compat_ptr(kp->data), &up->data);
+	if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
+		return NULL;
+	if (put_user(kp->datasize, &up->datasize))
+		return NULL;
+	if (put_user(compat_ptr(kp->data), &up->data))
+		return NULL;
 	return up;
 }
 
-- 
1.7.3.2.343.g7d43d

Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Mauro Carvalho Chehab]

Em 21-12-2010 16:34, Thiago Farina escreveu:
> On Tue, Dec 21, 2010 at 4:25 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Tuesday 21 December 2010 02:18:06 Thiago Farina wrote:
>>> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
>>> index e30e8df..55825ec 100644
>>> --- a/drivers/media/video/v4l2-compat-ioctl32.c
>>> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>>> @@ -206,7 +206,9 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
>>>          * user address is invalid, the native ioctl will do
>>>          * the error handling for us
>>>          */
>>> -       (void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
>>> +       if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
>>> +               return NULL;
>>> +
>>>         (void) put_user(kp->datasize, &up->datasize);
>>>         (void) put_user(compat_ptr(kp->data), &up->data);
>>>         return up;
>>
>> Did you read the comment above the code you changed?
>>
> Yes, I read, but I went ahead.
> 
>> You can probably change this function to look at the return code of
>> copy_to_user, but then you need to treat the put_user return code
>> the same, and change the comment.
>>
> 
> Right, I will do the same with put_user, but I'm afraid of changing the comment.

Well, we should just remove all V4L1 stuff for .38, so I don't see much sense on keeping
the VIDIOCGMICROCODE32 compat stuff.

Cheers,
Mauro

Re: [PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Andreas Oberritter]

On 12/22/2010 12:48 AM, Thiago Farina wrote:
> This fix the following warning:
> drivers/media/video/v4l2-compat-ioctl32.c: In function ‘get_microcode32’:
> drivers/media/video/v4l2-compat-ioctl32.c:209: warning: ignoring return value of ‘copy_to_user’, declared with attribute warn_unused_result
> 
> Signed-off-by: Thiago Farina <tfransosi@gmail.com>
> ---
>  Changes from v1:
>  - Check the return code of put_user too.
>  - Remove the obsolete comment.
> 
>  drivers/media/video/v4l2-compat-ioctl32.c |   14 ++++++--------
>  1 files changed, 6 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index e30e8df..6f2a022 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -201,14 +201,12 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
>  
>  	up = compat_alloc_user_space(sizeof(*up));

I don't know anything about that code, but I assume that "up" should be
checked for NULL before use and should be freed in case an error occurs
below.

>  
> -	/*
> -	 * NOTE! We don't actually care if these fail. If the
> -	 * user address is invalid, the native ioctl will do
> -	 * the error handling for us
> -	 */
> -	(void) copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat));
> -	(void) put_user(kp->datasize, &up->datasize);
> -	(void) put_user(compat_ptr(kp->data), &up->data);
> +	if (copy_to_user(up->loadwhat, kp->loadwhat, sizeof(up->loadwhat)))
> +		return NULL;
> +	if (put_user(kp->datasize, &up->datasize))
> +		return NULL;
> +	if (put_user(compat_ptr(kp->data), &up->data))
> +		return NULL;
>  	return up;
>  }
>  

Re: [PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user

[Arnd Bergmann]

On Wednesday 22 December 2010 15:14:49 Andreas Oberritter wrote:
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> > index e30e8df..6f2a022 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -201,14 +201,12 @@ static struct video_code __user *get_microcode32(struct video_code32 *kp)
> >  
> >       up = compat_alloc_user_space(sizeof(*up));
> 
> I don't know anything about that code, but I assume that "up" should be
> checked for NULL before use and should be freed in case an error occurs
> below.
> 

No, that's fine. compat_alloc_user_space() is very special -- the allocated
memory is on the user stack and automatically gets freed when the kernel
returns to user space.

We treat the resulting pointer like any other user pointer, i.e. we only
ever access it using {get,put}_user and copy_{from,to}_user, which check
that it's pointing to real user memory. A null pointer here would only
mean that the user had an invalid stack pointer before entering the kernel,
but causes no more problems than passing NULL as the ioctl argument.

	Arnd