[PATCH v2 0/2] media: videobuf2: publish buffers in bufs_bitmap under mmap_lock

[PATCH v2 0/2] media: videobuf2: publish buffers in bufs_bitmap under mmap_lock

[Ricardo Ribalda]

vb2_mmap() only takes q->mmap_lock, not the main queue serialization
lock (q->lock). Commit a286b0837e13 ("media: core: Add bitmap manage
bufs array entries") started setting bits in q->bufs_bitmap inside
__vb2_queue_alloc() before buffer memory is allocated and without
holding q->mmap_lock, allowing a concurrent mmap() to access a
half-initialized buffer and hit a use-after-free if allocation or
.buf_init() fails, and prematurely counting tentative buffers during
partial-allocation .queue_setup() re-checks.

Patch 1 fixes this by dropping set_bit() from vb2_queue_add_buffer() and
publishing the range in q->bufs_bitmap via vb2_queue_publish_buffers()
under q->mmap_lock once allocation and .queue_setup() succeed.

Patch 2 adds smp_mb__before_atomic() / test_bit_acquire() ordering on
q->bufs_bitmap for lockless vb2_get_buffer() callers (such as
vb2_find_buffer() called from stateless mem2mem .device_run() while
VIDIOC_CREATE_BUFS runs on weakly-ordered architectures).

This series was generated by an LLM, but supervised and reviewed by a
human (not the most handsome human, but a human).

---
Changes in v2:
- Keep vb2_queue_add_buffer() without set_bit() so __vb2_queue_alloc()
  stays unchanged, and add vb2_queue_publish_buffers() using atomic
  set_bit() (rather than non-atomic bitmap_set(), since vb2_get_buffer()
  has lockless readers).
- Split the smp_mb__before_atomic() / test_bit_acquire() memory barrier
  pair into a separate patch with an explanation of the arm64
  VIDIOC_CREATE_BUFS vs vb2_find_buffer() race.
- Drop lockdep_assert_held() from vb2_queue_remove_buffer().
- Simplify the !q->bufs guard in __vb2_queue_free() to 'if (!q->bufs)
  return;' (still needed as a NULL check now that __vb2_queue_free()
  indexes q->bufs[] directly instead of going through vb2_get_buffer(),
  since vb2_core_queue_release() and REQBUFS(0) call it when
  q->bufs == NULL).
- Clarify @bufs and @bufs_bitmap documentation in struct vb2_queue.
- Link to v1: https://lore.kernel.org/r/20261102-vb2-mmap_lock-v1-1-fe550fe03ed8@chromium.org

---
Ricardo Ribalda (2):
      media: videobuf2: publish buffers in bufs_bitmap under mmap_lock
      media: videobuf2: order buffer initialization before bufs_bitmap publication

 drivers/media/common/videobuf2/videobuf2-core.c | 74 +++++++++++++++++++++----
 include/media/videobuf2-core.h                  | 14 +++--
 2 files changed, 74 insertions(+), 14 deletions(-)
---
base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
change-id: 20261102-vb2-mmap_lock-959e0cc4edb3

Best regards,
-- 
Ricardo Ribalda <ribalda@chromium.org>

[PATCH v2 1/2] media: videobuf2: publish buffers in bufs_bitmap under mmap_lock

[Ricardo Ribalda]

vb2_mmap() only takes q->mmap_lock, not the main queue lock (q->lock).
To keep vb2_mmap() safe, newly allocated buffers must not become visible
in vb2_get_buffer() until they are fully initialized and q->mmap_lock is
held.

Commit a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
started setting q->bufs_bitmap inside __vb2_queue_alloc() before buffer
memory is allocated and without holding q->mmap_lock. As a result:

- A concurrent mmap() can find a half-initialized buffer, and if buffer
  allocation or .buf_init() fails, __vb2_queue_alloc() frees that buffer
  without holding q->mmap_lock, causing a use-after-free in vb2_mmap().
- If fewer buffers are allocated than requested, they are already
  counted as active in q->bufs_bitmap before .queue_setup() checks
  whether the smaller count is acceptable.

