Hi, Tomasz, Thanks for the patch review.
> -----Original Message----- > From: Tomasz Figa [mailto:[email protected]] > Sent: Friday, January 12, 2018 12:17 AM > To: Zhi, Yong <[email protected]> > Cc: Linux Media Mailing List <[email protected]>; Sakari Ailus > <[email protected]>; Mani, Rajmohan > <[email protected]>; Cao, Bingbu <[email protected]> > 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 <[email protected]> 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 <[email protected]> > > Signed-off-by: Cao Bing Bu <[email protected]> > > --- > > 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
