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.
Suggested-by: Sergey Senozhatsky <[email protected]>
Cc: [email protected]
Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap")
Reviewed-by: Sergey Senozhatsky <[email protected]>
Signed-off-by: Ricardo Ribalda <[email protected]>
---
drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
index 543da515c761..89c8bacb94a7 100644
--- a/drivers/media/common/videobuf2/videobuf2-core.c
+++ b/drivers/media/common/videobuf2/videobuf2-core.c
@@ -2256,7 +2256,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
* The buffer length was page_aligned at __vb2_buf_mem_alloc(),
* so, we need to do the same here.
*/
- length = PAGE_ALIGN(vb->planes[plane].length);
+ length = PAGE_ALIGN((unsigned long)vb->planes[plane].length);
if (length < (vma->vm_end - vma->vm_start)) {
dprintk(q, 1,
"MMAP invalid, as it would overflow buffer length\n");
--
2.30.1.766.gb4fecdf3b7-goog
On Wed, Mar 10, 2021 at 08:51:27AM +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.
>
> Suggested-by: Sergey Senozhatsky <[email protected]>
> Cc: [email protected]
> Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap")
> Reviewed-by: Sergey Senozhatsky <[email protected]>
> Signed-off-by: Ricardo Ribalda <[email protected]>
> ---
> drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> index 543da515c761..89c8bacb94a7 100644
> --- a/drivers/media/common/videobuf2/videobuf2-core.c
> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> @@ -2256,7 +2256,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> * The buffer length was page_aligned at __vb2_buf_mem_alloc(),
> * so, we need to do the same here.
> */
> - length = PAGE_ALIGN(vb->planes[plane].length);
> + length = PAGE_ALIGN((unsigned long)vb->planes[plane].length);
> if (length < (vma->vm_end - vma->vm_start)) {
> dprintk(q, 1,
> "MMAP invalid, as it would overflow buffer length\n");
> --
> 2.30.1.766.gb4fecdf3b7-goog
>
Acked-by: Tomasz Figa <[email protected]>
Best regards,
Tomasz
Hi Tomasz
Thanks for your review!
We finally figured out that we would not like to support allocations
of exactly 4GiB. The rest of the code is not designed for it.
And you will get a descriptive enough error if you overrun (at least
in recent kernels):
if (length < (vma->vm_end - vma->vm_start)) {
Best regards!
On Fri, Jun 4, 2021 at 2:06 PM Tomasz Figa <[email protected]> wrote:
>
> On Wed, Mar 10, 2021 at 08:51:27AM +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.
> >
> > Suggested-by: Sergey Senozhatsky <[email protected]>
> > Cc: [email protected]
> > Fixes: 7f8414594e47 ("[media] media: videobuf2: fix the length check for mmap")
> > Reviewed-by: Sergey Senozhatsky <[email protected]>
> > Signed-off-by: Ricardo Ribalda <[email protected]>
> > ---
> > drivers/media/common/videobuf2/videobuf2-core.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
> > index 543da515c761..89c8bacb94a7 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-core.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-core.c
> > @@ -2256,7 +2256,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> > * The buffer length was page_aligned at __vb2_buf_mem_alloc(),
> > * so, we need to do the same here.
> > */
> > - length = PAGE_ALIGN(vb->planes[plane].length);
> > + length = PAGE_ALIGN((unsigned long)vb->planes[plane].length);
> > if (length < (vma->vm_end - vma->vm_start)) {
> > dprintk(q, 1,
> > "MMAP invalid, as it would overflow buffer length\n");
> > --
> > 2.30.1.766.gb4fecdf3b7-goog
> >
>
> Acked-by: Tomasz Figa <[email protected]>
>
> Best regards,
> Tomasz
--
Ricardo Ribalda