Fix this by keeping new buffers only in q->bufs[] during allocation and
setting their bits in q->bufs_bitmap under q->mmap_lock once allocation
and .queue_setup() succeed, and let __vb2_queue_free() read q->bufs[]
directly when cleaning up unexposed buffers.

Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
Cc: stable@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 66 +++++++++++++++++++++----
 include/media/videobuf2-core.h                  |  8 +--
 2 files changed, 61 insertions(+), 13 deletions(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index b0a6084f1757..70ef38c3d16a 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -431,6 +431,9 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
  * @q:	pointer to &struct vb2_queue with videobuf2 queue.
  * @vb:	pointer to &struct vb2_buffer to be added to the queue.
  * @index: index where add vb2_buffer in the queue
+ *
+ * The buffer is stored in @q->bufs[] but remains invisible to vb2_get_buffer()
+ * until published with vb2_queue_publish_buffers().
  */
 static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
 {
@@ -439,7 +442,31 @@ static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
 	q->bufs[index] = vb;
 	vb->index = index;
 	vb->vb2_queue = q;
-	set_bit(index, q->bufs_bitmap);
+}
+
+/**
+ * vb2_queue_publish_buffers() - publish buffers staged by __vb2_queue_alloc()
+ * @q:		pointer to &struct vb2_queue with videobuf2 queue.
+ * @start:	index of the first buffer to publish.
+ * @count:	number of buffers to publish.
+ *
+ * Set the @q->bufs_bitmap bits of the @count fully initialized buffers that
+ * __vb2_queue_alloc() stored in @q->bufs[] starting at @start, which makes
+ * them visible to vb2_get_buffer().
+ */
+static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
+				      unsigned int count)
+{
+	unsigned int i;
+
+	lockdep_assert_held(&q->mmap_lock);
+
+	for (i = start; i < start + count; i++) {
+		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
+			    test_bit(i, q->bufs_bitmap)))
+			continue;
+		set_bit(i, q->bufs_bitmap);
+	}
 }
 
 /**
@@ -460,6 +487,11 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
  * @first_index: index of the first created buffer, all newly allocated buffers
  *		 have indices in the range [first_index..first_index+count-1]
  *
+ * The allocated buffers are stored in q->bufs[] but not published in
+ * q->bufs_bitmap: the caller must either publish them with
+ * vb2_queue_publish_buffers() or free them with __vb2_queue_free(), both under
+ * q->mmap_lock.
+ *
  * Returns the number of buffers successfully allocated.
  */
 static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
