* [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access @ 2018-01-04 2:57 Yong Zhi 2018-01-04 2:57 ` [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings Yong Zhi ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Yong Zhi @ 2018-01-04 2:57 UTC (permalink / raw) To: linux-media, sakari.ailus; +Cc: tfiga, rajmohan.mani, Yong Zhi, Cao Bing Bu 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings 2018-01-04 2:57 [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Yong Zhi @ 2018-01-04 2:57 ` Yong Zhi 2018-01-12 8:19 ` Tomasz Figa 2018-01-12 8:16 ` [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Tomasz Figa 2018-01-12 17:59 ` Mani, Rajmohan 2 siblings, 1 reply; 12+ messages in thread From: Yong Zhi @ 2018-01-04 2:57 UTC (permalink / raw) To: linux-media, sakari.ailus; +Cc: tfiga, rajmohan.mani, Yong Zhi, Cao Bing Bu 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings 2018-01-04 2:57 ` [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings Yong Zhi @ 2018-01-12 8:19 ` Tomasz Figa 2018-01-14 22:55 ` Sakari Ailus 2018-01-15 17:07 ` Zhi, Yong 0 siblings, 2 replies; 12+ messages in thread From: Tomasz Figa @ 2018-01-12 8:19 UTC (permalink / raw) To: Yong Zhi Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao Bing Bu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings 2018-01-12 8:19 ` Tomasz Figa @ 2018-01-14 22:55 ` Sakari Ailus 2018-01-15 17:07 ` Zhi, Yong 1 sibling, 0 replies; 12+ messages in thread From: Sakari Ailus @ 2018-01-14 22:55 UTC (permalink / raw) To: Tomasz Figa Cc: Yong Zhi, Linux Media Mailing List, Mani, Rajmohan, Cao Bing Bu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings 2018-01-12 8:19 ` Tomasz Figa 2018-01-14 22:55 ` Sakari Ailus @ 2018-01-15 17:07 ` Zhi, Yong 2018-01-16 2:42 ` Tomasz Figa 1 sibling, 1 reply; 12+ messages in thread From: Zhi, Yong @ 2018-01-15 17:07 UTC (permalink / raw) To: Tomasz Figa Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao, Bingbu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings 2018-01-15 17:07 ` Zhi, Yong @ 2018-01-16 2:42 ` Tomasz Figa 0 siblings, 0 replies; 12+ messages in thread From: Tomasz Figa @ 2018-01-16 2:42 UTC (permalink / raw) To: Zhi, Yong Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao, Bingbu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-04 2:57 [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Yong Zhi 2018-01-04 2:57 ` [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings Yong Zhi @ 2018-01-12 8:16 ` Tomasz Figa 2018-01-15 17:05 ` Zhi, Yong 2018-01-12 17:59 ` Mani, Rajmohan 2 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2018-01-12 8:16 UTC (permalink / raw) To: Yong Zhi Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao Bing Bu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-12 8:16 ` [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Tomasz Figa @ 2018-01-15 17:05 ` Zhi, Yong 2018-01-16 2:40 ` Tomasz Figa 0 siblings, 1 reply; 12+ messages in thread From: Zhi, Yong @ 2018-01-15 17:05 UTC (permalink / raw) To: Tomasz Figa Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao, Bingbu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-15 17:05 ` Zhi, Yong @ 2018-01-16 2:40 ` Tomasz Figa 2018-01-16 4:05 ` Cao, Bingbu 0 siblings, 1 reply; 12+ messages in thread From: Tomasz Figa @ 2018-01-16 2:40 UTC (permalink / raw) To: Zhi, Yong Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan, Cao, Bingbu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-16 2:40 ` Tomasz Figa @ 2018-01-16 4:05 ` Cao, Bingbu 2018-01-16 4:07 ` Tomasz Figa 0 siblings, 1 reply; 12+ messages in thread From: Cao, Bingbu @ 2018-01-16 4:05 UTC (permalink / raw) To: Tomasz Figa, Zhi, Yong Cc: Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-16 4:05 ` Cao, Bingbu @ 2018-01-16 4:07 ` Tomasz Figa 0 siblings, 0 replies; 12+ messages in thread From: Tomasz Figa @ 2018-01-16 4:07 UTC (permalink / raw) To: Cao, Bingbu Cc: Zhi, Yong, Linux Media Mailing List, Sakari Ailus, Mani, Rajmohan 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access 2018-01-04 2:57 [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Yong Zhi 2018-01-04 2:57 ` [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings Yong Zhi 2018-01-12 8:16 ` [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Tomasz Figa @ 2018-01-12 17:59 ` Mani, Rajmohan 2 siblings, 0 replies; 12+ messages in thread From: Mani, Rajmohan @ 2018-01-12 17:59 UTC (permalink / raw) To: Zhi, Yong, linux-media, sakari.ailus; +Cc: tfiga, Cao, Bingbu 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-01-16 4:08 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-04 2:57 [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Yong Zhi 2018-01-04 2:57 ` [PATCH 2/2] media: intel-ipu3: cio2: fix for wrong vb2buf state warnings Yong Zhi 2018-01-12 8:19 ` Tomasz Figa 2018-01-14 22:55 ` Sakari Ailus 2018-01-15 17:07 ` Zhi, Yong 2018-01-16 2:42 ` Tomasz Figa 2018-01-12 8:16 ` [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access Tomasz Figa 2018-01-15 17:05 ` Zhi, Yong 2018-01-16 2:40 ` Tomasz Figa 2018-01-16 4:05 ` Cao, Bingbu 2018-01-16 4:07 ` Tomasz Figa 2018-01-12 17:59 ` Mani, Rajmohan
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.