[13/20] media: soc_camera: v4l2-compliance fixes for querycap

  Fill in bus_info field and zero reserved field.

Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
Reviewed-by: William Towle <william.towle@codethink.co.uk>
---
 drivers/media/platform/soc_camera/soc_camera.c |    2 ++
 1 file changed, 2 insertions(+)
  

On 21/05/15 06:58, Hans Verkuil wrote:
> On 05/20/2015 06:39 PM, William Towle wrote:
>> Fill in bus_info field and zero reserved field.
>>
>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>> ---
>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>> index fd7497e..583c5e6 100644
>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>  	WARN_ON(priv != file->private_data);
>>  
>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
> 
> Why the memset? That shouldn't be needed.

v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
v4l-utils v1.6.2 [1])

Thanks,
Rob

[1]
http://git.linuxtv.org/cgit.cgi/v4l-utils.git/tree/utils/v4l2-compliance/v4l2-compliance.cpp?id=v4l-utils-1.6.2#n308
> Regards,
> 
> 	Hans
> 
>>  	return ici->ops->querycap(ici, cap);
>>  }
>>  
>>
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi William,

On Wed, 20 May 2015, William Towle wrote:

> Fill in bus_info field and zero reserved field.
> 
> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>

If you're the author and the submitter of this patch, why is Rob's Sob 
here needed? Or is he the author?

Thanks
Guennadi

> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> ---
>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> index fd7497e..583c5e6 100644
> --- a/drivers/media/platform/soc_camera/soc_camera.c
> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>  	WARN_ON(priv != file->private_data);
>  
>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>  	return ici->ops->querycap(ici, cap);
>  }
>  
> -- 
> 1.7.10.4
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
> On 21/05/15 06:58, Hans Verkuil wrote:
> > On 05/20/2015 06:39 PM, William Towle wrote:
> >> Fill in bus_info field and zero reserved field.
> >>
> >> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
> >> Reviewed-by: William Towle <william.towle@codethink.co.uk>
> >> ---
> >>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
> >> index fd7497e..583c5e6 100644
> >> --- a/drivers/media/platform/soc_camera/soc_camera.c
> >> +++ b/drivers/media/platform/soc_camera/soc_camera.c
> >> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
> >>  	WARN_ON(priv != file->private_data);
> >>  
> >>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
> >> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
> >> +	memset(cap->reserved, 0, sizeof(cap->reserved));
> > 
> > Why the memset? That shouldn't be needed.
> 
> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
> v4l-utils v1.6.2 [1])

I'm puzzled by that.  Isn't this function called by v4l_querycap(),
which is called by video_usercopy()?  And video_usercopy() zeroes the
entire structure before doing so, or at least it appears to be intended
to.

Anyway, if we're failing to initialise kernel memory that's copied to
user-space, that's a (usually minor) security issue and the fix ought to
be cc'd to stable.

Ben.


--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 05/29/2015 03:08 AM, Ben Hutchings wrote:
> On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
>> On 21/05/15 06:58, Hans Verkuil wrote:
>>> On 05/20/2015 06:39 PM, William Towle wrote:
>>>> Fill in bus_info field and zero reserved field.
>>>>
>>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>>>> ---
>>>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>>>  1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>>>> index fd7497e..583c5e6 100644
>>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>>>  	WARN_ON(priv != file->private_data);
>>>>  
>>>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>>>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>>>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>>>
>>> Why the memset? That shouldn't be needed.
>>
>> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
>> v4l-utils v1.6.2 [1])

William, you should use the latest v4l-utils compiled from the git repo.
Unlikely to be related to this, though.

> 
> I'm puzzled by that.  Isn't this function called by v4l_querycap(),
> which is called by video_usercopy()?  And video_usercopy() zeroes the
> entire structure before doing so, or at least it appears to be intended
> to.

Right. So I don't understand this. Can you dig a bit deeper why this would
be needed here? It should not be necessary at all, so if reserved is non-zero,
then someone is writing data where it shouldn't.

Regards,

	Hans

> 
> Anyway, if we're failing to initialise kernel memory that's copied to
> user-space, that's a (usually minor) security issue and the fix ought to
> be cc'd to stable.
> 
> Ben.
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

On 29/05/15 11:10, Hans Verkuil wrote:
> On 05/29/2015 03:08 AM, Ben Hutchings wrote:
>> On Thu, 2015-05-21 at 13:46 +0100, Rob Taylor wrote:
>>> On 21/05/15 06:58, Hans Verkuil wrote:
>>>> On 05/20/2015 06:39 PM, William Towle wrote:
>>>>> Fill in bus_info field and zero reserved field.
>>>>>
>>>>> Signed-off-by: Rob Taylor <rob.taylor@codethink.co.uk>
>>>>> Reviewed-by: William Towle <william.towle@codethink.co.uk>
>>>>> ---
>>>>>  drivers/media/platform/soc_camera/soc_camera.c |    2 ++
>>>>>  1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/platform/soc_camera/soc_camera.c b/drivers/media/platform/soc_camera/soc_camera.c
>>>>> index fd7497e..583c5e6 100644
>>>>> --- a/drivers/media/platform/soc_camera/soc_camera.c
>>>>> +++ b/drivers/media/platform/soc_camera/soc_camera.c
>>>>> @@ -954,6 +954,8 @@ static int soc_camera_querycap(struct file *file, void  *priv,
>>>>>  	WARN_ON(priv != file->private_data);
>>>>>  
>>>>>  	strlcpy(cap->driver, ici->drv_name, sizeof(cap->driver));
>>>>> +	strlcpy(cap->bus_info, "platform:soc_camera", sizeof(cap->bus_info));
>>>>> +	memset(cap->reserved, 0, sizeof(cap->reserved));
>>>>
>>>> Why the memset? That shouldn't be needed.
>>>
>>> v4l2-complience complained it wasn't zero (v4l2-compliance.cpp:308 in
>>> v4l-utils v1.6.2 [1])
> 
> William, you should use the latest v4l-utils compiled from the git repo.
> Unlikely to be related to this, though.
> 
>>
>> I'm puzzled by that.  Isn't this function called by v4l_querycap(),
>> which is called by video_usercopy()?  And video_usercopy() zeroes the
>> entire structure before doing so, or at least it appears to be intended
>> to.
> 
> Right. So I don't understand this. Can you dig a bit deeper why this would
> be needed here? It should not be necessary at all, so if reserved is non-zero,
> then someone is writing data where it shouldn't.


I've just done some digging (including rolling back to when I saw the
issue). Turns out the message I was attempting to 'fix' here was from an
ancient v4l2-compliance, and clearly the outcome of a late night hacking...

The memset is indeed not needed.

Thanks
Rob
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html