@@ -562,7 +594,8 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
 	struct vb2_buffer *vb;
 
 	for (i = start; i < start + count; i++) {
-		vb = vb2_get_buffer(q, i);
+		/* See __vb2_queue_free() for why vb2_get_buffer() isn't used */
+		vb = q->bufs[i];
 		if (!vb)
 			continue;
 
@@ -580,6 +613,11 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
  * __vb2_queue_free() - free @count buffers from @start index of the queue - video memory and
  * related information, if no buffers are left return the queue to an
  * uninitialized state. Might be called even if the queue has already been freed.
+ *
+ * Buffers are looked up directly in q->bufs[] instead of through
+ * vb2_get_buffer() so that buffers staged by __vb2_queue_alloc() that were
+ * never published in q->bufs_bitmap (because a later .queue_setup() call
+ * rejected the allocated number of buffers) are freed as well.
  */
 static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned int count)
 {
@@ -587,9 +625,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
 
 	lockdep_assert_held(&q->mmap_lock);
 
+	/* q->bufs is NULL until vb2_core_allocated_buffers_storage() runs */
+	if (!q->bufs)
+		return;
+
 	/* Call driver-provided cleanup function for each buffer, if provided */
 	for (i = start; i < start + count; i++) {
-		struct vb2_buffer *vb = vb2_get_buffer(q, i);
+		struct vb2_buffer *vb = q->bufs[i];
 
 		if (vb && vb->planes[0].mem_priv)
 			call_void_vb_qop(vb, buf_cleanup, vb);
@@ -624,7 +666,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
 		q->cnt_unprepare_streaming = 0;
 	}
 	for (i = start; i < start + count; i++) {
-		struct vb2_buffer *vb = vb2_get_buffer(q, i);
+		struct vb2_buffer *vb = q->bufs[i];
 		bool unbalanced;
 
 		if (!vb)
@@ -676,7 +718,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
 
 	/* Free vb2 buffers */
 	for (i = start; i < start + count; i++) {
-		struct vb2_buffer *vb = vb2_get_buffer(q, i);
+		struct vb2_buffer *vb = q->bufs[i];
 
 		if (!vb)
 			continue;
@@ -1000,14 +1042,16 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
 
 	if (ret < 0) {
 		/*
-		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
-		 * from already queued buffers and it will reset q->memory to
+		 * Note: the buffers allocated by __vb2_queue_alloc() were never
+		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
+		 * straight from q->bufs[] and resets q->memory to
 		 * VB2_MEMORY_UNKNOWN.
 		 */
 		__vb2_queue_free(q, first_index, allocated_buffers);
 		mutex_unlock(&q->mmap_lock);
 		return ret;
 	}
+	vb2_queue_publish_buffers(q, first_index, allocated_buffers);
 	mutex_unlock(&q->mmap_lock);
 
 	/*
@@ -1125,14 +1169,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
 
 	if (ret < 0) {
 		/*
-		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
-		 * from already queued buffers and it will reset q->memory to
-		 * VB2_MEMORY_UNKNOWN.
+		 * Note: the buffers allocated by __vb2_queue_alloc() were never
+		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
+		 * straight from q->bufs[] and resets q->memory to
+		 * VB2_MEMORY_UNKNOWN if no buffers are left.
 		 */
 		__vb2_queue_free(q, *first_index, allocated_buffers);
 		mutex_unlock(&q->mmap_lock);
 		return -ENOMEM;
 	}
+	vb2_queue_publish_buffers(q, *first_index, allocated_buffers);
 	mutex_unlock(&q->mmap_lock);
 
 	/*
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 4b4f4c15c53a..2e93e6985d5f 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -564,9 +564,11 @@ struct vb2_buf_ops {
  * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
  * @memory:	current memory type used
  * @dma_dir:	DMA mapping direction.
- * @bufs:	videobuf2 buffer structures. If it is non-NULL then
- *		bufs_bitmap is also non-NULL.
- * @bufs_bitmap: bitmap tracking whether each bufs[] entry is used
+ * @bufs:	videobuf2 buffer structures. Must be accessed with
+ *		vb2_get_buffer(), as an allocated buffer cannot be used
+ *		until it is published.
+ * @bufs_bitmap: bitmap tracking published buffers. Bits are set and cleared
+ *		under @mmap_lock.
  * @max_num_buffers: upper limit of number of allocated/used buffers.
  *		     If set to 0 v4l2 core will change it VB2_MAX_FRAME
  *		     for backward compatibility.

-- 
2.56.0.385.gac671c819a-goog

[PATCH v2 2/2] media: videobuf2: order buffer initialization before bufs_bitmap publication

[Ricardo Ribalda]

Stateless mem2mem decoders call vb2_find_buffer() -> vb2_get_buffer()
from .device_run() without holding q->lock or q->mmap_lock, which can
run concurrently with VIDIOC_CREATE_BUFS on a streaming queue.

Both set_bit() and test_bit() are unordered, and mutex_lock(&q->mmap_lock)
is only an acquire barrier. On weakly-ordered architectures such as
arm64, a lockless vb2_get_buffer() caller can observe a newly set bit in
q->bufs_bitmap before the kzalloc() zeroing, buffer initialization, and
q->bufs[] pointer store performed by __vb2_queue_alloc() become visible.

Pair smp_mb__before_atomic() in vb2_queue_publish_buffers() with
test_bit_acquire() in vb2_get_buffer() so that any reader observing a
set bit in q->bufs_bitmap also sees a fully initialized vb2_buffer.

Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
Cc: stable@vger.kernel.org
Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
---
 drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
 include/media/videobuf2-core.h                  | 6 +++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 70ef38c3d16a..d0dd4b22b646 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -461,6 +461,14 @@ static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
 
 	lockdep_assert_held(&q->mmap_lock);
 
+	/*
+	 * Order the buffer initialization and the q->bufs[] stores done by
+	 * __vb2_queue_alloc() before the bits that publish them. Pairs with
+	 * test_bit_acquire() in vb2_get_buffer(), which is also called
+	 * without q->lock or q->mmap_lock held, e.g. by vb2_find_buffer()
+	 * from a mem2mem .device_run().
+	 */
+	smp_mb__before_atomic();
 	for (i = start; i < start + count; i++) {
 		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
 			    test_bit(i, q->bufs_bitmap)))
diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
index 2e93e6985d5f..20b961da21e0 100644
--- a/include/media/videobuf2-core.h
+++ b/include/media/videobuf2-core.h
@@ -1288,7 +1288,11 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
 	if (index >= q->max_num_buffers)
 		return NULL;
 
-	if (test_bit(index, q->bufs_bitmap))
+	/*
+	 * A set bit guarantees that the buffer is fully initialized, pairs
+	 * with smp_mb__before_atomic() in vb2_queue_publish_buffers().
+	 */
+	if (test_bit_acquire(index, q->bufs_bitmap))
 		return q->bufs[index];
 	return NULL;
 }

-- 
2.56.0.385.gac671c819a-goog

Re: [PATCH v2 2/2] media: videobuf2: order buffer initialization before bufs_bitmap publication

[Marek Szyprowski]

On 03.11.2026 11:56, Ricardo Ribalda wrote:
> Stateless mem2mem decoders call vb2_find_buffer() -> vb2_get_buffer()
> from .device_run() without holding q->lock or q->mmap_lock, which can
> run concurrently with VIDIOC_CREATE_BUFS on a streaming queue.
>
> Both set_bit() and test_bit() are unordered, and mutex_lock(&q->mmap_lock)
> is only an acquire barrier. On weakly-ordered architectures such as
> arm64, a lockless vb2_get_buffer() caller can observe a newly set bit in
> q->bufs_bitmap before the kzalloc() zeroing, buffer initialization, and
> q->bufs[] pointer store performed by __vb2_queue_alloc() become visible.
>
> Pair smp_mb__before_atomic() in vb2_queue_publish_buffers() with
> test_bit_acquire() in vb2_get_buffer() so that any reader observing a
> set bit in q->bufs_bitmap also sees a fully initialized vb2_buffer.
>
> Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>


Can't we use ordinary locking instead of those memory barriers and relying
on not-always-atomic bit operations? For me this looks like over-optimization,
taking into account that those operations are not on the critical path nor
anyhow time-comparable with filling the whole buffer with image data.


> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
>  include/media/videobuf2-core.h                  | 6 +++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 70ef38c3d16a..d0dd4b22b646 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -461,6 +461,14 @@ static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
>  
>  	lockdep_assert_held(&q->mmap_lock);
>  
> +	/*
> +	 * Order the buffer initialization and the q->bufs[] stores done by
> +	 * __vb2_queue_alloc() before the bits that publish them. Pairs with
> +	 * test_bit_acquire() in vb2_get_buffer(), which is also called
> +	 * without q->lock or q->mmap_lock held, e.g. by vb2_find_buffer()
> +	 * from a mem2mem .device_run().
> +	 */
> +	smp_mb__before_atomic();
>  	for (i = start; i < start + count; i++) {
>  		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
>  			    test_bit(i, q->bufs_bitmap)))
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 2e93e6985d5f..20b961da21e0 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -1288,7 +1288,11 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>  	if (index >= q->max_num_buffers)
>  		return NULL;
>  
> -	if (test_bit(index, q->bufs_bitmap))
> +	/*
> +	 * A set bit guarantees that the buffer is fully initialized, pairs
> +	 * with smp_mb__before_atomic() in vb2_queue_publish_buffers().
> +	 */
> +	if (test_bit_acquire(index, q->bufs_bitmap))
>  		return q->bufs[index];
>  	return NULL;
>  }
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2 2/2] media: videobuf2: order buffer initialization before bufs_bitmap publication

[Hans Verkuil]

On 11/3/26 14:39, Marek Szyprowski wrote:
> On 03.11.2026 11:56, Ricardo Ribalda wrote:
>> Stateless mem2mem decoders call vb2_find_buffer() -> vb2_get_buffer()
>> from .device_run() without holding q->lock or q->mmap_lock, which can
>> run concurrently with VIDIOC_CREATE_BUFS on a streaming queue.
>>
>> Both set_bit() and test_bit() are unordered, and mutex_lock(&q->mmap_lock)
>> is only an acquire barrier. On weakly-ordered architectures such as
>> arm64, a lockless vb2_get_buffer() caller can observe a newly set bit in
>> q->bufs_bitmap before the kzalloc() zeroing, buffer initialization, and
>> q->bufs[] pointer store performed by __vb2_queue_alloc() become visible.
>>
>> Pair smp_mb__before_atomic() in vb2_queue_publish_buffers() with
>> test_bit_acquire() in vb2_get_buffer() so that any reader observing a
>> set bit in q->bufs_bitmap also sees a fully initialized vb2_buffer.
>>
>> Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> 
> 
> Can't we use ordinary locking instead of those memory barriers and relying
> on not-always-atomic bit operations? For me this looks like over-optimization,
> taking into account that those operations are not on the critical path nor
> anyhow time-comparable with filling the whole buffer with image data.

I was thinking the same thing.

I *think* the code is correct, but I'm not sure. Memory barriers are hard to get
right, and I consider myself to be a newbie when it comes to that. A simple spinlock
is much easier to understand, and this is hardly a high-performance loop where you
need to squeeze the last bit of performance out of it.

It's why I wanted to split this off from the original patch, so it is easier to
review.

Regards,

	Hans

