Re: [PATCHv2 13/13] v4l2-compat-ioctl32.c: refactor, fix security bug in compat ioctl32

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 01/30/18 12:46, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the update. Please see a few additional comments below.
> 
> On Tue, Jan 30, 2018 at 11:27:01AM +0100, Hans Verkuil wrote:
> ...
>> @@ -891,30 +1057,53 @@ static long do_video_ioctl(struct file *file, unsigned int cmd, unsigned long ar
>>  	case VIDIOC_STREAMOFF:
>>  	case VIDIOC_S_INPUT:
>>  	case VIDIOC_S_OUTPUT:
>> -		err = get_user(karg.vi, (s32 __user *)up);
>> +		err = alloc_userspace(sizeof(unsigned int), 0, &up_native);
>> +		if (!err && assign_in_user((unsigned int __user *)up_native,
>> +					   (compat_uint_t __user *)up))
>> +			err = -EFAULT;
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_INPUT:
>>  	case VIDIOC_G_OUTPUT:
>> +		err = alloc_userspace(sizeof(unsigned int), 0,
>> +				      &up_native);
> 
> Fits on a single line.

Changed.

> 
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_EDID:
>>  	case VIDIOC_S_EDID:
>> -		err = get_v4l2_edid32(&karg.v2edid, up);
>> +		err = alloc_userspace(sizeof(struct v4l2_edid), 0, &up_native);
>> +		if (!err)
>> +			err = get_v4l2_edid32(up_native, up);
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_G_FMT:
>>  	case VIDIOC_S_FMT:
>>  	case VIDIOC_TRY_FMT:
>> -		err = get_v4l2_format32(&karg.v2f, up);
>> +		err = bufsize_v4l2_format(up, &aux_space);
>> +		if (!err)
>> +			err = alloc_userspace(sizeof(struct v4l2_format),
>> +					      aux_space, &up_native);
>> +		if (!err) {
>> +			aux_buf = up_native + sizeof(struct v4l2_format);
>> +			err = get_v4l2_format32(up_native, up,
>> +						aux_buf, aux_space);
>> +		}
>>  		compatible_arg = 0;
>>  		break;
>>  
>>  	case VIDIOC_CREATE_BUFS:
>> -		err = get_v4l2_create32(&karg.v2crt, up);
>> +		err = bufsize_v4l2_create(up, &aux_space);
>> +		if (!err)
>> +			err = alloc_userspace(sizeof(struct v4l2_create_buffers),
>> +					    aux_space, &up_native);
>> +		if (!err) {
>> +			aux_buf = up_native + sizeof(struct v4l2_create_buffers);
> 
> A few lines over 80 characters. It's not a lot but I see no reason to avoid
> wrapping them either.

I'm not changing this. It looks really ugly if I split it up.

> 
>> +			err = get_v4l2_create32(up_native, up,
>> +						aux_buf, aux_space);
>> +		}
>>  		compatible_arg = 0;
>>  		break;
>>  
> 
> The above can be addressed later, right now this isn't a priority.
> 
> Acked-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> 

Thanks!

	Hans



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]