The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org --- drivers/media/common/videobuf2/videobuf2-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02281d13505f..543da515c761 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) { + unsigned long size = vb->planes[plane].length; + /* Memops alloc requires size to be page aligned. */ - unsigned long size = PAGE_ALIGN(vb->planes[plane].length); + size = PAGE_ALIGN(size);
/* Did it wrap around? */ if (size < vb->planes[plane].length)
On (21/03/10 00:43), Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Hi Ricardo,
@@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;- /* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size);/* Did it wrap around? */ if (size < vb->planes[plane].length)
Shouldn't the same be done in vb2_mmap()?
-ss
Hi Sergey
On Wed, Mar 10, 2021 at 2:42 AM Sergey Senozhatsky sergey.senozhatsky.work@gmail.com wrote:
On (21/03/10 00:43), Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Hi Ricardo,
@@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;/* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size); /* Did it wrap around? */ if (size < vb->planes[plane].length)Shouldn't the same be done in vb2_mmap()?
Indeed, I was having tunnel vision focussing on my issue.
I have sent a new patch.
Thanks!
-ss
-- Ricardo Ribalda
On (21/03/10 00:43), Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
Reviewed-by: Sergey Senozhatsky senozhatsky@chromium.org
Hi Ricardo,
Thank you for the patch.
On Wed, Mar 10, 2021 at 12:43:17AM +0100, Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/common/videobuf2/videobuf2-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02281d13505f..543da515c761 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;
unsigned long is still 32-bit on 32-bit platforms.
- /* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size);/* Did it wrap around? */ if (size < vb->planes[plane].length)
Doesn't this address the issue already ?
Hi Laurent
On Wed, Mar 10, 2021 at 8:49 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
Thank you for the patch.
Thank you!
On Wed, Mar 10, 2021 at 12:43:17AM +0100, Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/common/videobuf2/videobuf2-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02281d13505f..543da515c761 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;unsigned long is still 32-bit on 32-bit platforms.
/* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size); /* Did it wrap around? */ if (size < vb->planes[plane].length)Doesn't this address the issue already ?
Yes and no. If you need to allocate 0xffffffff you are still affected by the underrun. The core will return an error instead of doing the allocation.
(yes, I know it is a lot of memory for a buffer)
-- Regards,
Laurent Pinchart
Hi Ricardo,
On Wed, Mar 10, 2021 at 08:58:39AM +0100, Ricardo Ribalda wrote:
On Wed, Mar 10, 2021 at 8:49 AM Laurent Pinchart wrote:
On Wed, Mar 10, 2021 at 12:43:17AM +0100, Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/common/videobuf2/videobuf2-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02281d13505f..543da515c761 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;unsigned long is still 32-bit on 32-bit platforms.
/* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size); /* Did it wrap around? */ if (size < vb->planes[plane].length)Doesn't this address the issue already ?
Yes and no. If you need to allocate 0xffffffff you are still affected by the underrun. The core will return an error instead of doing the allocation.
(yes, I know it is a lot of memory for a buffer)
That's my point, I don't think there's a need for this :-) Especially with v4l2_buffer.m.offset being a __u32, we are limited to 4GB for *all* buffers.
Hi Laurent
On Wed, Mar 10, 2021 at 9:12 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ricardo,
On Wed, Mar 10, 2021 at 08:58:39AM +0100, Ricardo Ribalda wrote:
On Wed, Mar 10, 2021 at 8:49 AM Laurent Pinchart wrote:
On Wed, Mar 10, 2021 at 12:43:17AM +0100, Ricardo Ribalda wrote:
The plane_length is an unsigned integer. So, if we have a size of 0xffffffff bytes we incorrectly allocate 0 bytes instead of 1 << 32.
Cc: stable@vger.kernel.org Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap") Signed-off-by: Ricardo Ribalda ribalda@chromium.org
drivers/media/common/videobuf2/videobuf2-core.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c index 02281d13505f..543da515c761 100644 --- a/drivers/media/common/videobuf2/videobuf2-core.c +++ b/drivers/media/common/videobuf2/videobuf2-core.c @@ -223,8 +223,10 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) * NOTE: mmapped areas should be page aligned */ for (plane = 0; plane < vb->num_planes; ++plane) {
unsigned long size = vb->planes[plane].length;unsigned long is still 32-bit on 32-bit platforms.
/* Memops alloc requires size to be page aligned. */
unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
size = PAGE_ALIGN(size); /* Did it wrap around? */ if (size < vb->planes[plane].length)Doesn't this address the issue already ?
Yes and no. If you need to allocate 0xffffffff you are still affected by the underrun. The core will return an error instead of doing the allocation.
(yes, I know it is a lot of memory for a buffer)
That's my point, I don't think there's a need for this :-) Especially with v4l2_buffer.m.offset being a __u32, we are limited to 4GB for *all* buffers.
I guess I will convert this patch into a documentation patch, so we explicitly know the limit of the API (1<<32 - PAGE_SIZE).
Thanks!
-- Regards,
Laurent Pinchart
linux-stable-mirror@lists.linaro.org
-
Laurent Pinchart -
Ricardo Ribalda -
Sergey Senozhatsky