Re: [PATCH v3,02/21] v4l2: handle secure memory flags in queue setup

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

 



Yunfei,

Can you please integrate these changes into the patch?

Thanks,
Jeff

On Mon, Dec 11, 2023 at 2:45 AM Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote:
>
> Hi Yunfei, Jeffrey,
>
> Some comments below:
>
> On 06/12/2023 09:15, Yunfei Dong wrote:
> > From: Jeffrey Kardatzke <jkardatzke@xxxxxxxxxx>
> >
> > Validates the secure memory flags when setting up a queue and ensures
> > the queue has the proper capability.
> >
> > Signed-off-by: Jeffrey Kardatzke <jkardatzke@xxxxxxxxxx>
> > Signed-off-by: Yunfei Dong <yunfei.dong@xxxxxxxxxxxx>
> > ---
> >  .../media/common/videobuf2/videobuf2-core.c   | 23 +++++++++++++
> >  .../media/common/videobuf2/videobuf2-v4l2.c   | 34 +++++++++++++------
> >  2 files changed, 46 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 8c1df829745b..09dc030484be 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -813,6 +813,15 @@ static bool verify_coherency_flags(struct vb2_queue *q, bool non_coherent_mem)
> >       return true;
> >  }
> >
> > +static bool verify_secure_mem_flags(struct vb2_queue *q, bool secure_mem)
> > +{
> > +     if (secure_mem != q->secure_mem) {
> > +             dprintk(q, 1, "secure memory model mismatch\n");
> > +             return false;
> > +     }
> > +     return true;
> > +}
> > +
> >  int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >                    unsigned int flags, unsigned int *count)
> >  {
> > @@ -820,6 +829,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >       unsigned int q_num_bufs = vb2_get_num_buffers(q);
> >       unsigned plane_sizes[VB2_MAX_PLANES] = { };
> >       bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> > +     bool secure_mem = flags & V4L2_MEMORY_FLAG_SECURE;
> >       unsigned int i;
> >       int ret = 0;
> >
> > @@ -836,6 +846,8 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >       if (*count == 0 || q_num_bufs != 0 ||
> >           (q->memory != VB2_MEMORY_UNKNOWN && q->memory != memory) ||
> >           !verify_coherency_flags(q, non_coherent_mem)) {
> > +             bool no_previous_buffers = !q->num_buffers;
> > +
> >               /*
> >                * We already have buffers allocated, so first check if they
> >                * are not in use and can be freed.
> > @@ -854,6 +866,12 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >               __vb2_queue_free(q, q_num_bufs);
> >               mutex_unlock(&q->mmap_lock);
> >
> > +             /*
> > +              * Do not allow switching secure buffer mode.
> > +              */
> > +             if (!no_previous_buffers && !verify_secure_mem_flags(q, secure_mem))
> > +                     return -EINVAL;
> > +
>
> Why is this needed? Here VIDIOC_REQBUFS is called either to just delete
> all existing buffers (count == 0), or to delete all existing buffers and
> allocate new buffers (count > 0).
>
> Since in both cases all existing buffers are deleted, you are free to choose
> whatever new secure mode you want.
>
> >               /*
> >                * In case of REQBUFS(0) return immediately without calling
> >                * driver's queue_setup() callback and allocating resources.
> > @@ -882,6 +900,7 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
> >       if (ret)
> >               return ret;
> >       set_queue_coherency(q, non_coherent_mem);
> > +     q->secure_mem = secure_mem;
> >
> >       /*
> >        * Ask the driver how many buffers and planes per buffer it requires.
> > @@ -986,6 +1005,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> >       unsigned plane_sizes[VB2_MAX_PLANES] = { };
> >       bool non_coherent_mem = flags & V4L2_MEMORY_FLAG_NON_COHERENT;
> >       unsigned int q_num_bufs = vb2_get_num_buffers(q);
> > +     bool secure_mem = flags & V4L2_MEMORY_FLAG_SECURE;
> >       bool no_previous_buffers = !q_num_bufs;
> >       int ret = 0;
> >
> > @@ -1015,6 +1035,7 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> >                       return ret;
> >               q->waiting_for_buffers = !q->is_output;
> >               set_queue_coherency(q, non_coherent_mem);
> > +             q->secure_mem = secure_mem;
> >       } else {
> >               if (q->memory != memory) {
> >                       dprintk(q, 1, "memory model mismatch\n");
> > @@ -1022,6 +1043,8 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
> >               }
> >               if (!verify_coherency_flags(q, non_coherent_mem))
> >                       return -EINVAL;
> > +             if (!verify_secure_mem_flags(q, secure_mem))
> > +                     return -EINVAL;
> >       }
> >
> >       num_buffers = min(*count, q->max_num_buffers - q_num_bufs);
> > diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > index 54d572c3b515..0a530830276c 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
> > @@ -686,22 +686,30 @@ static void fill_buf_caps(struct vb2_queue *q, u32 *caps)
> >               *caps |= V4L2_BUF_CAP_SUPPORTS_MMAP_CACHE_HINTS;
> >       if (q->supports_requests)
> >               *caps |= V4L2_BUF_CAP_SUPPORTS_REQUESTS;
> > +     if (q->allow_secure_mem && q->io_modes & VB2_DMABUF)
> > +             *caps |= V4L2_BUF_CAP_SUPPORTS_SECURE_MEM;
> >  }
> >
> > -static void validate_memory_flags(struct vb2_queue *q,
> > +static bool validate_memory_flags(struct vb2_queue *q,
> >                                 int memory,
> >                                 u32 *flags)
> >  {
> > +     if (*flags & V4L2_MEMORY_FLAG_SECURE &&
> > +         (!q->allow_secure_mem || memory != V4L2_MEMORY_DMABUF)) {
> > +             return false;
> > +     }
> > +
>
> This check belongs to videobuf2-core.c and the check should be done
> in vb2_core_reqbufs and vb2_core_create_bufs.
>
> So just leave this function as a void.
>
> >       if (!q->allow_cache_hints || memory != V4L2_MEMORY_MMAP) {
> >               /*
> > -              * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only,
> > -              * but in order to avoid bugs we zero out all bits.
> > +              * This needs to clear V4L2_MEMORY_FLAG_NON_COHERENT only.
>
> Just drop this as well since it adds no useful information anymore.
>
> >                */
> > -             *flags = 0;
> > -     } else {
> > -             /* Clear all unknown flags. */
> > -             *flags &= V4L2_MEMORY_FLAG_NON_COHERENT;
> > +             *flags &= ~V4L2_MEMORY_FLAG_NON_COHERENT;
> >       }
> > +
> > +     /* Clear all unknown flags. */
> > +     *flags &= V4L2_MEMORY_FLAG_NON_COHERENT | V4L2_MEMORY_FLAG_SECURE;
>
> This is still needed here.
>
> > +
> > +     return true;
> >  }
> >
>
> So the following changes from here...
>
> >  int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> > @@ -710,7 +718,8 @@ int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> >       u32 flags = req->flags;
> >
> >       fill_buf_caps(q, &req->capabilities);
> > -     validate_memory_flags(q, req->memory, &flags);
> > +     if (!validate_memory_flags(q, req->memory, &flags))
> > +             return -EINVAL;
> >       req->flags = flags;
> >       return ret ? ret : vb2_core_reqbufs(q, req->memory,
> >                                           req->flags, &req->count);
> > @@ -752,7 +761,8 @@ int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create)
> >       unsigned i;
> >
> >       fill_buf_caps(q, &create->capabilities);
> > -     validate_memory_flags(q, create->memory, &create->flags);
> > +     if (!validate_memory_flags(q, create->memory, &create->flags))
> > +             return -EINVAL;
> >       create->index = vb2_get_num_buffers(q);
> >       create->max_num_buffers = q->max_num_buffers;
> >       create->capabilities |= V4L2_BUF_CAP_SUPPORTS_MAX_NUM_BUFFERS;
> > @@ -1007,7 +1017,8 @@ int vb2_ioctl_reqbufs(struct file *file, void *priv,
> >       u32 flags = p->flags;
> >
> >       fill_buf_caps(vdev->queue, &p->capabilities);
> > -     validate_memory_flags(vdev->queue, p->memory, &flags);
> > +     if (!validate_memory_flags(vdev->queue, p->memory, &flags))
> > +             return -EINVAL;
> >       p->flags = flags;
> >       if (res)
> >               return res;
> > @@ -1031,7 +1042,8 @@ int vb2_ioctl_create_bufs(struct file *file, void *priv,
> >
> >       p->index = vdev->queue->num_buffers;
> >       fill_buf_caps(vdev->queue, &p->capabilities);
> > -     validate_memory_flags(vdev->queue, p->memory, &p->flags);
> > +     if (!validate_memory_flags(vdev->queue, p->memory, &p->flags))
> > +             return -EINVAL;
> >       /*
> >        * If count == 0, then just check if memory and type are valid.
> >        * Any -EBUSY result from vb2_verify_memory_type can be mapped to 0.
>
> ...to the end should all be dropped since the vb2 core will do the checks.
>
> Regards,
>
>         Hans




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux