[PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Yong Zhi]

When dmabuf is used for BLOB type frame, the frame
buffers allocated by gralloc will hold more pages
than the valid frame data due to height alignment.

In this case, the page numbers in sg list could exceed the
FBPT upper limit value - max_lops(8)*1024 to cause crash.

Limit the LOP access to the valid data length
to avoid FBPT sub-entries overflow.

Signed-off-by: Yong Zhi <yong.zhi@intel.com>
Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 941caa987dab..949f43d206ad 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
 	static const unsigned int entries_per_page =
 		CIO2_PAGE_SIZE / sizeof(u32);
-	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
-	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
+	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
+					  CIO2_PAGE_SIZE) + 1;
+	unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
 	struct sg_table *sg;
 	struct sg_page_iter sg_iter;
 	int i, j;
@@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
 
 	i = j = 0;
 	for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
+		if (!pages--)
+			break;
 		b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >> PAGE_SHIFT;
 		j++;
 		if (j == entries_per_page) {
-- 
2.7.4

[PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Yong Zhi]

cio2 driver should release buffer with QUEUED state
when start_stream op failed, wrong buffer state will
cause vb2 core throw a warning.

Signed-off-by: Yong Zhi <yong.zhi@intel.com>
Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
---
 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
index 949f43d206ad..106d04306372 100644
--- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
+++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
@@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void *cio2_ptr)
 
 /**************** Videobuf2 interface ****************/
 
-static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
+static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
+					enum vb2_buffer_state state)
 {
 	unsigned int i;
 
@@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
 		if (q->bufs[i]) {
 			atomic_dec(&q->bufs_queued);
 			vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf,
-					VB2_BUF_STATE_ERROR);
+					state);
 		}
 	}
 }
@@ -1019,7 +1020,7 @@ static int cio2_vb2_start_streaming(struct vb2_queue *vq, unsigned int count)
 	media_pipeline_stop(&q->vdev.entity);
 fail_pipeline:
 	dev_dbg(&cio2->pci_dev->dev, "failed to start streaming (%d)\n", r);
-	cio2_vb2_return_all_buffers(q);
+	cio2_vb2_return_all_buffers(q, VB2_BUF_STATE_QUEUED);
 	pm_runtime_put(&cio2->pci_dev->dev);
 
 	return r;
@@ -1035,7 +1036,7 @@ static void cio2_vb2_stop_streaming(struct vb2_queue *vq)
 			"failed to stop sensor streaming\n");
 
 	cio2_hw_exit(cio2, q);
-	cio2_vb2_return_all_buffers(q);
+	cio2_vb2_return_all_buffers(q, VB2_BUF_STATE_ERROR);
 	media_pipeline_stop(&q->vdev.entity);
 	pm_runtime_put(&cio2->pci_dev->dev);
 	cio2->streaming = false;
-- 
2.7.4

Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Tomasz Figa]

On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> When dmabuf is used for BLOB type frame, the frame
> buffers allocated by gralloc will hold more pages
> than the valid frame data due to height alignment.
>
> In this case, the page numbers in sg list could exceed the
> FBPT upper limit value - max_lops(8)*1024 to cause crash.
>
> Limit the LOP access to the valid data length
> to avoid FBPT sub-entries overflow.
>
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 941caa987dab..949f43d206ad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>                 container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>         static const unsigned int entries_per_page =
>                 CIO2_PAGE_SIZE / sizeof(u32);
> -       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE);
> -       unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> +       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> +                                         CIO2_PAGE_SIZE) + 1;

Why + 1? This would still overflow the buffer, wouldn't it?

> +       unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>         struct sg_table *sg;
>         struct sg_page_iter sg_iter;
>         int i, j;
> @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>
>         i = j = 0;
>         for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
> +               if (!pages--)
> +                       break;

Or perhaps we should check here for (pages > 1)?

Best regards,
Tomasz

Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Tomasz Figa]

On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> cio2 driver should release buffer with QUEUED state
> when start_stream op failed, wrong buffer state will
> cause vb2 core throw a warning.
>
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 949f43d206ad..106d04306372 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void *cio2_ptr)
>
>  /**************** Videobuf2 interface ****************/
>
> -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
> +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
> +                                       enum vb2_buffer_state state)
>  {
>         unsigned int i;
>
> @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
>                 if (q->bufs[i]) {
>                         atomic_dec(&q->bufs_queued);
>                         vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf,
> -                                       VB2_BUF_STATE_ERROR);
> +                                       state);

nit: Does it really exceed 80 characters after folding into previous line?

With the nit fixed:
Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Mani, Rajmohan]

Hi Yong,

> -----Original Message-----
> From: Zhi, Yong
> Sent: Wednesday, January 03, 2018 6:57 PM
> To: linux-media@vger.kernel.org; sakari.ailus@linux.intel.com
> Cc: tfiga@chromium.org; Mani, Rajmohan <rajmohan.mani@intel.com>; Zhi,
> Yong <yong.zhi@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
> Subject: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds
> access
> 
> When dmabuf is used for BLOB type frame, the frame buffers allocated by
> gralloc will hold more pages than the valid frame data due to height alignment.
> 
> In this case, the page numbers in sg list could exceed the FBPT upper limit
> value - max_lops(8)*1024 to cause crash.
> 
> Limit the LOP access to the valid data length to avoid FBPT sub-entries
> overflow.
> 
> Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> ---
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 941caa987dab..949f43d206ad 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>  		container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>  	static const unsigned int entries_per_page =
>  		CIO2_PAGE_SIZE / sizeof(u32);
> -	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> CIO2_PAGE_SIZE);
> -	unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> +	unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> +					  CIO2_PAGE_SIZE) + 1;
> +	unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>  	struct sg_table *sg;
>  	struct sg_page_iter sg_iter;
>  	int i, j;
> @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> 
>  	i = j = 0;

Nit: separate assignments are preferred over multiple assignments

>  	for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
> +		if (!pages--)
> +			break;
>  		b->lop[i][j] = sg_page_iter_dma_address(&sg_iter) >>
> PAGE_SHIFT;
>  		j++;
>  		if (j == entries_per_page) {
> --
> 2.7.4

Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Sakari Ailus]

Hi Tomasz and others,

On Fri, Jan 12, 2018 at 05:19:04PM +0900, Tomasz Figa wrote:
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
...
> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
> >                 if (q->bufs[i]) {
> >                         atomic_dec(&q->bufs_queued);
> >                         vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf,
> > -                                       VB2_BUF_STATE_ERROR);
> > +                                       state);
> 
> nit: Does it really exceed 80 characters after folding into previous line?
> 
> With the nit fixed:
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>

The patches have been merged to media tree master; if there are matters to
address, then please send more patches on top of the master branch. :-)

Thanks.

-- 
Cheers,

Sakari Ailus
sakari.ailus@linux.intel.com

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Zhi, Yong]

Hi, Tomasz,

Thanks for the patch review.

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga@chromium.org]
> Sent: Friday, January 12, 2018 12:17 AM
> To: Zhi, Yong <yong.zhi@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-
> bounds access
> 
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> > When dmabuf is used for BLOB type frame, the frame buffers allocated
> > by gralloc will hold more pages than the valid frame data due to
> > height alignment.
> >
> > In this case, the page numbers in sg list could exceed the FBPT upper
> > limit value - max_lops(8)*1024 to cause crash.
> >
> > Limit the LOP access to the valid data length to avoid FBPT
> > sub-entries overflow.
> >
> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 941caa987dab..949f43d206ad 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
> >                 container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> >         static const unsigned int entries_per_page =
> >                 CIO2_PAGE_SIZE / sizeof(u32);
> > -       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> CIO2_PAGE_SIZE);
> > -       unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
> > +       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> > +                                         CIO2_PAGE_SIZE) + 1;
> 
> Why + 1? This would still overflow the buffer, wouldn't it?

The "pages" variable is used to calculate lops which has one extra page at the end that points to dummy page.

> 
> > +       unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
> >         struct sg_table *sg;
> >         struct sg_page_iter sg_iter;
> >         int i, j;
> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> > *vb)
> >
> >         i = j = 0;
> >         for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
> > +               if (!pages--)
> > +                       break;
> 
> Or perhaps we should check here for (pages > 1)?

This is so that the end of lop is set to the dummy_page.

> 
> Best regards,
> Tomasz

RE: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Zhi, Yong]

Hi, Tomasz,

Thanks for reviewing the patch.

> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga@chromium.org]
> Sent: Friday, January 12, 2018 12:19 AM
> To: Zhi, Yong <yong.zhi@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Mani, Rajmohan
> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state
> warnings
> 
> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> > cio2 driver should release buffer with QUEUED state when start_stream
> > op failed, wrong buffer state will cause vb2 core throw a warning.
> >
> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> > ---
> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > index 949f43d206ad..106d04306372 100644
> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void
> > *cio2_ptr)
> >
> >  /**************** Videobuf2 interface ****************/
> >
> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
> > +                                       enum vb2_buffer_state state)
> >  {
> >         unsigned int i;
> >
> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct
> cio2_queue *q)
> >                 if (q->bufs[i]) {
> >                         atomic_dec(&q->bufs_queued);
> >                         vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf,
> > -                                       VB2_BUF_STATE_ERROR);
> > +                                       state);
> 
> nit: Does it really exceed 80 characters after folding into previous line?
> 

Thanks for catching this, seems this patch was merged, may I fix it in future patch?

> With the nit fixed:
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> 
> Best regards,
> Tomasz

Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Tomasz Figa]

Hi Yong,

On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong <yong.zhi@intel.com> wrote:
> Hi, Tomasz,
>
> Thanks for the patch review.
>
>> -----Original Message-----
>> From: Tomasz Figa [mailto:tfiga@chromium.org]
>> Sent: Friday, January 12, 2018 12:17 AM
>> To: Zhi, Yong <yong.zhi@intel.com>
>> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
>> <sakari.ailus@linux.intel.com>; Mani, Rajmohan
>> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
>> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-
>> bounds access
>>
>> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
>> > When dmabuf is used for BLOB type frame, the frame buffers allocated
>> > by gralloc will hold more pages than the valid frame data due to
>> > height alignment.
>> >
>> > In this case, the page numbers in sg list could exceed the FBPT upper
>> > limit value - max_lops(8)*1024 to cause crash.
>> >
>> > Limit the LOP access to the valid data length to avoid FBPT
>> > sub-entries overflow.
>> >
>> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
>> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
>> > ---
>> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
>> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > index 941caa987dab..949f43d206ad 100644
>> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer *vb)
>> >                 container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>> >         static const unsigned int entries_per_page =
>> >                 CIO2_PAGE_SIZE / sizeof(u32);
>> > -       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> CIO2_PAGE_SIZE);
>> > -       unsigned int lops = DIV_ROUND_UP(pages + 1, entries_per_page);
>> > +       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> > +                                         CIO2_PAGE_SIZE) + 1;
>>
>> Why + 1? This would still overflow the buffer, wouldn't it?
>
> The "pages" variable is used to calculate lops which has one extra page at the end that points to dummy page.
>
>>
>> > +       unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>> >         struct sg_table *sg;
>> >         struct sg_page_iter sg_iter;
>> >         int i, j;
>> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> > *vb)
>> >
>> >         i = j = 0;
>> >         for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
>> > +               if (!pages--)
>> > +                       break;
>>
>> Or perhaps we should check here for (pages > 1)?
>
> This is so that the end of lop is set to the dummy_page.

How about this simple example:

vb->planes[0].length = 1023 * 4096
pages = 1023 + 1 = 1024
lops  = 1

If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
will iterate for pages from 1024 to 1 inclusive and ends up
overflowing the dummy page to next lop (i == 1 and j == 0), but we
only allocated 1 lop.

Best regards,
Tomasz

Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings

[Tomasz Figa]

On Tue, Jan 16, 2018 at 2:07 AM, Zhi, Yong <yong.zhi@intel.com> wrote:
> Hi, Tomasz,
>
> Thanks for reviewing the patch.
>
>> -----Original Message-----
>> From: Tomasz Figa [mailto:tfiga@chromium.org]
>> Sent: Friday, January 12, 2018 12:19 AM
>> To: Zhi, Yong <yong.zhi@intel.com>
>> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
>> <sakari.ailus@linux.intel.com>; Mani, Rajmohan
>> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
>> Subject: Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state
>> warnings
>>
>> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
>> > cio2 driver should release buffer with QUEUED state when start_stream
>> > op failed, wrong buffer state will cause vb2 core throw a warning.
>> >
>> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
>> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
>> > ---
>> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 9 +++++----
>> >  1 file changed, 5 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > index 949f43d206ad..106d04306372 100644
>> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> > @@ -785,7 +785,8 @@ static irqreturn_t cio2_irq(int irq, void
>> > *cio2_ptr)
>> >
>> >  /**************** Videobuf2 interface ****************/
>> >
>> > -static void cio2_vb2_return_all_buffers(struct cio2_queue *q)
>> > +static void cio2_vb2_return_all_buffers(struct cio2_queue *q,
>> > +                                       enum vb2_buffer_state state)
>> >  {
>> >         unsigned int i;
>> >
>> > @@ -793,7 +794,7 @@ static void cio2_vb2_return_all_buffers(struct
>> cio2_queue *q)
>> >                 if (q->bufs[i]) {
>> >                         atomic_dec(&q->bufs_queued);
>> >                         vb2_buffer_done(&q->bufs[i]->vbb.vb2_buf,
>> > -                                       VB2_BUF_STATE_ERROR);
>> > +                                       state);
>>
>> nit: Does it really exceed 80 characters after folding into previous line?
>>
>
> Thanks for catching this, seems this patch was merged, may I fix it in future patch?

It's just a nit that I was hoping to be fixed at patch applying time.
Otherwise just never mind.

Best regards,
Tomasz

RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Cao, Bingbu]

