diff mbox

[PATCHv10,22/26] v4l: vb2-dma-contig: fail if user ptr buffer is not correctly aligned

Message ID 1349880405-26049-23-git-send-email-t.stanislaws@samsung.com
State Accepted
Commit d81e870d5afa1b0a95ea94c4052d3c7e973fae8c
Headers show

Commit Message

Tomasz Stanislawski Oct. 10, 2012, 2:46 p.m. UTC
From: Marek Szyprowski <m.szyprowski@samsung.com>

The DMA transfer must be aligned to a specific value. If userptr is not aligned
to DMA requirements then unexpected corruptions of the memory may occur before
or after a buffer.  To prevent such situations, all unligned userptr buffers
are rejected at VIDIOC_QBUF.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/videobuf2-dma-contig.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

Comments

Laurent Pinchart Oct. 11, 2012, 9:36 p.m. UTC | #1
Hi Tomasz,

Thanks for the patch.

On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
> From: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> The DMA transfer must be aligned to a specific value. If userptr is not
> aligned to DMA requirements then unexpected corruptions of the memory may
> occur before or after a buffer.  To prevent such situations, all unligned
> userptr buffers are rejected at VIDIOC_QBUF.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
>  drivers/media/v4l2-core/videobuf2-dma-contig.c |   12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 2d661fd..571a919
> 100644
> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> @@ -493,6 +493,18 @@ static void *vb2_dc_get_userptr(void *alloc_ctx,
> unsigned long vaddr, struct vm_area_struct *vma;
>  	struct sg_table *sgt;
>  	unsigned long contig_size;
> +	unsigned long dma_align = dma_get_cache_alignment();
> +
> +	/* Only cache aligned DMA transfers are reliable */
> +	if (!IS_ALIGNED(vaddr | size, dma_align)) {
> +		pr_debug("user data must be aligned to %lu bytes\n", dma_align);
> +		return ERR_PTR(-EINVAL);
> +	}

Looks good to me.

> +	if (!size) {
> +		pr_debug("size is zero\n");
> +		return ERR_PTR(-EINVAL);
> +	}

Can this happen ? The vb2 core already has

                /* Check if the provided plane buffer is large enough */
                if (planes[plane].length < q->plane_sizes[plane]) {
                        ret = -EINVAL;
                        goto err;
                }

Unless queue_setup sets plane_sizes to 0 we can't reach vb2_dc_get_userptr.

>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>  	if (!buf)
Tomasz Stanislawski Oct. 12, 2012, 7:44 a.m. UTC | #2
Hi Laurent,
Thank you for the review.
Please refer to the comments below.

On 10/11/2012 11:36 PM, Laurent Pinchart wrote:
> Hi Tomasz,
> 
> Thanks for the patch.
> 
> On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
>> From: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>> The DMA transfer must be aligned to a specific value. If userptr is not
>> aligned to DMA requirements then unexpected corruptions of the memory may
>> occur before or after a buffer.  To prevent such situations, all unligned
>> userptr buffers are rejected at VIDIOC_QBUF.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-dma-contig.c |   12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 2d661fd..571a919
>> 100644
>> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
>> @@ -493,6 +493,18 @@ static void *vb2_dc_get_userptr(void *alloc_ctx,
>> unsigned long vaddr, struct vm_area_struct *vma;
>>  	struct sg_table *sgt;
>>  	unsigned long contig_size;
>> +	unsigned long dma_align = dma_get_cache_alignment();
>> +
>> +	/* Only cache aligned DMA transfers are reliable */
>> +	if (!IS_ALIGNED(vaddr | size, dma_align)) {
>> +		pr_debug("user data must be aligned to %lu bytes\n", dma_align);
>> +		return ERR_PTR(-EINVAL);
>> +	}
> 
> Looks good to me.
> 
>> +	if (!size) {
>> +		pr_debug("size is zero\n");
>> +		return ERR_PTR(-EINVAL);
>> +	}
> 
> Can this happen ? The vb2 core already has
> 
>                 /* Check if the provided plane buffer is large enough */
>                 if (planes[plane].length < q->plane_sizes[plane]) {
>                         ret = -EINVAL;
>                         goto err;
>                 }
> 
> Unless queue_setup sets plane_sizes to 0 we can't reach vb2_dc_get_userptr.
> 

Yes.. unfortunately, some drivers set plane_size to 0 at queue_setup.
Especially, if REQBUFS is called before any S_FMT.
Maybe it is just a driver bug.

However, VB2 makes no sanity check if plane_sizes[] is zero.
I was not able to find in Documentation nor code comments
any explicit statement that plane_size cannot be zero.

Therefore I have to reject reject a 0-bytes-long user pointer
at vb2_dc_get_userptr before creating an empty scatterlist
and passing it to the DMA layer.

Regards,
Tomasz Stanislawski

>>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
>>  	if (!buf)
>
Hans Verkuil Oct. 12, 2012, 7:58 a.m. UTC | #3
On Fri 12 October 2012 09:44:05 Tomasz Stanislawski wrote:
> Hi Laurent,
> Thank you for the review.
> Please refer to the comments below.
> 
> On 10/11/2012 11:36 PM, Laurent Pinchart wrote:
> > Hi Tomasz,
> > 
> > Thanks for the patch.
> > 
> > On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
> >> From: Marek Szyprowski <m.szyprowski@samsung.com>
> >>
> >> The DMA transfer must be aligned to a specific value. If userptr is not
> >> aligned to DMA requirements then unexpected corruptions of the memory may
> >> occur before or after a buffer.  To prevent such situations, all unligned
> >> userptr buffers are rejected at VIDIOC_QBUF.
> >>
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >>
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 2d661fd..571a919
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -493,6 +493,18 @@ static void *vb2_dc_get_userptr(void *alloc_ctx,
> >> unsigned long vaddr, struct vm_area_struct *vma;
> >>  	struct sg_table *sgt;
> >>  	unsigned long contig_size;
> >> +	unsigned long dma_align = dma_get_cache_alignment();
> >> +
> >> +	/* Only cache aligned DMA transfers are reliable */
> >> +	if (!IS_ALIGNED(vaddr | size, dma_align)) {
> >> +		pr_debug("user data must be aligned to %lu bytes\n", dma_align);
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> > 
> > Looks good to me.
> > 
> >> +	if (!size) {
> >> +		pr_debug("size is zero\n");
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> > 
> > Can this happen ? The vb2 core already has
> > 
> >                 /* Check if the provided plane buffer is large enough */
> >                 if (planes[plane].length < q->plane_sizes[plane]) {
> >                         ret = -EINVAL;
> >                         goto err;
> >                 }
> > 
> > Unless queue_setup sets plane_sizes to 0 we can't reach vb2_dc_get_userptr.
> > 
> 
> Yes.. unfortunately, some drivers set plane_size to 0 at queue_setup.
> Especially, if REQBUFS is called before any S_FMT.
> Maybe it is just a driver bug.

That's a driver bug. Planes with size 0 make no sense whatsoever. vb2 should
WARN_ON on that and return an error.

My guess is that these drivers do not set up a default format as they should.

Regards,

	Hans

> However, VB2 makes no sanity check if plane_sizes[] is zero.
> I was not able to find in Documentation nor code comments
> any explicit statement that plane_size cannot be zero.
> 
> Therefore I have to reject reject a 0-bytes-long user pointer
> at vb2_dc_get_userptr before creating an empty scatterlist
> and passing it to the DMA layer.
> 
> Regards,
> Tomasz Stanislawski
> 
> >>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
> >>  	if (!buf)
> > 
>
Laurent Pinchart Oct. 12, 2012, 9:34 a.m. UTC | #4
Hi Tomasz,

On Friday 12 October 2012 09:44:05 Tomasz Stanislawski wrote:
> On 10/11/2012 11:36 PM, Laurent Pinchart wrote:
> > On Wednesday 10 October 2012 16:46:41 Tomasz Stanislawski wrote:
> >> From: Marek Szyprowski <m.szyprowski@samsung.com>
> >> 
> >> The DMA transfer must be aligned to a specific value. If userptr is not
> >> aligned to DMA requirements then unexpected corruptions of the memory may
> >> occur before or after a buffer.  To prevent such situations, all unligned
> >> userptr buffers are rejected at VIDIOC_QBUF.
> >> 
> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> >> ---
> >> 
> >>  drivers/media/v4l2-core/videobuf2-dma-contig.c |   12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >> 
> >> diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> b/drivers/media/v4l2-core/videobuf2-dma-contig.c index 2d661fd..571a919
> >> 100644
> >> --- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> +++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
> >> @@ -493,6 +493,18 @@ static void *vb2_dc_get_userptr(void *alloc_ctx,
> >> unsigned long vaddr, struct vm_area_struct *vma;
> >> 
> >>  	struct sg_table *sgt;
> >>  	unsigned long contig_size;
> >> 
> >> +	unsigned long dma_align = dma_get_cache_alignment();
> >> +
> >> +	/* Only cache aligned DMA transfers are reliable */
> >> +	if (!IS_ALIGNED(vaddr | size, dma_align)) {
> >> +		pr_debug("user data must be aligned to %lu bytes\n", dma_align);
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> > 
> > Looks good to me.
> > 
> >> +	if (!size) {
> >> +		pr_debug("size is zero\n");
> >> +		return ERR_PTR(-EINVAL);
> >> +	}
> > 
> > Can this happen ? The vb2 core already has
> > 
> >                 /* Check if the provided plane buffer is large enough */
> >                 if (planes[plane].length < q->plane_sizes[plane]) {
> >                 
> >                         ret = -EINVAL;
> >                         goto err;
> >                 
> >                 }
> > 
> > Unless queue_setup sets plane_sizes to 0 we can't reach
> > vb2_dc_get_userptr.
> 
> Yes.. unfortunately, some drivers set plane_size to 0 at queue_setup.
> Especially, if REQBUFS is called before any S_FMT.
> Maybe it is just a driver bug.
> 
> However, VB2 makes no sanity check if plane_sizes[] is zero.
> I was not able to find in Documentation nor code comments
> any explicit statement that plane_size cannot be zero.
> 
> Therefore I have to reject reject a 0-bytes-long user pointer
> at vb2_dc_get_userptr before creating an empty scatterlist
> and passing it to the DMA layer.

Wouldn't it then be better to add the sanity checks in the core ?

> >>  	buf = kzalloc(sizeof *buf, GFP_KERNEL);
> >>  	if (!buf)
diff mbox

Patch

diff --git a/drivers/media/v4l2-core/videobuf2-dma-contig.c b/drivers/media/v4l2-core/videobuf2-dma-contig.c
index 2d661fd..571a919 100644
--- a/drivers/media/v4l2-core/videobuf2-dma-contig.c
+++ b/drivers/media/v4l2-core/videobuf2-dma-contig.c
@@ -493,6 +493,18 @@  static void *vb2_dc_get_userptr(void *alloc_ctx, unsigned long vaddr,
 	struct vm_area_struct *vma;
 	struct sg_table *sgt;
 	unsigned long contig_size;
+	unsigned long dma_align = dma_get_cache_alignment();
+
+	/* Only cache aligned DMA transfers are reliable */
+	if (!IS_ALIGNED(vaddr | size, dma_align)) {
+		pr_debug("user data must be aligned to %lu bytes\n", dma_align);
+		return ERR_PTR(-EINVAL);
+	}
+
+	if (!size) {
+		pr_debug("size is zero\n");
+		return ERR_PTR(-EINVAL);
+	}
 
 	buf = kzalloc(sizeof *buf, GFP_KERNEL);
 	if (!buf)