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@xs4all.nl wrote:

Hi Yunfei, Jeffrey,

Some comments below:

On 06/12/2023 09:15, Yunfei Dong wrote:

...

From: Jeffrey Kardatzke jkardatzke@google.com

Validates the secure memory flags when setting up a queue and ensures the queue has the proper capability.

Signed-off-by: Jeffrey Kardatzke jkardatzke@google.com Signed-off-by: Yunfei Dong yunfei.dong@mediatek.com

---

.../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