> 
> 
>> ---
>>  drivers/media/common/videobuf2/videobuf2-core.c | 8 ++++++++
>>  include/media/videobuf2-core.h                  | 6 +++++-
>>  2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 70ef38c3d16a..d0dd4b22b646 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -461,6 +461,14 @@ static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
>>  
>>  	lockdep_assert_held(&q->mmap_lock);
>>  
>> +	/*
>> +	 * Order the buffer initialization and the q->bufs[] stores done by
>> +	 * __vb2_queue_alloc() before the bits that publish them. Pairs with
>> +	 * test_bit_acquire() in vb2_get_buffer(), which is also called
>> +	 * without q->lock or q->mmap_lock held, e.g. by vb2_find_buffer()
>> +	 * from a mem2mem .device_run().
>> +	 */
>> +	smp_mb__before_atomic();
>>  	for (i = start; i < start + count; i++) {
>>  		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
>>  			    test_bit(i, q->bufs_bitmap)))
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 2e93e6985d5f..20b961da21e0 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -1288,7 +1288,11 @@ static inline struct vb2_buffer *vb2_get_buffer(struct vb2_queue *q,
>>  	if (index >= q->max_num_buffers)
>>  		return NULL;
>>  
>> -	if (test_bit(index, q->bufs_bitmap))
>> +	/*
>> +	 * A set bit guarantees that the buffer is fully initialized, pairs
>> +	 * with smp_mb__before_atomic() in vb2_queue_publish_buffers().
>> +	 */
>> +	if (test_bit_acquire(index, q->bufs_bitmap))
>>  		return q->bufs[index];
>>  	return NULL;
>>  }
>>
> Best regards

Re: [PATCH v2 1/2] media: videobuf2: publish buffers in bufs_bitmap under mmap_lock

[Hans Verkuil]

Hi Ricardo,

On 11/3/26 11:56, Ricardo Ribalda wrote:
> vb2_mmap() only takes q->mmap_lock, not the main queue lock (q->lock).
> To keep vb2_mmap() safe, newly allocated buffers must not become visible
> in vb2_get_buffer() until they are fully initialized and q->mmap_lock is
> held.
> 
> Commit a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> started setting q->bufs_bitmap inside __vb2_queue_alloc() before buffer
> memory is allocated and without holding q->mmap_lock. As a result:
> 
> - A concurrent mmap() can find a half-initialized buffer, and if buffer
>   allocation or .buf_init() fails, __vb2_queue_alloc() frees that buffer
>   without holding q->mmap_lock, causing a use-after-free in vb2_mmap().
> - If fewer buffers are allocated than requested, they are already
>   counted as active in q->bufs_bitmap before .queue_setup() checks
>   whether the smaller count is acceptable.
> 
> Fix this by keeping new buffers only in q->bufs[] during allocation and
> setting their bits in q->bufs_bitmap under q->mmap_lock once allocation
> and .queue_setup() succeed, and let __vb2_queue_free() read q->bufs[]
> directly when cleaning up unexposed buffers.
> 

Thank you for making the change, it much easier to follow now.

Marek, Tomasz, can one (or both) of you review this patch as well?

I think this can go to the fixes branch. Patch 2/2 needs more discussion and
is something for the next branch.

Regards,

	Hans

> Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>
> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 66 +++++++++++++++++++++----
>  include/media/videobuf2-core.h                  |  8 +--
>  2 files changed, 61 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b0a6084f1757..70ef38c3d16a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -431,6 +431,9 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>   * @q:	pointer to &struct vb2_queue with videobuf2 queue.
>   * @vb:	pointer to &struct vb2_buffer to be added to the queue.
>   * @index: index where add vb2_buffer in the queue
> + *
> + * The buffer is stored in @q->bufs[] but remains invisible to vb2_get_buffer()
> + * until published with vb2_queue_publish_buffers().
>   */
>  static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
>  {
> @@ -439,7 +442,31 @@ static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
>  	q->bufs[index] = vb;
>  	vb->index = index;
>  	vb->vb2_queue = q;
> -	set_bit(index, q->bufs_bitmap);
> +}
> +
> +/**
> + * vb2_queue_publish_buffers() - publish buffers staged by __vb2_queue_alloc()
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @start:	index of the first buffer to publish.
> + * @count:	number of buffers to publish.
> + *
> + * Set the @q->bufs_bitmap bits of the @count fully initialized buffers that
> + * __vb2_queue_alloc() stored in @q->bufs[] starting at @start, which makes
> + * them visible to vb2_get_buffer().
> + */
> +static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
> +				      unsigned int count)
> +{
> +	unsigned int i;
> +
> +	lockdep_assert_held(&q->mmap_lock);
> +
> +	for (i = start; i < start + count; i++) {
> +		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
> +			    test_bit(i, q->bufs_bitmap)))
> +			continue;
> +		set_bit(i, q->bufs_bitmap);
> +	}
>  }
>  
>  /**
> @@ -460,6 +487,11 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>   * @first_index: index of the first created buffer, all newly allocated buffers
>   *		 have indices in the range [first_index..first_index+count-1]
>   *
> + * The allocated buffers are stored in q->bufs[] but not published in
> + * q->bufs_bitmap: the caller must either publish them with
> + * vb2_queue_publish_buffers() or free them with __vb2_queue_free(), both under
> + * q->mmap_lock.
> + *
>   * Returns the number of buffers successfully allocated.
>   */
>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> @@ -562,7 +594,8 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
>  	struct vb2_buffer *vb;
>  
>  	for (i = start; i < start + count; i++) {
> -		vb = vb2_get_buffer(q, i);
> +		/* See __vb2_queue_free() for why vb2_get_buffer() isn't used */
> +		vb = q->bufs[i];
>  		if (!vb)
>  			continue;
>  
> @@ -580,6 +613,11 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
>   * __vb2_queue_free() - free @count buffers from @start index of the queue - video memory and
>   * related information, if no buffers are left return the queue to an
>   * uninitialized state. Might be called even if the queue has already been freed.
> + *
> + * Buffers are looked up directly in q->bufs[] instead of through
> + * vb2_get_buffer() so that buffers staged by __vb2_queue_alloc() that were
> + * never published in q->bufs_bitmap (because a later .queue_setup() call
> + * rejected the allocated number of buffers) are freed as well.
>   */
>  static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned int count)
>  {
> @@ -587,9 +625,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  
>  	lockdep_assert_held(&q->mmap_lock);
>  
> +	/* q->bufs is NULL until vb2_core_allocated_buffers_storage() runs */
> +	if (!q->bufs)
> +		return;
> +
>  	/* Call driver-provided cleanup function for each buffer, if provided */
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  
>  		if (vb && vb->planes[0].mem_priv)
>  			call_void_vb_qop(vb, buf_cleanup, vb);
> @@ -624,7 +666,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  		q->cnt_unprepare_streaming = 0;
>  	}
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  		bool unbalanced;
>  
>  		if (!vb)
> @@ -676,7 +718,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  
>  	/* Free vb2 buffers */
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  
>  		if (!vb)
>  			continue;
> @@ -1000,14 +1042,16 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	if (ret < 0) {
>  		/*
> -		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> -		 * from already queued buffers and it will reset q->memory to
> +		 * Note: the buffers allocated by __vb2_queue_alloc() were never
> +		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
> +		 * straight from q->bufs[] and resets q->memory to
>  		 * VB2_MEMORY_UNKNOWN.
>  		 */
>  		__vb2_queue_free(q, first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return ret;
>  	}
> +	vb2_queue_publish_buffers(q, first_index, allocated_buffers);
>  	mutex_unlock(&q->mmap_lock);
>  
>  	/*
> @@ -1125,14 +1169,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	if (ret < 0) {
>  		/*
> -		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> -		 * from already queued buffers and it will reset q->memory to
> -		 * VB2_MEMORY_UNKNOWN.
> +		 * Note: the buffers allocated by __vb2_queue_alloc() were never
> +		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
> +		 * straight from q->bufs[] and resets q->memory to
> +		 * VB2_MEMORY_UNKNOWN if no buffers are left.
>  		 */
>  		__vb2_queue_free(q, *first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return -ENOMEM;
>  	}
> +	vb2_queue_publish_buffers(q, *first_index, allocated_buffers);
>  	mutex_unlock(&q->mmap_lock);
>  
>  	/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b4f4c15c53a..2e93e6985d5f 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -564,9 +564,11 @@ struct vb2_buf_ops {
>   * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
>   * @memory:	current memory type used
>   * @dma_dir:	DMA mapping direction.
> - * @bufs:	videobuf2 buffer structures. If it is non-NULL then
> - *		bufs_bitmap is also non-NULL.
> - * @bufs_bitmap: bitmap tracking whether each bufs[] entry is used
> + * @bufs:	videobuf2 buffer structures. Must be accessed with
> + *		vb2_get_buffer(), as an allocated buffer cannot be used
> + *		until it is published.
> + * @bufs_bitmap: bitmap tracking published buffers. Bits are set and cleared
> + *		under @mmap_lock.
>   * @max_num_buffers: upper limit of number of allocated/used buffers.
>   *		     If set to 0 v4l2 core will change it VB2_MAX_FRAME
>   *		     for backward compatibility.
> 

Re: [PATCH v2 1/2] media: videobuf2: publish buffers in bufs_bitmap under mmap_lock

[Marek Szyprowski]

On 03.11.2026 11:56, Ricardo Ribalda wrote:
> vb2_mmap() only takes q->mmap_lock, not the main queue lock (q->lock).
> To keep vb2_mmap() safe, newly allocated buffers must not become visible
> in vb2_get_buffer() until they are fully initialized and q->mmap_lock is
> held.
>
> Commit a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> started setting q->bufs_bitmap inside __vb2_queue_alloc() before buffer
> memory is allocated and without holding q->mmap_lock. As a result:
>
> - A concurrent mmap() can find a half-initialized buffer, and if buffer
>   allocation or .buf_init() fails, __vb2_queue_alloc() frees that buffer
>   without holding q->mmap_lock, causing a use-after-free in vb2_mmap().
> - If fewer buffers are allocated than requested, they are already
>   counted as active in q->bufs_bitmap before .queue_setup() checks
>   whether the smaller count is acceptable.
>
> Fix this by keeping new buffers only in q->bufs[] during allocation and
> setting their bits in q->bufs_bitmap under q->mmap_lock once allocation
> and .queue_setup() succeed, and let __vb2_queue_free() read q->bufs[]
> directly when cleaning up unexposed buffers.
>
> Fixes: a286b0837e13 ("media: core: Add bitmap manage bufs array entries")
> Cc: stable@vger.kernel.org
> Signed-off-by: Ricardo Ribalda <ribalda@chromium.org>

One minor issue below, besides that is look fine.

Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com>


> ---
>  drivers/media/common/videobuf2/videobuf2-core.c | 66 +++++++++++++++++++++----
>  include/media/videobuf2-core.h                  |  8 +--
>  2 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index b0a6084f1757..70ef38c3d16a 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -431,6 +431,9 @@ static void init_buffer_cache_hints(struct vb2_queue *q, struct vb2_buffer *vb)
>   * @q:	pointer to &struct vb2_queue with videobuf2 queue.
>   * @vb:	pointer to &struct vb2_buffer to be added to the queue.
>   * @index: index where add vb2_buffer in the queue
> + *
> + * The buffer is stored in @q->bufs[] but remains invisible to vb2_get_buffer()
> + * until published with vb2_queue_publish_buffers().
>   */
>  static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, unsigned int index)
>  {
> @@ -439,7 +442,31 @@ static void vb2_queue_add_buffer(struct vb2_queue *q, struct vb2_buffer *vb, uns
>  	q->bufs[index] = vb;
>  	vb->index = index;
>  	vb->vb2_queue = q;
> -	set_bit(index, q->bufs_bitmap);
> +}
> +
> +/**
> + * vb2_queue_publish_buffers() - publish buffers staged by __vb2_queue_alloc()
> + * @q:		pointer to &struct vb2_queue with videobuf2 queue.
> + * @start:	index of the first buffer to publish.
> + * @count:	number of buffers to publish.
> + *
> + * Set the @q->bufs_bitmap bits of the @count fully initialized buffers that
> + * __vb2_queue_alloc() stored in @q->bufs[] starting at @start, which makes
> + * them visible to vb2_get_buffer().
> + */
> +static void vb2_queue_publish_buffers(struct vb2_queue *q, unsigned int start,
> +				      unsigned int count)
> +{
> +	unsigned int i;
> +
> +	lockdep_assert_held(&q->mmap_lock);
> +
> +	for (i = start; i < start + count; i++) {
> +		if (WARN_ON(i >= q->max_num_buffers || !q->bufs[i] ||
> +			    test_bit(i, q->bufs_bitmap)))
> +			continue;
> +		set_bit(i, q->bufs_bitmap);
> +	}
>  }
>  
>  /**
> @@ -460,6 +487,11 @@ static void vb2_queue_remove_buffer(struct vb2_buffer *vb)
>   * @first_index: index of the first created buffer, all newly allocated buffers
>   *		 have indices in the range [first_index..first_index+count-1]
>   *
> + * The allocated buffers are stored in q->bufs[] but not published in
> + * q->bufs_bitmap: the caller must either publish them with
> + * vb2_queue_publish_buffers() or free them with __vb2_queue_free(), both under
> + * q->mmap_lock.
> + *
>   * Returns the number of buffers successfully allocated.
>   */
>  static int __vb2_queue_alloc(struct vb2_queue *q, enum vb2_memory memory,
> @@ -562,7 +594,8 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
>  	struct vb2_buffer *vb;
>  
>  	for (i = start; i < start + count; i++) {
> -		vb = vb2_get_buffer(q, i);
> +		/* See __vb2_queue_free() for why vb2_get_buffer() isn't used */
> +		vb = q->bufs[i];
>  		if (!vb)
>  			continue;
>  
> @@ -580,6 +613,11 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int start, unsigned int
>   * __vb2_queue_free() - free @count buffers from @start index of the queue - video memory and
>   * related information, if no buffers are left return the queue to an
>   * uninitialized state. Might be called even if the queue has already been freed.
> + *
> + * Buffers are looked up directly in q->bufs[] instead of through
> + * vb2_get_buffer() so that buffers staged by __vb2_queue_alloc() that were
> + * never published in q->bufs_bitmap (because a later .queue_setup() call
> + * rejected the allocated number of buffers) are freed as well.
>   */
>  static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned int count)
>  {
> @@ -587,9 +625,13 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  
>  	lockdep_assert_held(&q->mmap_lock);
>  
> +	/* q->bufs is NULL until vb2_core_allocated_buffers_storage() runs */
> +	if (!q->bufs)
> +		return;
> +
>  	/* Call driver-provided cleanup function for each buffer, if provided */
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  
>  		if (vb && vb->planes[0].mem_priv)
>  			call_void_vb_qop(vb, buf_cleanup, vb);
> @@ -624,7 +666,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  		q->cnt_unprepare_streaming = 0;
>  	}
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  		bool unbalanced;
>  
>  		if (!vb)
> @@ -676,7 +718,7 @@ static void __vb2_queue_free(struct vb2_queue *q, unsigned int start, unsigned i
>  
>  	/* Free vb2 buffers */
>  	for (i = start; i < start + count; i++) {
> -		struct vb2_buffer *vb = vb2_get_buffer(q, i);
> +		struct vb2_buffer *vb = q->bufs[i];
>  
>  		if (!vb)
>  			continue;
> @@ -1000,14 +1042,16 @@ int vb2_core_reqbufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	if (ret < 0) {
>  		/*
> -		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> -		 * from already queued buffers and it will reset q->memory to
> +		 * Note: the buffers allocated by __vb2_queue_alloc() were never
> +		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
> +		 * straight from q->bufs[] and resets q->memory to
>  		 * VB2_MEMORY_UNKNOWN.
>  		 */
>  		__vb2_queue_free(q, first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return ret;
>  	}
> +	vb2_queue_publish_buffers(q, first_index, allocated_buffers);
>  	mutex_unlock(&q->mmap_lock);
>  
>  	/*
> @@ -1125,14 +1169,16 @@ int vb2_core_create_bufs(struct vb2_queue *q, enum vb2_memory memory,
>  
>  	if (ret < 0) {
>  		/*
> -		 * Note: __vb2_queue_free() will subtract 'allocated_buffers'
> -		 * from already queued buffers and it will reset q->memory to
> -		 * VB2_MEMORY_UNKNOWN.
> +		 * Note: the buffers allocated by __vb2_queue_alloc() were never
> +		 * published in q->bufs_bitmap, __vb2_queue_free() frees them
> +		 * straight from q->bufs[] and resets q->memory to
> +		 * VB2_MEMORY_UNKNOWN if no buffers are left.
>  		 */
>  		__vb2_queue_free(q, *first_index, allocated_buffers);
>  		mutex_unlock(&q->mmap_lock);
>  		return -ENOMEM;
>  	}
> +	vb2_queue_publish_buffers(q, *first_index, allocated_buffers);
>  	mutex_unlock(&q->mmap_lock);
>  
>  	/*
> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
> index 4b4f4c15c53a..2e93e6985d5f 100644
> --- a/include/media/videobuf2-core.h
> +++ b/include/media/videobuf2-core.h
> @@ -564,9 +564,11 @@ struct vb2_buf_ops {
>   * @mmap_lock:	private mutex used when buffers are allocated/freed/mmapped
>   * @memory:	current memory type used
>   * @dma_dir:	DMA mapping direction.
> - * @bufs:	videobuf2 buffer structures. If it is non-NULL then
> - *		bufs_bitmap is also non-NULL.
> - * @bufs_bitmap: bitmap tracking whether each bufs[] entry is used
> + * @bufs:	videobuf2 buffer structures. Must be accessed with
> + *		vb2_get_buffer(), as an allocated buffer cannot be used
> + *		until it is published.


"Drivers must access it through vb2_get_buffer()", otherwise You risk getting
tons of poor-LLM generated patches fixing vb2-core to use vb2_get_buffer()
instead of poking q->bufs[].

> + * @bufs_bitmap: bitmap tracking published buffers. Bits are set and cleared
> + *		under @mmap_lock.
>   * @max_num_buffers: upper limit of number of allocated/used buffers.
>   *		     If set to 0 v4l2 core will change it VB2_MAX_FRAME
>   *		     for backward compatibility.
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland