Videobuf2 corner case fixes

PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)

drivers/media/common/videobuf2/videobuf2-core.c +4 -0

index 0ca81d495bdaf..0234ddbfa4de2 100644
@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
 
+		/* Did it wrap around? */
+		if (size < vb->planes[plane].length)
+			goto free;
+
 		mem_priv = call_ptr_memop(vb, alloc,
 				q->alloc_devs[plane] ? : q->dev,
 				q->dma_attrs, size, q->dma_dir, q->gfp_flags);

PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-core.c index 0ca81d495bdaf..0234ddbfa4de2 100644

Hi Sakari,

Thank you for the patch.

Quoted 18 lines · On Thursday, 13 December 2018 12:40:04 EET Sakari Ailus wrote:

PAGE_ALIGN() may wrap the buffer size around to 0. Prevent this by checking that the aligned value is not smaller than the unaligned one.

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-core.c | 4 ++++
 1 file changed, 4 insertions(+)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-core.c index 0ca81d495bdaf..0234ddbfa4de2 100644

drivers/media/common/videobuf2/videobuf2-core.c static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) · line 216

Quoted 10 lines

drivers/media/common/videobuf2/videobuf2-core.c +4 -0

@@ -207,6 +207,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
 	for (plane = 0; plane < vb->num_planes; ++plane) {
 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
 
+		/* Did it wrap around? */
+		if (size < vb->planes[plane].length)
+			goto free;
+
 		mem_priv = call_ptr_memop(vb, alloc,
 				q->alloc_devs[plane] ? : q->dev,
 				q->dma_attrs, size, q->dma_dir, q->gfp_flags);

Wouldn't it be better to reject length > INT_MAX (or some variations of that) a few steps before, for instance just before calling __vb2_queue_alloc() ? There's already a check in vb2_core_reqbufs():

        /* Check that driver has set sane values */
        if (WARN_ON(!num_planes))
                return -EINVAL;

        for (i = 0; i < num_planes; i++)
                if (WARN_ON(!plane_sizes[i]))
                        return -EINVAL;

It could be extended to validate the sizes against wrap-around, and moved to a separate function to be called in vb2_core_create_bufs() as well (as those checks are missing there). Alternatively, the checks could be moved to the beginning of __vb2_queue_alloc().

buf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.

Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/media/common/videobuf2/videobuf2-dma-sg.c +1 -1

index 015e737095cdd..e9bfea986cc47 100644
@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct vb2_dma_sg_buf *buf,
 		gfp_t gfp_flags)
 {
 	unsigned int last_page = 0;
-	int size = buf->size;
+	unsigned long size = buf->size;
 
 	while (size > 0) {
 		struct page *pages;

buf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.

Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cdd..e9bfea986cc47 100644

drivers/media/common/videobuf2/videobuf2-dma-sg.c +1 -1

@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
 vb2_dma_sg_buf *buf, gfp_t gfp_flags)
 {
 	unsigned int last_page = 0;
-	int size = buf->size;
+	unsigned long size = buf->size;
 
 	while (size > 0) {
 		struct page *pages;

Hi Sakari,

Thank you for the patch.

Quoted 3 lines · On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:

buf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.

Quoted 3 lines

Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.

This feels a bit fragile to me. We at least need a big comment in the code to explain this. Another option would be a size -= min(..., size) just to make sure.

Quoted 24 lines

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cdd..e9bfea986cc47 100644

drivers/media/common/videobuf2/videobuf2-dma-sg.c +1 -1

@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
 vb2_dma_sg_buf *buf, gfp_t gfp_flags)
 {
 	unsigned int last_page = 0;
-	int size = buf->size;
+	unsigned long size = buf->size;
 
 	while (size > 0) {
 		struct page *pages;

Hi Laurent,

Quoted 8 lines · Laurent Pinchart

Hi Sakari,

Thank you for the patch.

Quoted 3 lines · On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:

buf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.

Quoted 7 lines

Quoted 3 lines

Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.

This feels a bit fragile to me. We at least need a big comment in the code to explain this. Another option would be a size -= min(..., size) just to make sure.

I was thinking of something like:

	if (WARN_ON(size & ~PAGE_MASK))
		return -ENOMEM;

But I opted to writing the third patch as this is not the only place where the page alignment could be relevant.

What do you think?

Quoted 29 lines

Quoted 29 lines

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---

 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cdd..e9bfea986cc47 100644

drivers/media/common/videobuf2/videobuf2-dma-sg.c +1 -1

@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
 vb2_dma_sg_buf *buf, gfp_t gfp_flags)
 
 {
 
 	unsigned int last_page = 0;
 
-	int size = buf->size;
+	unsigned long size = buf->size;

drivers/media/common/videobuf2/videobuf2-dma-sg.c +0 -0

 	while (size > 0) {
 
 		struct page *pages;

Quoted 24 lines · On Thursday, 13 December 2018 15:00:23 EET Sakari Ailus wrote:

Hi Laurent,

Quoted 8 lines · Laurent Pinchart

Hi Sakari,

Thank you for the patch.

Quoted 3 lines · On Thursday, 13 December 2018 12:40:05 EET Sakari Ailus wrote:

buf->size is an unsigned long; casting that to int will lead to an overflow if buf->size exceeds INT_MAX.

Quoted 7 lines

Quoted 3 lines

Fix this by changing the type to unsigned long instead. This is possible as the buf->size is always aligned to PAGE_SIZE, and therefore the size will never have values lesser than 0.

This feels a bit fragile to me. We at least need a big comment in the code to explain this. Another option would be a size -= min(..., size) just to make sure.

I was thinking of something like:

	if (WARN_ON(size & ~PAGE_MASK))
		return -ENOMEM;

Quoted 4 lines

But I opted to writing the third patch as this is not the only place where the page alignment could be relevant.

What do you think?

I'd do both :-)

Quoted 30 lines

Quoted 30 lines

Quoted 30 lines

Note on backporting to stable: the file used to be under drivers/media/v4l2-core, it was moved to the current location after 4.14.

Signed-off-by Sakari Ailus <[email protected]>

Cc [email protected]

---

 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(quoted diff) +0 -0

b/drivers/media/common/videobuf2/videobuf2-dma-sg.c index 015e737095cdd..e9bfea986cc47 100644

drivers/media/common/videobuf2/videobuf2-dma-sg.c +1 -1

@@ -59,7 +59,7 @@ static int vb2_dma_sg_alloc_compacted(struct
 vb2_dma_sg_buf *buf, gfp_t gfp_flags)
 
 {
 
 	unsigned int last_page = 0;
 
-	int size = buf->size;
+	unsigned long size = buf->size;

drivers/media/common/videobuf2/videobuf2-dma-sg.c +0 -0

 	while (size > 0) {
 
 		struct page *pages;

Signature · 3 lines

Regards,

Laurent Pinchart

The size argument of the alloc memop, which allocates buffer memory, is page aligned. Document it as such, as code elsewhere has not taken this into account.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/media/videobuf2-core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

include/media/videobuf2-core.h +2 -1

index e86981d615ae4..68b9fe660e4f1 100644
@@ -54,7 +54,8 @@ struct vb2_threadio_data;
  *		will then be passed as @buf_priv argument to other ops in this
  *		structure. Additional gfp_flags to use when allocating the
  *		are also passed to this operation. These flags are from the
- *		gfp_flags field of vb2_queue.
+ *		gfp_flags field of vb2_queue. The size argument to this function
+ *		shall be *page aligned*.
  * @put:	inform the allocator that the buffer will no longer be used;
  *		usually will result in the allocator freeing the buffer (if
  *		no other users of this buffer are present); the @buf_priv

The size argument of the alloc memop, which allocates buffer memory, is page aligned. Document it as such, as code elsewhere has not taken this into account.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/media/videobuf2-core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

include/media/videobuf2-core.h +0 -0

index e86981d615ae4..68b9fe660e4f1 100644

Hi Sakari,

Thank you for the patch.

Quoted 9 lines · On Thursday, 13 December 2018 12:40:06 EET Sakari Ailus wrote:

The size argument of the alloc memop, which allocates buffer memory, is page aligned. Document it as such, as code elsewhere has not taken this into account.

Signed-off-by Sakari Ailus <[email protected]>

---
 include/media/videobuf2-core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Quoted 5 lines

include/media/videobuf2-core.h +0 -0

index e86981d615ae4..68b9fe660e4f1 100644

include/media/videobuf2-core.h struct vb2_threadio_data; · line 61

Quoted 9 lines

include/media/videobuf2-core.h +2 -1

@@ -54,7 +54,8 @@ struct vb2_threadio_data;
  *		will then be passed as @buf_priv argument to other ops in this
  *		structure. Additional gfp_flags to use when allocating the
  *		are also passed to this operation. These flags are from the
- *		gfp_flags field of vb2_queue.
+ *		gfp_flags field of vb2_queue. The size argument to this function
+ *		shall be *page aligned*.
  * @put:	inform the allocator that the buffer will no longer be used;
  *		usually will result in the allocator freeing the buffer (if
  *		no other users of this buffer are present); the @buf_priv

I wonder if a WARN_ON() to ensure this would make sense. In any case,

Reviewed-by Laurent Pinchart <[email protected]>

Quoted 20 lines · Laurent Pinchart

Quoted 19 lines · On Thursday, 13 December 2018 12:40:06 EET Sakari Ailus wrote:

The size argument of the alloc memop, which allocates buffer memory, is page aligned. Document it as such, as code elsewhere has not taken this into account.

Signed-off-by Sakari Ailus <[email protected]>

---

 include/media/videobuf2-core.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

(quoted diff) +0 -0

b/include/media/videobuf2-core.h index e86981d615ae4..68b9fe660e4f1 100644

include/media/videobuf2-core.h +0 -0

@@ -54,7 +54,8 @@ struct vb2_threadio_data;
  *		will then be passed as @buf_priv argument to other ops in this
  *		structure. Additional gfp_flags to use when allocating the
  *		are also passed to this operation. These flags are from the

Quoted 8 lines

include/media/videobuf2-core.h struct vb2_threadio_data; · line 61

Quoted 6 lines

include/media/videobuf2-core.h +2 -1

- *		gfp_flags field of vb2_queue.
+ *		gfp_flags field of vb2_queue. The size argument to this function
+ *		shall be *page aligned*.
  * @put:	inform the allocator that the buffer will no longer be used;
  *		usually will result in the allocator freeing the buffer (if
  *		no other users of this buffer are present); the @buf_priv

I wonder if a WARN_ON() to ensure this would make sense. In any case,

There's a single place where the alloc() op is called. I thought it'd be silly to put a check after the line of code that performs the alignment.

Perhaps a comment right there?

I'm open to other ideas that don't seem silly. :-)

Quoted 1 line

Reviewed-by Laurent Pinchart <[email protected]>