Re: [PATCH 11/12] v4l2-compat-ioctl32.c: don't copy back the result for certain errors

[Hans Verkuil <hverkuil@xs4all.nl>]

On 01/29/2018 10:56 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Fri, Jan 26, 2018 at 01:43:26PM +0100, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Some ioctls need to copy back the result even if the ioctl returned
>> an error. However, don't do this for the error codes -ENOTTY, -EFAULT
>> and -ENOIOCTLCMD. It makes no sense in those cases.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Shouldn't such a change be made to video_usercopy() as well? Doesn't need
> to be in this set though.

Good point. I'll add that for v2. This is not actually a bug as such, but
it's just weird to copy back results if the ioctl wasn't implemented at all.

I realize that I need to drop the -EFAULT check: if you call VIDIOC_G_EXT_CTRLS
with an incorrect userspace buffer for the payload, then the control framework
will set error_idx to the index of the control with the wrong buffer. So you do
need to copy back the data in case of -EFAULT.

I can also drop -ENOIOCTLCMD since video_usercopy() converts that to -ENOTTY.

Regards,

	Hans

> 
>> ---
>>  drivers/media/v4l2-core/v4l2-compat-ioctl32.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> index 790473b45a21..2aa9b43daf60 100644
>> --- a/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> +++ b/drivers/media/v4l2-core/v4l2-compat-ioctl32.c
>> @@ -966,6 +966,9 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  		set_fs(old_fs);
>>  	}
>>  
>> +	if (err == -ENOTTY || err == -EFAULT || err == -ENOIOCTLCMD)
>> +		return err;
>> +
>>  	/* Special case: even after an error we need to put the
>>  	   results back for these ioctls since the error_idx will
>>  	   contain information on which control failed. */
>