I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct.
should be 'if (!--pages)'.
The last page from sg list is the last valid page.


__________________________
BRs,
Cao, Bingbu



> -----Original Message-----
> From: Tomasz Figa [mailto:tfiga@chromium.org]
> Sent: Tuesday, January 16, 2018 10:40 AM
> To: Zhi, Yong <yong.zhi@intel.com>
> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Mani, Rajmohan <rajmohan.mani@intel.com>;
> Cao, Bingbu <bingbu.cao@intel.com>
> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-
> of-bounds access
> 
> Hi Yong,
> 
> On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong <yong.zhi@intel.com> wrote:
> > Hi, Tomasz,
> >
> > Thanks for the patch review.
> >
> >> -----Original Message-----
> >> From: Tomasz Figa [mailto:tfiga@chromium.org]
> >> Sent: Friday, January 12, 2018 12:17 AM
> >> To: Zhi, Yong <yong.zhi@intel.com>
> >> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari
> >> Ailus <sakari.ailus@linux.intel.com>; Mani, Rajmohan
> >> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
> >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with
> >> out-of- bounds access
> >>
> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
> >> > When dmabuf is used for BLOB type frame, the frame buffers
> >> > allocated by gralloc will hold more pages than the valid frame data
> >> > due to height alignment.
> >> >
> >> > In this case, the page numbers in sg list could exceed the FBPT
> >> > upper limit value - max_lops(8)*1024 to cause crash.
> >> >
> >> > Limit the LOP access to the valid data length to avoid FBPT
> >> > sub-entries overflow.
> >> >
> >> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
> >> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
> >> > ---
> >> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > index 941caa987dab..949f43d206ad 100644
> >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> *vb)
> >> >                 container_of(vb, struct cio2_buffer, vbb.vb2_buf);
> >> >         static const unsigned int entries_per_page =
> >> >                 CIO2_PAGE_SIZE / sizeof(u32);
> >> > -       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> >> CIO2_PAGE_SIZE);
> >> > -       unsigned int lops = DIV_ROUND_UP(pages + 1,
> entries_per_page);
> >> > +       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
> >> > +                                         CIO2_PAGE_SIZE) + 1;
> >>
> >> Why + 1? This would still overflow the buffer, wouldn't it?
> >
> > The "pages" variable is used to calculate lops which has one extra
> page at the end that points to dummy page.
> >
> >>
> >> > +       unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
> >> >         struct sg_table *sg;
> >> >         struct sg_page_iter sg_iter;
> >> >         int i, j;
> >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
> >> > *vb)
> >> >
> >> >         i = j = 0;
> >> >         for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
> >> > +               if (!pages--)
> >> > +                       break;
> >>
> >> Or perhaps we should check here for (pages > 1)?
> >
> > This is so that the end of lop is set to the dummy_page.
> 
> How about this simple example:
> 
> vb->planes[0].length = 1023 * 4096
> pages = 1023 + 1 = 1024
> lops  = 1
> 
> If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
> will iterate for pages from 1024 to 1 inclusive and ends up overflowing
> the dummy page to next lop (i == 1 and j == 0), but we only allocated 1
> lop.
> 
> Best regards,
> Tomasz

Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access

[Tomasz Figa]

Hi Bingbu,

On Tue, Jan 16, 2018 at 1:05 PM, Cao, Bingbu <bingbu.cao@intel.com> wrote:
> I think if set the pages as the DIV_ROUND_UP(vb->planes[0].length, CIO2_PAGE_SIZE) + 1, the ' if (!pages--)' in loop is not correct.
> should be 'if (!--pages)'.
> The last page from sg list is the last valid page.
>

Yes, that's exactly what I meant.

By the way, please avoid top-posting to mailing lists, it isn't well
received by recipients.

