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@xxxxxxxxx>
> >>>> ---
> >>>> 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
--
Sakari Ailus
e-mail: sakari.ailus@xxxxxx
