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