Best regards,
Tomasz

>
> __________________________
> BRs,
> Cao, Bingbu
>
>
>
>> -----Original Message-----
>> From: Tomasz Figa [mailto:tfiga@chromium.org]
>> Sent: Tuesday, January 16, 2018 10:40 AM
>> To: Zhi, Yong <yong.zhi@intel.com>
>> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari Ailus
>> <sakari.ailus@linux.intel.com>; Mani, Rajmohan <rajmohan.mani@intel.com>;
>> Cao, Bingbu <bingbu.cao@intel.com>
>> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-
>> of-bounds access
>>
>> Hi Yong,
>>
>> On Tue, Jan 16, 2018 at 2:05 AM, Zhi, Yong <yong.zhi@intel.com> wrote:
>> > Hi, Tomasz,
>> >
>> > Thanks for the patch review.
>> >
>> >> -----Original Message-----
>> >> From: Tomasz Figa [mailto:tfiga@chromium.org]
>> >> Sent: Friday, January 12, 2018 12:17 AM
>> >> To: Zhi, Yong <yong.zhi@intel.com>
>> >> Cc: Linux Media Mailing List <linux-media@vger.kernel.org>; Sakari
>> >> Ailus <sakari.ailus@linux.intel.com>; Mani, Rajmohan
>> >> <rajmohan.mani@intel.com>; Cao, Bingbu <bingbu.cao@intel.com>
>> >> Subject: Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with
>> >> out-of- bounds access
>> >>
>> >> On Thu, Jan 4, 2018 at 11:57 AM, Yong Zhi <yong.zhi@intel.com> wrote:
>> >> > When dmabuf is used for BLOB type frame, the frame buffers
>> >> > allocated by gralloc will hold more pages than the valid frame data
>> >> > due to height alignment.
>> >> >
>> >> > In this case, the page numbers in sg list could exceed the FBPT
>> >> > upper limit value - max_lops(8)*1024 to cause crash.
>> >> >
>> >> > Limit the LOP access to the valid data length to avoid FBPT
>> >> > sub-entries overflow.
>> >> >
>> >> > Signed-off-by: Yong Zhi <yong.zhi@intel.com>
>> >> > Signed-off-by: Cao Bing Bu <bingbu.cao@intel.com>
>> >> > ---
>> >> >  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 7 +++++--
>> >> >  1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > index 941caa987dab..949f43d206ad 100644
>> >> > --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
>> >> > @@ -838,8 +838,9 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> *vb)
>> >> >                 container_of(vb, struct cio2_buffer, vbb.vb2_buf);
>> >> >         static const unsigned int entries_per_page =
>> >> >                 CIO2_PAGE_SIZE / sizeof(u32);
>> >> > -       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> >> CIO2_PAGE_SIZE);
>> >> > -       unsigned int lops = DIV_ROUND_UP(pages + 1,
>> entries_per_page);
>> >> > +       unsigned int pages = DIV_ROUND_UP(vb->planes[0].length,
>> >> > +                                         CIO2_PAGE_SIZE) + 1;
>> >>
>> >> Why + 1? This would still overflow the buffer, wouldn't it?
>> >
>> > The "pages" variable is used to calculate lops which has one extra
>> page at the end that points to dummy page.
>> >
>> >>
>> >> > +       unsigned int lops = DIV_ROUND_UP(pages, entries_per_page);
>> >> >         struct sg_table *sg;
>> >> >         struct sg_page_iter sg_iter;
>> >> >         int i, j;
>> >> > @@ -869,6 +870,8 @@ static int cio2_vb2_buf_init(struct vb2_buffer
>> >> > *vb)
>> >> >
>> >> >         i = j = 0;
>> >> >         for_each_sg_page(sg->sgl, &sg_iter, sg->nents, 0) {
>> >> > +               if (!pages--)
>> >> > +                       break;
>> >>
>> >> Or perhaps we should check here for (pages > 1)?
>> >
>> > This is so that the end of lop is set to the dummy_page.
>>
>> How about this simple example:
>>
>> vb->planes[0].length = 1023 * 4096
>> pages = 1023 + 1 = 1024
>> lops  = 1
>>
>> If sg->sgl includes more than 1023 pages, the for_each_sg_page() loop
>> will iterate for pages from 1024 to 1 inclusive and ends up overflowing
>> the dummy page to next lop (i == 1 and j == 0), but we only allocated 1
>> lop.
>>
>> Best regards,
>> Tomasz