On 02/09/18 14:04, Sakari Ailus wrote:
> Hi Hans,
>
> On Fri, Feb 09, 2018 at 01:52:50PM +0100, Hans Verkuil wrote:
>> On 02/09/18 13:46, Sakari Ailus wrote:
>>> On Fri, Feb 09, 2018 at 01:20:41PM +0100, Hans Verkuil wrote:
>>>> On 02/09/18 13:17, Sakari Ailus wrote:
>>>>> On Thu, Feb 08, 2018 at 09:36:51AM +0100, Hans Verkuil wrote:
>>>>>> MEDIA_IOC_SETUP_LINK didn't zero the reserved field of the
>>>>>> media_link_desc
>>>>>> struct. Do so in media_device_setup_link().
>>>>>>
>>>>>> Signed-off-by: Hans Verkuil <[email protected]>
>>>>>> ---
>>>>>> drivers/media/media-device.c | 2 ++
>>>>>> 1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>>>>>> index e79f72b8b858..afbf23a19e16 100644
>>>>>> --- a/drivers/media/media-device.c
>>>>>> +++ b/drivers/media/media-device.c
>>>>>> @@ -218,6 +218,8 @@ static long media_device_setup_link(struct
>>>>>> media_device *mdev,
>>>>>> if (link == NULL)
>>>>>> return -EINVAL;
>>>>>>
>>>>>> + memset(linkd->reserved, 0, sizeof(linkd->reserved));
>>>>>> +
>>>>>
>>>>> Doesn't media_device_enum_links() need the same for its reserved field?
>>>>
>>>> enum_links() already zeroes this (actually the whole media_link_desc
>>>> struct is zeroed).
>>>
>>> I can't see that being done in here and I also don't mean the compat
>>> variant. Can you point me to it?
>>>
>>
>> static long media_device_enum_links(struct media_device *mdev,
>> struct media_links_enum *links)
>> {
>> struct media_entity *entity;
>>
>> entity = find_entity(mdev, links->entity);
>> if (entity == NULL)
>> return -EINVAL;
>>
>> if (links->pads) {
>> ...
>> }
>>
>> if (links->links) {
>> struct media_link *link;
>> struct media_link_desc __user *ulink_desc = links->links;
>>
>> list_for_each_entry(link, &entity->links, list) {
>> struct media_link_desc klink_desc;
>>
>> /* Ignore backlinks. */
>> if (link->source->entity != entity)
>> continue;
>> memset(&klink_desc, 0, sizeof(klink_desc));
>> // ^^^^^^^^^^^ zeroed here
>>
>> media_device_kpad_to_upad(link->source,
>> &klink_desc.source);
>> media_device_kpad_to_upad(link->sink,
>> &klink_desc.sink);
>> klink_desc.flags = link->flags;
>> if (copy_to_user(ulink_desc, &klink_desc,
>> sizeof(*ulink_desc)))
>> // ^^^^^^^ copied back to userspace (including zeroed
>> reserved array) here
>
> We are indeed talking about a different reserved field. I mean the one in
> struct media_links_enum .
Ah! I missed that one. I added a check to v4l2-compliance and posted a patch
fixing this.
I thought you meant the reserved field in the media_link_desc structs. That was
very confusing :-)
Regards,
Hans
