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 <[email protected]>
---
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
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
On Tue, Dec 21, 2010 at 4:25 PM, Arnd Bergmann <[email protected]> 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.
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
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 <[email protected]>
---
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
Em 21-12-2010 16:34, Thiago Farina escreveu:
> On Tue, Dec 21, 2010 at 4:25 PM, Arnd Bergmann <[email protected]> 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
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 <[email protected]>
> ---
> 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;
> }
>
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