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@xxxxxxxxxxxx] > Sent: Tuesday, January 16, 2018 10:40 AM > To: Zhi, Yong <yong.zhi@xxxxxxxxx> > Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan <rajmohan.mani@xxxxxxxxx>; > Cao, Bingbu <bingbu.cao@xxxxxxxxx> > 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@xxxxxxxxx> wrote: > > Hi, Tomasz, > > > > Thanks for the patch review. > > > >> -----Original Message----- > >> From: Tomasz Figa [mailto:tfiga@xxxxxxxxxxxx] > >> Sent: Friday, January 12, 2018 12:17 AM > >> To: Zhi, Yong <yong.zhi@xxxxxxxxx> > >> Cc: Linux Media Mailing List <linux-media@xxxxxxxxxxxxxxx>; Sakari > >> Ailus <sakari.ailus@xxxxxxxxxxxxxxx>; Mani, Rajmohan > >> <rajmohan.mani@xxxxxxxxx>; Cao, Bingbu <bingbu.cao@xxxxxxxxx> > >> 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@xxxxxxxxx> 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@xxxxxxxxx> > >> > Signed-off-by: Cao Bing Bu <bingbu.cao@xxxxxxxxx> > >> > --- > >> > 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
