diff mbox

[PATCHv2,11/15] media-device.c: zero reserved field

Message ID 20180208083655.32248-12-hverkuil@xs4all.nl (mailing list archive)
State Superseded, archived
Delegated to: Hans Verkuil
Headers

Commit Message

Hans Verkuil Feb. 8, 2018, 8:36 a.m. UTC 
  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 <hans.verkuil@cisco.com>
---
 drivers/media/media-device.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Sakari Ailus Feb. 9, 2018, 12:17 p.m. UTC | #1 
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 <hans.verkuil@cisco.com>
> ---
>  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?

>  	/* Setup the link on both entities. */
>  	return __media_entity_setup_link(link, linkd->flags);
>  }
> -- 
> 2.15.1
>
Hans Verkuil Feb. 9, 2018, 12:20 p.m. UTC | #2 
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 <hans.verkuil@cisco.com>
>> ---
>>  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).

Regards,

	Hans

> 
>>  	/* Setup the link on both entities. */
>>  	return __media_entity_setup_link(link, linkd->flags);
>>  }
>> -- 
>> 2.15.1
>>
>
Sakari Ailus Feb. 9, 2018, 12:46 p.m. UTC | #3 
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 <hans.verkuil@cisco.com>
> >> ---
> >>  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?
Hans Verkuil (hansverk) Feb. 9, 2018, 12:52 p.m. UTC | #4 
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 <hans.verkuil@cisco.com>
>>>> ---
>>>>  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
                                return -EFAULT;
                        ulink_desc++;
                }
        }

        return 0;
}

Regards,

	Hans
Sakari Ailus Feb. 9, 2018, 1:04 p.m. UTC | #5 
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 <hans.verkuil@cisco.com>
> >>>> ---
> >>>>  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 .

>                                 return -EFAULT;
>                         ulink_desc++;
>                 }
>         }
> 
>         return 0;
> }
> 
> Regards,
> 
> 	Hans
Hans Verkuil Feb. 9, 2018, 1:27 p.m. UTC | #6 
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 <hans.verkuil@cisco.com>
>>>>>> ---
>>>>>>  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
diff mbox

Patch

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));
+
 	/* Setup the link on both entities. */
 	return __media_entity_setup_link(link, linkd->flags);
 }