Hi Bingbu, On Tue, Jan 16, 2018 at 1:05 PM, Cao, Bingbu <bingbu.cao@xxxxxxxxx> 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@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
