* [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
@ 2010-12-21 1:18 Thiago Farina
2010-12-21 18:25 ` Arnd Bergmann
0 siblings, 1 reply; 8+ messages in thread
From: Thiago Farina @ 2010-12-21 1:18 UTC (permalink / raw)
To: linux-kernel
Cc: Thiago Farina, Guennadi Liakhovetski, Mauro Carvalho Chehab,
linux-media
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 1:18 [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user Thiago Farina
@ 2010-12-21 18:25 ` Arnd Bergmann
2010-12-21 18:34 ` Thiago Farina
0 siblings, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2010-12-21 18:25 UTC (permalink / raw)
To: Thiago Farina
Cc: linux-kernel, Guennadi Liakhovetski, Mauro Carvalho Chehab,
linux-media
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 18:25 ` Arnd Bergmann
@ 2010-12-21 18:34 ` Thiago Farina
2010-12-21 19:03 ` Arnd Bergmann
2010-12-21 23:55 ` [PATCH] " Mauro Carvalho Chehab
0 siblings, 2 replies; 8+ messages in thread
From: Thiago Farina @ 2010-12-21 18:34 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-kernel, Guennadi Liakhovetski, Mauro Carvalho Chehab,
linux-media
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 18:34 ` Thiago Farina
@ 2010-12-21 19:03 ` Arnd Bergmann
2010-12-21 23:48 ` [PATCH v2] " Thiago Farina
2010-12-21 23:55 ` [PATCH] " Mauro Carvalho Chehab
1 sibling, 1 reply; 8+ messages in thread
From: Arnd Bergmann @ 2010-12-21 19:03 UTC (permalink / raw)
To: Thiago Farina
Cc: linux-kernel, Guennadi Liakhovetski, Mauro Carvalho Chehab,
linux-media
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 19:03 ` Arnd Bergmann
@ 2010-12-21 23:48 ` Thiago Farina
2010-12-22 14:14 ` Andreas Oberritter
0 siblings, 1 reply; 8+ messages in thread
From: Thiago Farina @ 2010-12-21 23:48 UTC (permalink / raw)
To: linux-kernel
Cc: arnd, Guennadi Liakhovetski, Mauro Carvalho Chehab, linux-media
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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 23:48 ` [PATCH v2] " Thiago Farina
@ 2010-12-22 14:14 ` Andreas Oberritter
2010-12-22 15:50 ` Arnd Bergmann
0 siblings, 1 reply; 8+ messages in thread
From: Andreas Oberritter @ 2010-12-22 14:14 UTC (permalink / raw)
To: Thiago Farina
Cc: linux-kernel, arnd, Guennadi Liakhovetski, Mauro Carvalho Chehab,
linux-media
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;
> }
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-22 14:14 ` Andreas Oberritter
@ 2010-12-22 15:50 ` Arnd Bergmann
0 siblings, 0 replies; 8+ messages in thread
From: Arnd Bergmann @ 2010-12-22 15:50 UTC (permalink / raw)
To: Andreas Oberritter
Cc: Thiago Farina, linux-kernel, Guennadi Liakhovetski,
Mauro Carvalho Chehab, linux-media
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user
2010-12-21 18:34 ` Thiago Farina
2010-12-21 19:03 ` Arnd Bergmann
@ 2010-12-21 23:55 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 8+ messages in thread
From: Mauro Carvalho Chehab @ 2010-12-21 23:55 UTC (permalink / raw)
To: Thiago Farina
Cc: Arnd Bergmann, linux-kernel, Guennadi Liakhovetski, linux-media
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2010-12-22 15:50 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-12-21 1:18 [PATCH] drivers/media/video/v4l2-compat-ioctl32.c: Check the return value of copy_to_user Thiago Farina
2010-12-21 18:25 ` Arnd Bergmann
2010-12-21 18:34 ` Thiago Farina
2010-12-21 19:03 ` Arnd Bergmann
2010-12-21 23:48 ` [PATCH v2] " Thiago Farina
2010-12-22 14:14 ` Andreas Oberritter
2010-12-22 15:50 ` Arnd Bergmann
2010-12-21 23:55 ` [PATCH] " Mauro Carvalho Chehab
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox