LinuxTV Patchwork [v2,2/3] videobuf2-dma-sg: Prevent size from overflowing

login
register
mail settings
Submitter Sakari Ailus
Date Jan. 8, 2019, 8:58 a.m.
Message ID <20190108085836.9376-3-sakari.ailus@linux.intel.com>
Download mbox | patch
Permalink /patch/53740/
State New
Headers show

Comments

Sakari Ailus - Jan. 8, 2019, 8:58 a.m.
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 <sakari.ailus@linux.intel.com>
Cc: stable@vger.kernel.org
Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
 drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Mauro Carvalho Chehab - Jan. 8, 2019, 1:09 p.m.
Em Tue,  8 Jan 2019 10:58:35 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> 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 <sakari.ailus@linux.intel.com>
> Cc: stable@vger.kernel.org
> Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
>  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> index 015e737095cd..5fdb8d7051f6 100644
> --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> @@ -59,7 +59,10 @@ 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;

OK.

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

Hmm... why do we need a warn on here? This is called by this code:

static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
{
	struct vb2_queue *q = vb->vb2_queue;
	void *mem_priv;
	int plane;
	int ret = -ENOMEM;

	/*
	 * Allocate memory for all planes in this buffer
	 * NOTE: mmapped areas should be page aligned
	 */
	for (plane = 0; plane < vb->num_planes; ++plane) {
		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);

		mem_priv = call_ptr_memop(vb, alloc,
				q->alloc_devs[plane] ? : q->dev,
				q->dma_attrs, size, q->dma_dir, q->gfp_flags);

With already insures that size is page aligned.

Thanks,
Mauro
Sakari Ailus - Jan. 8, 2019, 1:29 p.m.
On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue,  8 Jan 2019 10:58:35 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > 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 <sakari.ailus@linux.intel.com>
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > index 015e737095cd..5fdb8d7051f6 100644
> > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > @@ -59,7 +59,10 @@ 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;
> 
> OK.
> 
> > +
> > +	if (WARN_ON(size & ~PAGE_MASK))
> > +		return -ENOMEM;
> 
> Hmm... why do we need a warn on here? This is called by this code:

This was suggested as a sanity check in review of v1 of the set.

Supposing that someone once removed that alignment, things would go rather
completely haywire. There would probably be lots of other troubles as well
but this one would probably corrupt system memory (at least).

> 
> static int __vb2_buf_mem_alloc(struct vb2_buffer *vb)
> {
> 	struct vb2_queue *q = vb->vb2_queue;
> 	void *mem_priv;
> 	int plane;
> 	int ret = -ENOMEM;
> 
> 	/*
> 	 * Allocate memory for all planes in this buffer
> 	 * NOTE: mmapped areas should be page aligned
> 	 */
> 	for (plane = 0; plane < vb->num_planes; ++plane) {
> 		unsigned long size = PAGE_ALIGN(vb->planes[plane].length);
> 
> 		mem_priv = call_ptr_memop(vb, alloc,
> 				q->alloc_devs[plane] ? : q->dev,
> 				q->dma_attrs, size, q->dma_dir, q->gfp_flags);
> 
> With already insures that size is page aligned.
> 
> Thanks,
> Mauro
Mauro Carvalho Chehab - Jan. 8, 2019, 1:44 p.m.
Em Tue, 8 Jan 2019 15:29:26 +0200
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> > Em Tue,  8 Jan 2019 10:58:35 +0200
> > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> >   
> > > 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 <sakari.ailus@linux.intel.com>
> > > Cc: stable@vger.kernel.org
> > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > ---
> > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > index 015e737095cd..5fdb8d7051f6 100644
> > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > @@ -59,7 +59,10 @@ 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;  
> > 
> > OK.
> >   
> > > +
> > > +	if (WARN_ON(size & ~PAGE_MASK))
> > > +		return -ENOMEM;  
> > 
> > Hmm... why do we need a warn on here? This is called by this code:  
> 
> This was suggested as a sanity check in review of v1 of the set.
> 
> Supposing that someone once removed that alignment, things would go rather
> completely haywire. There would probably be lots of other troubles as well
> but this one would probably corrupt system memory (at least).

Well, patch 3 prevents that. See: this is not like something that driver
developers can mess with that, as the only place where the .alloc() ops
is called is by the VB2 core, and it already ensures page alignment.

If one would ever try to remove PAGE_ALIGN() from vb2 core, we'll nack it,
as we know that such change will break things.

Thanks,
Mauro
Sakari Ailus - Jan. 8, 2019, 1:57 p.m.
On Tue, Jan 08, 2019 at 11:44:01AM -0200, Mauro Carvalho Chehab wrote:
> Em Tue, 8 Jan 2019 15:29:26 +0200
> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> 
> > On Tue, Jan 08, 2019 at 11:09:42AM -0200, Mauro Carvalho Chehab wrote:
> > > Em Tue,  8 Jan 2019 10:58:35 +0200
> > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:
> > >   
> > > > 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 <sakari.ailus@linux.intel.com>
> > > > Cc: stable@vger.kernel.org
> > > > Reviewed-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > > > ---
> > > >  drivers/media/common/videobuf2/videobuf2-dma-sg.c | 5 ++++-
> > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > index 015e737095cd..5fdb8d7051f6 100644
> > > > --- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > +++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
> > > > @@ -59,7 +59,10 @@ 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;  
> > > 
> > > OK.
> > >   
> > > > +
> > > > +	if (WARN_ON(size & ~PAGE_MASK))
> > > > +		return -ENOMEM;  
> > > 
> > > Hmm... why do we need a warn on here? This is called by this code:  
> > 
> > This was suggested as a sanity check in review of v1 of the set.
> > 
> > Supposing that someone once removed that alignment, things would go rather
> > completely haywire. There would probably be lots of other troubles as well
> > but this one would probably corrupt system memory (at least).
> 
> Well, patch 3 prevents that. See: this is not like something that driver
> developers can mess with that, as the only place where the .alloc() ops
> is called is by the VB2 core, and it already ensures page alignment.
> 
> If one would ever try to remove PAGE_ALIGN() from vb2 core, we'll nack it,
> as we know that such change will break things.

Indeed. I'm certainly fine with dropping the sanity check; I think there
are enough warnings elsewhere plus common sense to avoid making such a
change.

Patch

diff --git a/drivers/media/common/videobuf2/videobuf2-dma-sg.c b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
index 015e737095cd..5fdb8d7051f6 100644
--- a/drivers/media/common/videobuf2/videobuf2-dma-sg.c
+++ b/drivers/media/common/videobuf2/videobuf2-dma-sg.c
@@ -59,7 +59,10 @@  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;
+
+	if (WARN_ON(size & ~PAGE_MASK))
+		return -ENOMEM;
 
 	while (size > 0) {
 		struct page *pages;

Privacy Policy