* [PATCH v2] videobuf2-core: Verify planes lengths for output buffers @ 2012-10-16 15:37 Laurent Pinchart 2012-11-09 23:33 ` Pawel Osciak 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2012-10-16 15:37 UTC (permalink / raw) To: linux-media; +Cc: Pawel Osciak, Marek Szyprowski, Kyungmin Park, Hans Verkuil For output buffers application provide to the kernel the number of bytes they stored in each plane of the buffer. Verify that the value is smaller than or equal to the plane length. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> --- drivers/media/v4l2-core/videobuf2-core.c | 39 ++++++++++++++++++++++++++++++ 1 files changed, 39 insertions(+), 0 deletions(-) Changes compared to v1: - Sanity check the data_offset value for each plane. diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -296,6 +296,41 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer } /** + * __verify_length() - Verify that the bytesused value for each plane fits in + * the plane length and that the data offset doesn't exceed the bytesused value. + */ +static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) +{ + unsigned int length; + unsigned int plane; + + if (!V4L2_TYPE_IS_OUTPUT(b->type)) + return 0; + + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { + for (plane = 0; plane < vb->num_planes; ++plane) { + length = (b->memory == V4L2_MEMORY_USERPTR) + ? b->m.planes[plane].length + : vb->v4l2_planes[plane].length; + + if (b->m.planes[plane].bytesused > length) + return -EINVAL; + if (b->m.planes[plane].data_offset >= + b->m.planes[plane].bytesused) + return -EINVAL; + } + } else { + length = (b->memory == V4L2_MEMORY_USERPTR) + ? b->length : vb->v4l2_planes[0].length; + + if (b->bytesused > length) + return -EINVAL; + } + + return 0; +} + +/** * __buffer_in_use() - return true if the buffer is in use and * the queue cannot be freed (by the means of REQBUFS(0)) call */ @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) struct vb2_queue *q = vb->vb2_queue; int ret; + ret = __verify_length(vb, b); + if (ret < 0) + return ret; + switch (q->memory) { case V4L2_MEMORY_MMAP: ret = __qbuf_mmap(vb, b); -- Regards, Laurent Pinchart ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2012-10-16 15:37 [PATCH v2] videobuf2-core: Verify planes lengths for output buffers Laurent Pinchart @ 2012-11-09 23:33 ` Pawel Osciak 2012-11-12 11:35 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Pawel Osciak @ 2012-11-09 23:33 UTC (permalink / raw) To: Laurent Pinchart Cc: linux-media, Marek Szyprowski, Kyungmin Park, Hans Verkuil On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > For output buffers application provide to the kernel the number of bytes > they stored in each plane of the buffer. Verify that the value is > smaller than or equal to the plane length. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > --- Acked-by: Pawel Osciak <pawel@osciak.com> > drivers/media/v4l2-core/videobuf2-core.c | 39 ++++++++++++++++++++++++++++++ > 1 files changed, 39 insertions(+), 0 deletions(-) > > Changes compared to v1: > > - Sanity check the data_offset value for each plane. > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index 432df11..479337d 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -296,6 +296,41 @@ static int __verify_planes_array(struct vb2_buffer *vb, const struct v4l2_buffer > } > > /** > + * __verify_length() - Verify that the bytesused value for each plane fits in > + * the plane length and that the data offset doesn't exceed the bytesused value. > + */ > +static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) > +{ > + unsigned int length; > + unsigned int plane; > + > + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > + return 0; > + > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > + for (plane = 0; plane < vb->num_planes; ++plane) { > + length = (b->memory == V4L2_MEMORY_USERPTR) > + ? b->m.planes[plane].length > + : vb->v4l2_planes[plane].length; > + > + if (b->m.planes[plane].bytesused > length) > + return -EINVAL; > + if (b->m.planes[plane].data_offset >= > + b->m.planes[plane].bytesused) > + return -EINVAL; > + } > + } else { > + length = (b->memory == V4L2_MEMORY_USERPTR) > + ? b->length : vb->v4l2_planes[0].length; > + > + if (b->bytesused > length) > + return -EINVAL; > + } > + > + return 0; > +} > + > +/** > * __buffer_in_use() - return true if the buffer is in use and > * the queue cannot be freed (by the means of REQBUFS(0)) call > */ > @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > struct vb2_queue *q = vb->vb2_queue; > int ret; > > + ret = __verify_length(vb, b); > + if (ret < 0) > + return ret; > + > switch (q->memory) { > case V4L2_MEMORY_MMAP: > ret = __qbuf_mmap(vb, b); > -- > Regards, > > Laurent Pinchart > -- Best regards, Pawel Osciak ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2012-11-09 23:33 ` Pawel Osciak @ 2012-11-12 11:35 ` Laurent Pinchart 2013-08-07 10:44 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2012-11-12 11:35 UTC (permalink / raw) To: Pawel Osciak; +Cc: linux-media, Marek Szyprowski, Kyungmin Park, Hans Verkuil Hi Pawel, On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > > For output buffers application provide to the kernel the number of bytes > > they stored in each plane of the buffer. Verify that the value is > > smaller than or equal to the plane length. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > --- > > Acked-by: Pawel Osciak <pawel@osciak.com> You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. When you ack a videobuf2 patch, should we assume that you will take it in your git tree ? > > drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++++++++ > > 1 files changed, 39 insertions(+), 0 deletions(-) > > > > Changes compared to v1: > > > > - Sanity check the data_offset value for each plane. > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d 100644 > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > @@ -296,6 +296,41 @@ static int __verify_planes_array(struct vb2_buffer > > *vb, const struct v4l2_buffer> > > } > > > > /** > > > > + * __verify_length() - Verify that the bytesused value for each plane > > fits in + * the plane length and that the data offset doesn't exceed the > > bytesused value. + */ > > +static int __verify_length(struct vb2_buffer *vb, const struct > > v4l2_buffer *b) +{ > > + unsigned int length; > > + unsigned int plane; > > + > > + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > > + return 0; > > + > > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > + ? b->m.planes[plane].length > > + : vb->v4l2_planes[plane].length; > > + > > + if (b->m.planes[plane].bytesused > length) > > + return -EINVAL; > > + if (b->m.planes[plane].data_offset >= > > + b->m.planes[plane].bytesused) > > + return -EINVAL; > > + } > > + } else { > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > + ? b->length : vb->v4l2_planes[0].length; > > + > > + if (b->bytesused > length) > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +/** > > * __buffer_in_use() - return true if the buffer is in use and > > * the queue cannot be freed (by the means of REQBUFS(0)) call > > */ > > @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer *vb, > > const struct v4l2_buffer *b)> > > struct vb2_queue *q = vb->vb2_queue; > > int ret; > > > > + ret = __verify_length(vb, b); > > + if (ret < 0) > > + return ret; > > + > > switch (q->memory) { > > case V4L2_MEMORY_MMAP: > > ret = __qbuf_mmap(vb, b); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2012-11-12 11:35 ` Laurent Pinchart @ 2013-08-07 10:44 ` Laurent Pinchart 2013-08-08 12:14 ` Marek Szyprowski 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2013-08-07 10:44 UTC (permalink / raw) To: Pawel Osciak Cc: linux-media, Marek Szyprowski, Kyungmin Park, Hans Verkuil, Mauro Carvalho Chehab On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > > On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > > > For output buffers application provide to the kernel the number of bytes > > > they stored in each plane of the buffer. Verify that the value is > > > smaller than or equal to the plane length. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > > --- > > > > Acked-by: Pawel Osciak <pawel@osciak.com> > > You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. When > you ack a videobuf2 patch, should we assume that you will take it in your > git tree ? Ping ? I'd like to get this patch in v3.12, should I send a pull request ? > > > drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++++++ > > > 1 files changed, 39 insertions(+), 0 deletions(-) > > > > > > Changes compared to v1: > > > > > > - Sanity check the data_offset value for each plane. > > > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > > b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d 100644 > > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > > @@ -296,6 +296,41 @@ static int __verify_planes_array(struct vb2_buffer > > > *vb, const struct v4l2_buffer> > > > > > > } > > > > > > /** > > > + * __verify_length() - Verify that the bytesused value for each plane > > > fits in > > > + * the plane length and that the data offset doesn't exceed the > > > bytesused value. > > > + */ > > > +static int __verify_length(struct vb2_buffer *vb, const struct > > > v4l2_buffer *b) > > > +{ > > > + unsigned int length; > > > + unsigned int plane; > > > + > > > + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > > > + return 0; > > > + > > > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > + ? b->m.planes[plane].length > > > + : vb->v4l2_planes[plane].length; > > > + > > > + if (b->m.planes[plane].bytesused > length) > > > + return -EINVAL; > > > + if (b->m.planes[plane].data_offset >= > > > + b->m.planes[plane].bytesused) > > > + return -EINVAL; > > > + } > > > + } else { > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > + ? b->length : vb->v4l2_planes[0].length; > > > + > > > + if (b->bytesused > length) > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +/** > > > * __buffer_in_use() - return true if the buffer is in use and > > > * the queue cannot be freed (by the means of REQBUFS(0)) call > > > */ > > > @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer *vb, > > > const struct v4l2_buffer *b)> > > > struct vb2_queue *q = vb->vb2_queue; > > > int ret; > > > > > > + ret = __verify_length(vb, b); > > > + if (ret < 0) > > > + return ret; > > > + > > > switch (q->memory) { > > > case V4L2_MEMORY_MMAP: > > > ret = __qbuf_mmap(vb, b); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-07 10:44 ` Laurent Pinchart @ 2013-08-08 12:14 ` Marek Szyprowski 2013-08-08 12:35 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Marek Szyprowski @ 2013-08-08 12:14 UTC (permalink / raw) To: Laurent Pinchart Cc: Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil, Mauro Carvalho Chehab On 8/7/2013 12:44 PM, Laurent Pinchart wrote: > On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > > On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > > > On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > > > > For output buffers application provide to the kernel the number of bytes > > > > they stored in each plane of the buffer. Verify that the value is > > > > smaller than or equal to the plane length. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > > > --- > > > > > > Acked-by: Pawel Osciak <pawel@osciak.com> > > > > You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. When > > you ack a videobuf2 patch, should we assume that you will take it in your > > git tree ? > > Ping ? I'd like to get this patch in v3.12, should I send a pull request ? Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> Feel free to include it in your pull-request. I'm sorry for so huge delay in my response. > > > > drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++++++ > > > > 1 files changed, 39 insertions(+), 0 deletions(-) > > > > > > > > Changes compared to v1: > > > > > > > > - Sanity check the data_offset value for each plane. > > > > > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > > > b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d 100644 > > > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > > > @@ -296,6 +296,41 @@ static int __verify_planes_array(struct vb2_buffer > > > > *vb, const struct v4l2_buffer> > > > > > > > > } > > > > > > > > /** > > > > + * __verify_length() - Verify that the bytesused value for each plane > > > > fits in > > > > + * the plane length and that the data offset doesn't exceed the > > > > bytesused value. > > > > + */ > > > > +static int __verify_length(struct vb2_buffer *vb, const struct > > > > v4l2_buffer *b) > > > > +{ > > > > + unsigned int length; > > > > + unsigned int plane; > > > > + > > > > + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > > > > + return 0; > > > > + > > > > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > > + ? b->m.planes[plane].length > > > > + : vb->v4l2_planes[plane].length; > > > > + > > > > + if (b->m.planes[plane].bytesused > length) > > > > + return -EINVAL; > > > > + if (b->m.planes[plane].data_offset >= > > > > + b->m.planes[plane].bytesused) > > > > + return -EINVAL; > > > > + } > > > > + } else { > > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > > + ? b->length : vb->v4l2_planes[0].length; > > > > + > > > > + if (b->bytesused > length) > > > > + return -EINVAL; > > > > + } > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +/** > > > > * __buffer_in_use() - return true if the buffer is in use and > > > > * the queue cannot be freed (by the means of REQBUFS(0)) call > > > > */ > > > > @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer *vb, > > > > const struct v4l2_buffer *b)> > > > > struct vb2_queue *q = vb->vb2_queue; > > > > int ret; > > > > > > > > + ret = __verify_length(vb, b); > > > > + if (ret < 0) > > > > + return ret; > > > > + > > > > switch (q->memory) { > > > > case V4L2_MEMORY_MMAP: > > > > ret = __qbuf_mmap(vb, b); > Best regards -- Marek Szyprowski Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-08 12:14 ` Marek Szyprowski @ 2013-08-08 12:35 ` Laurent Pinchart 2013-08-26 13:55 ` Sylwester Nawrocki 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2013-08-08 12:35 UTC (permalink / raw) To: Marek Szyprowski Cc: Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil, Mauro Carvalho Chehab Hi Marek, On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: > On 8/7/2013 12:44 PM, Laurent Pinchart wrote: > > On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > > > On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > > > > On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > > > > > For output buffers application provide to the kernel the number of > > > > > bytes they stored in each plane of the buffer. Verify that the value > > > > > is smaller than or equal to the plane length. > > > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > > > > --- > > > > > > > > Acked-by: Pawel Osciak <pawel@osciak.com> > > > > > > You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. > > > When you ack a videobuf2 patch, should we assume that you will take it > > > in your git tree ? > > > > Ping ? I'd like to get this patch in v3.12, should I send a pull request ? > > Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Feel free to include it in your pull-request. I'm sorry for so huge > delay in my response. No worries. I'll send a pull request to Mauro. > > > > > drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++ > > > > > 1 files changed, 39 insertions(+), 0 deletions(-) > > > > > > > > > > Changes compared to v1: > > > > > > > > > > - Sanity check the data_offset value for each plane. > > > > > > > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > > > > b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d > > > > > 100644 > > > > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > > > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > > > > @@ -296,6 +296,41 @@ static int __verify_planes_array(struct > > > > > vb2_buffer > > > > > *vb, const struct v4l2_buffer> > > > > > > > > > > } > > > > > > > > > > /** > > > > > > > > > > + * __verify_length() - Verify that the bytesused value for each > > > > > plane > > > > > fits in > > > > > + * the plane length and that the data offset doesn't exceed the > > > > > bytesused value. > > > > > + */ > > > > > +static int __verify_length(struct vb2_buffer *vb, const struct > > > > > v4l2_buffer *b) > > > > > +{ > > > > > + unsigned int length; > > > > > + unsigned int plane; > > > > > + > > > > > + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > > > > > + return 0; > > > > > + > > > > > + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > > > > + for (plane = 0; plane < vb->num_planes; ++plane) { > > > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > > > + ? b->m.planes[plane].length > > > > > + : vb->v4l2_planes[plane].length; > > > > > + > > > > > + if (b->m.planes[plane].bytesused > length) > > > > > + return -EINVAL; > > > > > + if (b->m.planes[plane].data_offset >= > > > > > + b->m.planes[plane].bytesused) > > > > > + return -EINVAL; > > > > > + } > > > > > + } else { > > > > > + length = (b->memory == V4L2_MEMORY_USERPTR) > > > > > + ? b->length : vb->v4l2_planes[0].length; > > > > > + > > > > > + if (b->bytesused > length) > > > > > + return -EINVAL; > > > > > + } > > > > > + > > > > > + return 0; > > > > > +} > > > > > + > > > > > +/** > > > > > > > > > > * __buffer_in_use() - return true if the buffer is in use and > > > > > * the queue cannot be freed (by the means of REQBUFS(0)) call > > > > > */ > > > > > > > > > > @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer > > > > > *vb, > > > > > const struct v4l2_buffer *b)> > > > > > > > > > > struct vb2_queue *q = vb->vb2_queue; > > > > > int ret; > > > > > > > > > > + ret = __verify_length(vb, b); > > > > > + if (ret < 0) > > > > > + return ret; > > > > > + > > > > > > > > > > switch (q->memory) { > > > > > > > > > > case V4L2_MEMORY_MMAP: > > > > > ret = __qbuf_mmap(vb, b); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-08 12:35 ` Laurent Pinchart @ 2013-08-26 13:55 ` Sylwester Nawrocki 2013-08-26 14:32 ` Mauro Carvalho Chehab 0 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2013-08-26 13:55 UTC (permalink / raw) To: Laurent Pinchart Cc: Marek Szyprowski, Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil, Mauro Carvalho Chehab Hi All, On 08/08/2013 02:35 PM, Laurent Pinchart wrote: > Hi Marek, > > On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: >> On 8/7/2013 12:44 PM, Laurent Pinchart wrote: >>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: >>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: >>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: >>>>>> For output buffers application provide to the kernel the number of >>>>>> bytes they stored in each plane of the buffer. Verify that the value >>>>>> is smaller than or equal to the plane length. >>>>>> >>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >>>>>> --- >>>>> >>>>> Acked-by: Pawel Osciak <pawel@osciak.com> >>>> >>>> You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. >>>> When you ack a videobuf2 patch, should we assume that you will take it >>>> in your git tree ? >>> >>> Ping ? I'd like to get this patch in v3.12, should I send a pull request ? >> >> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> >> >> Feel free to include it in your pull-request. I'm sorry for so huge >> delay in my response. > > No worries. I'll send a pull request to Mauro. > >>>>>> drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++ >>>>>> 1 files changed, 39 insertions(+), 0 deletions(-) >>>>>> >>>>>> Changes compared to v1: >>>>>> >>>>>> - Sanity check the data_offset value for each plane. >>>>>> >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d >>>>>> 100644 >>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct >>>>>> vb2_buffer >>>>>> *vb, const struct v4l2_buffer> >>>>>> >>>>>> } >>>>>> >>>>>> /** >>>>>> >>>>>> + * __verify_length() - Verify that the bytesused value for each >>>>>> plane >>>>>> fits in >>>>>> + * the plane length and that the data offset doesn't exceed the >>>>>> bytesused value. >>>>>> + */ >>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct >>>>>> v4l2_buffer *b) >>>>>> +{ >>>>>> + unsigned int length; >>>>>> + unsigned int plane; >>>>>> + >>>>>> + if (!V4L2_TYPE_IS_OUTPUT(b->type)) >>>>>> + return 0; >>>>>> + >>>>>> + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { >>>>>> + for (plane = 0; plane < vb->num_planes; ++plane) { >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) >>>>>> + ? b->m.planes[plane].length >>>>>> + : vb->v4l2_planes[plane].length; >>>>>> + >>>>>> + if (b->m.planes[plane].bytesused > length) >>>>>> + return -EINVAL; >>>>>> + if (b->m.planes[plane].data_offset >= >>>>>> + b->m.planes[plane].bytesused) >>>>>> + return -EINVAL; This patch causes regressions. After kernel upgrade applications that zero the planes array and don't set bytesused will stop working. We could say that these are buggy applications, but if it has been allowed for several kernel releases failing VIDIOC_QBUF on this check now is plainly a regression IMO. I guess Linus wouldn't be happy about a change like this. With this patch it is no longer possible to queue a buffer with bytesused set to 0. I think it shouldn't be disallowed to queue a buffer with no data to be used. So the check should likely be instead: if (b->m.planes[plane].bytesused > 0 && b->m.planes[plane].data_offset >= b->m.planes[plane].bytesused) return -EINVAL; Sorry for the late review of this. >>>>>> + } >>>>>> + } else { >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) >>>>>> + ? b->length : vb->v4l2_planes[0].length; >>>>>> + >>>>>> + if (b->bytesused > length) >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> >>>>>> * __buffer_in_use() - return true if the buffer is in use and >>>>>> * the queue cannot be freed (by the means of REQBUFS(0)) call >>>>>> */ >>>>>> >>>>>> @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer >>>>>> *vb, >>>>>> const struct v4l2_buffer *b)> >>>>>> >>>>>> struct vb2_queue *q = vb->vb2_queue; >>>>>> int ret; >>>>>> >>>>>> + ret = __verify_length(vb, b); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> >>>>>> switch (q->memory) { >>>>>> >>>>>> case V4L2_MEMORY_MMAP: >>>>>> ret = __qbuf_mmap(vb, b); > Regards, -- Sylwester Nawrocki Samsung R&D Institute Poland ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-26 13:55 ` Sylwester Nawrocki @ 2013-08-26 14:32 ` Mauro Carvalho Chehab 2013-08-26 14:41 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Mauro Carvalho Chehab @ 2013-08-26 14:32 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Laurent Pinchart, Marek Szyprowski, Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil Em Mon, 26 Aug 2013 15:55:32 +0200 Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu: > Hi All, > > On 08/08/2013 02:35 PM, Laurent Pinchart wrote: > > Hi Marek, > > > > On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: > >> On 8/7/2013 12:44 PM, Laurent Pinchart wrote: > >>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > >>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > >>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > >>>>>> For output buffers application provide to the kernel the number of > >>>>>> bytes they stored in each plane of the buffer. Verify that the value > >>>>>> is smaller than or equal to the plane length. > >>>>>> > >>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>>>> --- > >>>>> > >>>>> Acked-by: Pawel Osciak <pawel@osciak.com> > >>>> > >>>> You're listed, as well as Marek and Kyungmin, as videobuf2 maintainers. > >>>> When you ack a videobuf2 patch, should we assume that you will take it > >>>> in your git tree ? > >>> > >>> Ping ? I'd like to get this patch in v3.12, should I send a pull request ? > >> > >> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> > >> Feel free to include it in your pull-request. I'm sorry for so huge > >> delay in my response. > > > > No worries. I'll send a pull request to Mauro. > > > >>>>>> drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++ > >>>>>> 1 files changed, 39 insertions(+), 0 deletions(-) > >>>>>> > >>>>>> Changes compared to v1: > >>>>>> > >>>>>> - Sanity check the data_offset value for each plane. > >>>>>> > >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c > >>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d > >>>>>> 100644 > >>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c > >>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c > >>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct > >>>>>> vb2_buffer > >>>>>> *vb, const struct v4l2_buffer> > >>>>>> > >>>>>> } > >>>>>> > >>>>>> /** > >>>>>> > >>>>>> + * __verify_length() - Verify that the bytesused value for each > >>>>>> plane > >>>>>> fits in > >>>>>> + * the plane length and that the data offset doesn't exceed the > >>>>>> bytesused value. > >>>>>> + */ > >>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct > >>>>>> v4l2_buffer *b) > >>>>>> +{ > >>>>>> + unsigned int length; > >>>>>> + unsigned int plane; > >>>>>> + > >>>>>> + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > >>>>>> + return 0; > >>>>>> + > >>>>>> + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > >>>>>> + for (plane = 0; plane < vb->num_planes; ++plane) { > >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) > >>>>>> + ? b->m.planes[plane].length > >>>>>> + : vb->v4l2_planes[plane].length; > >>>>>> + > >>>>>> + if (b->m.planes[plane].bytesused > length) > >>>>>> + return -EINVAL; > >>>>>> + if (b->m.planes[plane].data_offset >= > >>>>>> + b->m.planes[plane].bytesused) > >>>>>> + return -EINVAL; > > > This patch causes regressions. After kernel upgrade applications that > zero the planes array and don't set bytesused will stop working. > We could say that these are buggy applications, but if it has been > allowed for several kernel releases failing VIDIOC_QBUF on this check > now is plainly a regression IMO. I guess Linus wouldn't be happy about > a change like this. > > With this patch it is no longer possible to queue a buffer with bytesused > set to 0. I think it shouldn't be disallowed to queue a buffer with no > data to be used. So the check should likely be instead: > > if (b->m.planes[plane].bytesused > 0 && > b->m.planes[plane].data_offset >= > b->m.planes[plane].bytesused) > return -EINVAL; > > Sorry for the late review of this. Makes sense. Could you please send such patch? Regards, Mauro > > >>>>>> + } > >>>>>> + } else { > >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) > >>>>>> + ? b->length : vb->v4l2_planes[0].length; > >>>>>> + > >>>>>> + if (b->bytesused > length) > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>>> +/** > >>>>>> > >>>>>> * __buffer_in_use() - return true if the buffer is in use and > >>>>>> * the queue cannot be freed (by the means of REQBUFS(0)) call > >>>>>> */ > >>>>>> > >>>>>> @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer > >>>>>> *vb, > >>>>>> const struct v4l2_buffer *b)> > >>>>>> > >>>>>> struct vb2_queue *q = vb->vb2_queue; > >>>>>> int ret; > >>>>>> > >>>>>> + ret = __verify_length(vb, b); > >>>>>> + if (ret < 0) > >>>>>> + return ret; > >>>>>> + > >>>>>> > >>>>>> switch (q->memory) { > >>>>>> > >>>>>> case V4L2_MEMORY_MMAP: > >>>>>> ret = __qbuf_mmap(vb, b); > > > > Regards, -- Cheers, Mauro ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-26 14:32 ` Mauro Carvalho Chehab @ 2013-08-26 14:41 ` Laurent Pinchart 2013-08-26 15:03 ` Sylwester Nawrocki 0 siblings, 1 reply; 11+ messages in thread From: Laurent Pinchart @ 2013-08-26 14:41 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: Sylwester Nawrocki, Marek Szyprowski, Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil Hi Sylwester, On Monday 26 August 2013 11:32:01 Mauro Carvalho Chehab wrote: > Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu: > > On 08/08/2013 02:35 PM, Laurent Pinchart wrote: > > > On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: > > >> On 8/7/2013 12:44 PM, Laurent Pinchart wrote: > > >>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > > >>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > > >>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > > >>>>>> For output buffers application provide to the kernel the number of > > >>>>>> bytes they stored in each plane of the buffer. Verify that the > > >>>>>> value is smaller than or equal to the plane length. > > >>>>>> > > >>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > >>>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > > >>>>>> --- > > >>>>> > > >>>>> Acked-by: Pawel Osciak <pawel@osciak.com> > > >>>> > > >>>> You're listed, as well as Marek and Kyungmin, as videobuf2 > > >>>> maintainers. When you ack a videobuf2 patch, should we assume that > > >>>> you will take it in your git tree ? > > >>> > > >>> Ping ? I'd like to get this patch in v3.12, should I send a pull > > >>> request ? > > >> > > >> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > > >> > > >> Feel free to include it in your pull-request. I'm sorry for so huge > > >> delay in my response. > > > > > > No worries. I'll send a pull request to Mauro. > > > > > >>>>>> drivers/media/v4l2-core/videobuf2-core.c | 39 > > >>>>>> +++++++++++++++++++ > > >>>>>> 1 files changed, 39 insertions(+), 0 deletions(-) > > >>>>>> > > >>>>>> Changes compared to v1: > > >>>>>> > > >>>>>> - Sanity check the data_offset value for each plane. > > >>>>>> > > >>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > >>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d > > >>>>>> 100644 > > >>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c > > >>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c > > >>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct > > >>>>>> vb2_buffer > > >>>>>> *vb, const struct v4l2_buffer> > > >>>>>> } > > >>>>>> > > >>>>>> /** > > >>>>>> + * __verify_length() - Verify that the bytesused value for each > > >>>>>> plane > > >>>>>> fits in > > >>>>>> + * the plane length and that the data offset doesn't exceed the > > >>>>>> bytesused value. > > >>>>>> + */ > > >>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct > > >>>>>> v4l2_buffer *b) > > >>>>>> +{ > > >>>>>> + unsigned int length; > > >>>>>> + unsigned int plane; > > >>>>>> + > > >>>>>> + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > > >>>>>> + return 0; > > >>>>>> + > > >>>>>> + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > >>>>>> + for (plane = 0; plane < vb->num_planes; ++plane) { > > >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) > > >>>>>> + ? b->m.planes[plane].length > > >>>>>> + : vb->v4l2_planes[plane].length; > > >>>>>> + > > >>>>>> + if (b->m.planes[plane].bytesused > length) > > >>>>>> + return -EINVAL; > > >>>>>> + if (b->m.planes[plane].data_offset >= > > >>>>>> + b->m.planes[plane].bytesused) > > >>>>>> + return -EINVAL; > > > > This patch causes regressions. After kernel upgrade applications that > > zero the planes array and don't set bytesused will stop working. > > We could say that these are buggy applications, but if it has been > > allowed for several kernel releases failing VIDIOC_QBUF on this check > > now is plainly a regression IMO. I guess Linus wouldn't be happy about > > a change like this. > > > > With this patch it is no longer possible to queue a buffer with bytesused > > set to 0. I think it shouldn't be disallowed to queue a buffer with no > > data to be used. So the check should likely be instead: > > > > if (b->m.planes[plane].bytesused > 0 && > > b->m.planes[plane].data_offset >= > > b->m.planes[plane].bytesused) > > return -EINVAL; What about if (b->m.planes[plane].data_offset > 0 && b->m.planes[plane].data_offset >= b->m.planes[plane].bytesused) return -EINVAL; If data_offset is non-zero we don't want to accept a zero bytesused value. We could also catch data_offset == 0 && bytesused == 0 with a WARN_ONCE to try and get userspace applications fixed (this should definitely have been caught from the very start). > > Sorry for the late review of this. > > Makes sense. Could you please send such patch? > > > >>>>>> + } > > >>>>>> + } else { > > >>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) > > >>>>>> + ? b->length : vb->v4l2_planes[0].length; > > >>>>>> + > > >>>>>> + if (b->bytesused > length) > > >>>>>> + return -EINVAL; > > >>>>>> + } > > >>>>>> + > > >>>>>> + return 0; > > >>>>>> +} > > >>>>>> + > > >>>>>> +/** > > >>>>>> * __buffer_in_use() - return true if the buffer is in use and > > >>>>>> * the queue cannot be freed (by the means of REQBUFS(0)) call > > >>>>>> */ > > >>>>>> @@ -975,6 +1010,10 @@ static int __buf_prepare(struct vb2_buffer > > >>>>>> *vb, > > >>>>>> const struct v4l2_buffer *b)> > > >>>>>> > > >>>>>> struct vb2_queue *q = vb->vb2_queue; > > >>>>>> int ret; > > >>>>>> > > >>>>>> + ret = __verify_length(vb, b); > > >>>>>> + if (ret < 0) > > >>>>>> + return ret; > > >>>>>> + > > >>>>>> switch (q->memory) { > > >>>>>> case V4L2_MEMORY_MMAP: > > >>>>>> ret = __qbuf_mmap(vb, b); -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-26 14:41 ` Laurent Pinchart @ 2013-08-26 15:03 ` Sylwester Nawrocki 2013-08-27 8:50 ` Laurent Pinchart 0 siblings, 1 reply; 11+ messages in thread From: Sylwester Nawrocki @ 2013-08-26 15:03 UTC (permalink / raw) To: Laurent Pinchart Cc: Mauro Carvalho Chehab, Marek Szyprowski, Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil On 08/26/2013 04:41 PM, Laurent Pinchart wrote: > Hi Sylwester, > > On Monday 26 August 2013 11:32:01 Mauro Carvalho Chehab wrote: >> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu: >>> On 08/08/2013 02:35 PM, Laurent Pinchart wrote: >>>> On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: >>>>> On 8/7/2013 12:44 PM, Laurent Pinchart wrote: >>>>>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: >>>>>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: >>>>>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: >>>>>>>>> For output buffers application provide to the kernel the number of >>>>>>>>> bytes they stored in each plane of the buffer. Verify that the >>>>>>>>> value is smaller than or equal to the plane length. >>>>>>>>> >>>>>>>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >>>>>>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> >>>>>>>>> --- >>>>>>>> >>>>>>>> Acked-by: Pawel Osciak <pawel@osciak.com> >>>>>>> >>>>>>> You're listed, as well as Marek and Kyungmin, as videobuf2 >>>>>>> maintainers. When you ack a videobuf2 patch, should we assume that >>>>>>> you will take it in your git tree ? >>>>>> >>>>>> Ping ? I'd like to get this patch in v3.12, should I send a pull >>>>>> request ? >>>>> >>>>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>>> >>>>> Feel free to include it in your pull-request. I'm sorry for so huge >>>>> delay in my response. >>>> >>>> No worries. I'll send a pull request to Mauro. >>>> >>>>>>>>> drivers/media/v4l2-core/videobuf2-core.c | 39 >>>>>>>>> +++++++++++++++++++ >>>>>>>>> 1 files changed, 39 insertions(+), 0 deletions(-) >>>>>>>>> >>>>>>>>> Changes compared to v1: >>>>>>>>> >>>>>>>>> - Sanity check the data_offset value for each plane. >>>>>>>>> >>>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>>>>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d >>>>>>>>> 100644 >>>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>>>>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct >>>>>>>>> vb2_buffer >>>>>>>>> *vb, const struct v4l2_buffer> >>>>>>>>> } >>>>>>>>> >>>>>>>>> /** >>>>>>>>> + * __verify_length() - Verify that the bytesused value for each >>>>>>>>> plane >>>>>>>>> fits in >>>>>>>>> + * the plane length and that the data offset doesn't exceed the >>>>>>>>> bytesused value. >>>>>>>>> + */ >>>>>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct >>>>>>>>> v4l2_buffer *b) >>>>>>>>> +{ >>>>>>>>> + unsigned int length; >>>>>>>>> + unsigned int plane; >>>>>>>>> + >>>>>>>>> + if (!V4L2_TYPE_IS_OUTPUT(b->type)) >>>>>>>>> + return 0; >>>>>>>>> + >>>>>>>>> + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { >>>>>>>>> + for (plane = 0; plane < vb->num_planes; ++plane) { >>>>>>>>> + length = (b->memory == V4L2_MEMORY_USERPTR) >>>>>>>>> + ? b->m.planes[plane].length >>>>>>>>> + : vb->v4l2_planes[plane].length; >>>>>>>>> + >>>>>>>>> + if (b->m.planes[plane].bytesused > length) >>>>>>>>> + return -EINVAL; >>>>>>>>> + if (b->m.planes[plane].data_offset >= >>>>>>>>> + b->m.planes[plane].bytesused) >>>>>>>>> + return -EINVAL; >>> >>> This patch causes regressions. After kernel upgrade applications that >>> zero the planes array and don't set bytesused will stop working. >>> We could say that these are buggy applications, but if it has been >>> allowed for several kernel releases failing VIDIOC_QBUF on this check >>> now is plainly a regression IMO. I guess Linus wouldn't be happy about >>> a change like this. >>> >>> With this patch it is no longer possible to queue a buffer with bytesused >>> set to 0. I think it shouldn't be disallowed to queue a buffer with no >>> data to be used. So the check should likely be instead: >>> >>> if (b->m.planes[plane].bytesused > 0 && >>> b->m.planes[plane].data_offset >= >>> b->m.planes[plane].bytesused) >>> return -EINVAL; > > What about > > if (b->m.planes[plane].data_offset > 0 && > b->m.planes[plane].data_offset >= > b->m.planes[plane].bytesused) > return -EINVAL; > > If data_offset is non-zero we don't want to accept a zero bytesused value. You're right, that looks better. This will at least prevent failure of user space code like this one [1]. > We could also catch data_offset == 0 && bytesused == 0 with a WARN_ONCE to try > and get userspace applications fixed (this should definitely have been caught > from the very start). But is it really a critical error condition ? IIRC s5p-mfc driver uses buffers with bytesused = 0 to signal end of stream (I'm not judging whether it is right or not at the moment). I'm no sure if such a configuration should be disallowed right at the v4l2-core. >>> Sorry for the late review of this. >> >> Makes sense. Could you please send such patch? Sure, I'm preparing a patch. And... [...] >>>>>>>>> + ret = __verify_length(vb, b); >>>>>>>>> + if (ret < 0) additionally adding a debug print here, so it is easier to find out why QBUF fails. >>>>>>>>> + return ret; >>>>>>>>> + >>>>>>>>> switch (q->memory) { >>>>>>>>> case V4L2_MEMORY_MMAP: >>>>>>>>> ret = __qbuf_mmap(vb, b); [1] http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/mfc/fimc/fimc.c?id=0489f5277649826d1b38213c234fb0fe27206c2c#n543 -- Regards, Sylwester ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2] videobuf2-core: Verify planes lengths for output buffers 2013-08-26 15:03 ` Sylwester Nawrocki @ 2013-08-27 8:50 ` Laurent Pinchart 0 siblings, 0 replies; 11+ messages in thread From: Laurent Pinchart @ 2013-08-27 8:50 UTC (permalink / raw) To: Sylwester Nawrocki Cc: Mauro Carvalho Chehab, Marek Szyprowski, Pawel Osciak, linux-media, Kyungmin Park, Hans Verkuil Hi Sylwester, On Monday 26 August 2013 17:03:11 Sylwester Nawrocki wrote: > On 08/26/2013 04:41 PM, Laurent Pinchart wrote: > > On Monday 26 August 2013 11:32:01 Mauro Carvalho Chehab wrote: > >> Sylwester Nawrocki <s.nawrocki@samsung.com> escreveu: > >>> On 08/08/2013 02:35 PM, Laurent Pinchart wrote: > >>>> On Thursday 08 August 2013 14:14:30 Marek Szyprowski wrote: > >>>>> On 8/7/2013 12:44 PM, Laurent Pinchart wrote: > >>>>>> On Monday 12 November 2012 12:35:35 Laurent Pinchart wrote: > >>>>>>> On Friday 09 November 2012 15:33:22 Pawel Osciak wrote: > >>>>>>>> On Tue, Oct 16, 2012 at 8:37 AM, Laurent Pinchart wrote: > >>>>>>>>> For output buffers application provide to the kernel the number of > >>>>>>>>> bytes they stored in each plane of the buffer. Verify that the > >>>>>>>>> value is smaller than or equal to the plane length. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Laurent Pinchart > >>>>>>>>> <laurent.pinchart@ideasonboard.com> > >>>>>>>>> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > >>>>>>>>> --- > >>>>>>>> > >>>>>>>> Acked-by: Pawel Osciak <pawel@osciak.com> > >>>>>>> > >>>>>>> You're listed, as well as Marek and Kyungmin, as videobuf2 > >>>>>>> maintainers. When you ack a videobuf2 patch, should we assume that > >>>>>>> you will take it in your git tree ? > >>>>>> > >>>>>> Ping ? I'd like to get this patch in v3.12, should I send a pull > >>>>>> request ? > >>>>> > >>>>> Acked-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>>> > >>>>> Feel free to include it in your pull-request. I'm sorry for so huge > >>>>> delay in my response. > >>>> > >>>> No worries. I'll send a pull request to Mauro. > >>>> > >>>>>>>>> drivers/media/v4l2-core/videobuf2-core.c | 39 > >>>>>>>>> +++++++++++++++++++ > >>>>>>>>> 1 files changed, 39 insertions(+), 0 deletions(-) > >>>>>>>>> > >>>>>>>>> Changes compared to v1: > >>>>>>>>> > >>>>>>>>> - Sanity check the data_offset value for each plane. > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c > >>>>>>>>> b/drivers/media/v4l2-core/videobuf2-core.c index 432df11..479337d > >>>>>>>>> 100644 > >>>>>>>>> --- a/drivers/media/v4l2-core/videobuf2-core.c > >>>>>>>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c > >>>>>>>>> @@ -296,6 +296,41 @@ static int __verify_planes_array(struct > >>>>>>>>> vb2_buffer > >>>>>>>>> *vb, const struct v4l2_buffer> > >>>>>>>>> > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> /** > >>>>>>>>> > >>>>>>>>> + * __verify_length() - Verify that the bytesused value for each > >>>>>>>>> plane > >>>>>>>>> fits in > >>>>>>>>> + * the plane length and that the data offset doesn't exceed the > >>>>>>>>> bytesused value. > >>>>>>>>> + */ > >>>>>>>>> +static int __verify_length(struct vb2_buffer *vb, const struct > >>>>>>>>> v4l2_buffer *b) > >>>>>>>>> +{ > >>>>>>>>> + unsigned int length; > >>>>>>>>> + unsigned int plane; > >>>>>>>>> + > >>>>>>>>> + if (!V4L2_TYPE_IS_OUTPUT(b->type)) > >>>>>>>>> + return 0; > >>>>>>>>> + > >>>>>>>>> + if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > >>>>>>>>> + for (plane = 0; plane < vb->num_planes; ++plane) { > >>>>>>>>> + length = (b->memory == > >>>>>>>>> V4L2_MEMORY_USERPTR) > >>>>>>>>> + ? b->m.planes[plane].length > >>>>>>>>> + : vb->v4l2_planes[plane].length; > >>>>>>>>> + > >>>>>>>>> + if (b->m.planes[plane].bytesused > length) > >>>>>>>>> + return -EINVAL; > >>>>>>>>> + if (b->m.planes[plane].data_offset >= > >>>>>>>>> + b->m.planes[plane].bytesused) > >>>>>>>>> + return -EINVAL; > >>> > >>> This patch causes regressions. After kernel upgrade applications that > >>> zero the planes array and don't set bytesused will stop working. > >>> We could say that these are buggy applications, but if it has been > >>> allowed for several kernel releases failing VIDIOC_QBUF on this check > >>> now is plainly a regression IMO. I guess Linus wouldn't be happy about > >>> a change like this. > >>> > >>> With this patch it is no longer possible to queue a buffer with > >>> bytesused set to 0. I think it shouldn't be disallowed to queue a buffer > >>> with no data to be used. So the check should likely be instead: > >>> > >>> if (b->m.planes[plane].bytesused > 0 && > >>> b->m.planes[plane].data_offset >= > >>> b->m.planes[plane].bytesused) > >>> return -EINVAL; > > > > What about > > > > if (b->m.planes[plane].data_offset > 0 && > > b->m.planes[plane].data_offset >= > > b->m.planes[plane].bytesused) > > return -EINVAL; > > > > If data_offset is non-zero we don't want to accept a zero bytesused value. > > You're right, that looks better. > > This will at least prevent failure of user space code like this one [1]. > > > We could also catch data_offset == 0 && bytesused == 0 with a WARN_ONCE to > > try and get userspace applications fixed (this should definitely have > > been caught from the very start). > > But is it really a critical error condition ? IIRC s5p-mfc driver uses > buffers with bytesused = 0 to signal end of stream (I'm not judging whether > it is right or not at the moment). I'm no sure if such a configuration > should be disallowed right at the v4l2-core. You're absolutely right, bytesused == 0 is indeed valid. > >>> Sorry for the late review of this. > >> > >> Makes sense. Could you please send such patch? > > Sure, I'm preparing a patch. And... > > [...] > > >>>>>>>>> + ret = __verify_length(vb, b); > >>>>>>>>> + if (ret < 0) > > additionally adding a debug print here, so it is easier to find out > why QBUF fails. > > >>>>>>>>> + return ret; > >>>>>>>>> + > >>>>>>>>> > >>>>>>>>> switch (q->memory) { > >>>>>>>>> case V4L2_MEMORY_MMAP: > >>>>>>>>> ret = __qbuf_mmap(vb, b); > > [1] > http://cgit.freedesktop.org/gstreamer/gst-plugins-bad/tree/sys/mfc/fimc/fim > c.c?id=0489f5277649826d1b38213c234fb0fe27206c2c#n543 -- Regards, Laurent Pinchart ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-08-27 8:49 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-10-16 15:37 [PATCH v2] videobuf2-core: Verify planes lengths for output buffers Laurent Pinchart 2012-11-09 23:33 ` Pawel Osciak 2012-11-12 11:35 ` Laurent Pinchart 2013-08-07 10:44 ` Laurent Pinchart 2013-08-08 12:14 ` Marek Szyprowski 2013-08-08 12:35 ` Laurent Pinchart 2013-08-26 13:55 ` Sylwester Nawrocki 2013-08-26 14:32 ` Mauro Carvalho Chehab 2013-08-26 14:41 ` Laurent Pinchart 2013-08-26 15:03 ` Sylwester Nawrocki 2013-08-27 8:50 ` Laurent Pinchart
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox