RE: [PATCH 1/2] media: intel-ipu3: cio2: fix a crash with out-of-bounds access
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:tf...@chromium.org]
> Sent: Tuesday, January 16, 2018 10:40 AM
> To: Zhi, Yong
> Cc: Linux Media Mailing List ; Sakari Ailus
> ; Mani, Rajmohan ;
> Cao, Bingbu
> 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 wrote:
> > Hi, Tomasz,
> >
> > Thanks for the patch review.
> >
> >> -Original Message-
> >> From: Tomasz Figa [mailto:tf...@chromium.org]
> >> Sent: Friday, January 12, 2018 12:17 AM
> >> To: Zhi, Yong
> >> Cc: Linux Media Mailing List ; Sakari
> >> Ailus ; Mani, Rajmohan
> >> ; Cao, Bingbu
> >> 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 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
> >> > Signed-off-by: Cao Bing Bu
> >> > ---
> >> > 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] media: staging/intel-ipu3: mark PM function as __maybe_unused
Hi, Bergmann,
Thanks for your patch.
Reviewed-by: Cao, Bingbu
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:29 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ;
> Tomasz Figa ; Qiu, Tian Shu
> ; Cao, Bingbu ; linux-
> me...@vger.kernel.org; de...@driverdev.osuosl.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: mark PM function as
> __maybe_unused
>
> The imgu_rpm_dummy_cb() looks like an API misuse that is explained in
> the comment above it. Aside from that, it also causes a warning when
> power management support is disabled:
>
> drivers/staging/media/ipu3/ipu3.c:794:12: error: 'imgu_rpm_dummy_cb'
> defined but not used [-Werror=unused-function]
>
> The warning is at least easy to fix by marking the function as
> __maybe_unused.
>
> Fixes: 7fc7af649ca7 ("media: staging/intel-ipu3: Add imgu top level pci
> device driver")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3.c
> b/drivers/staging/media/ipu3/ipu3.c
> index d575ac78c8f0..d00d26264c37 100644
> --- a/drivers/staging/media/ipu3/ipu3.c
> +++ b/drivers/staging/media/ipu3/ipu3.c
> @@ -791,7 +791,7 @@ static int __maybe_unused imgu_resume(struct device
> *dev)
> * PCI rpm framework checks the existence of driver rpm callbacks.
> * Place a dummy callback here to avoid rpm going into error state.
> */
> -static int imgu_rpm_dummy_cb(struct device *dev)
> +static __maybe_unused int imgu_rpm_dummy_cb(struct device *dev)
> {
> return 0;
> }
> --
> 2.20.0
RE: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
__
BRs,
Cao, Bingbu
> -Original Message-
> From: Arnd Bergmann [mailto:a...@arndb.de]
> Sent: Tuesday, March 5, 2019 4:28 AM
> To: Sakari Ailus ; Mauro Carvalho Chehab
> ; Greg Kroah-Hartman
> Cc: Arnd Bergmann ; Zhi, Yong ; Cao,
> Bingbu ; linux-media@vger.kernel.org;
> de...@driverdev.osuosl.org; linux-ker...@vger.kernel.org
> Subject: [PATCH] media: staging/intel-ipu3: reduce kernel stack usage
>
> The imgu_css_queue structure is too large to be put on the kernel stack,
> as we can see in 32-bit builds:
>
> drivers/staging/media/ipu3/ipu3-css.c: In function 'imgu_css_fmt_try':
> drivers/staging/media/ipu3/ipu3-css.c:1863:1: error: the frame size of
> 1172 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> By dynamically allocating this array, the stack usage goes down to an
> acceptable 140 bytes for the same x86-32 configuration.
>
> Fixes: f5f2e4273518 ("media: staging/intel-ipu3: Add css pipeline
> programming")
> Signed-off-by: Arnd Bergmann
> ---
> drivers/staging/media/ipu3/ipu3-css.c | 25 +++--
> 1 file changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/staging/media/ipu3/ipu3-css.c
> b/drivers/staging/media/ipu3/ipu3-css.c
> index 15ab77e4b766..664c14b7a518 100644
> --- a/drivers/staging/media/ipu3/ipu3-css.c
> +++ b/drivers/staging/media/ipu3/ipu3-css.c
> @@ -3,6 +3,7 @@
>
> #include
> #include
> +#include
>
> #include "ipu3-css.h"
> #include "ipu3-css-fw.h"
> @@ -1744,7 +1745,7 @@ int imgu_css_fmt_try(struct imgu_css *css,
> struct v4l2_rect *const bds = &r[IPU3_CSS_RECT_BDS];
> struct v4l2_rect *const env = &r[IPU3_CSS_RECT_ENVELOPE];
> struct v4l2_rect *const gdc = &r[IPU3_CSS_RECT_GDC];
> - struct imgu_css_queue q[IPU3_CSS_QUEUES];
> + struct imgu_css_queue *q = kcalloc(IPU3_CSS_QUEUES, sizeof(struct
> +imgu_css_queue), GFP_KERNEL);
Could you use the devm_kcalloc()?
> struct v4l2_pix_format_mplane *const in =
> &q[IPU3_CSS_QUEUE_IN].fmt.mpix;
> struct v4l2_pix_format_mplane *const out = @@ -1753,6 +1754,11 @@
> int imgu_css_fmt_try(struct imgu_css *css,
> &q[IPU3_CSS_QUEUE_VF].fmt.mpix;
> int i, s, ret;
>
> + if (!q) {
> + ret = -ENOMEM;
> + goto out;
> + }
[Cao, Bingbu]
The goto here is wrong, you can just report an error, and I prefer it is next
to the alloc.
> +
> /* Adjust all formats, get statistics buffer sizes and formats */
> for (i = 0; i < IPU3_CSS_QUEUES; i++) {
> if (fmts[i])
> @@ -1766,7 +1772,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_notice(css->dev, "can not initialize queue %s\n",
> qnames[i]);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> }
> for (i = 0; i < IPU3_CSS_RECTS; i++) { @@ -1788,7 +1795,8 @@ int
> imgu_css_fmt_try(struct imgu_css *css,
> if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_IN]) ||
> !imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) {
> dev_warn(css->dev, "required queues are disabled\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> if (!imgu_css_queue_enabled(&q[IPU3_CSS_QUEUE_OUT])) { @@ -1829,7
> +1837,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> ret = imgu_css_find_binary(css, pipe, q, r);
> if (ret < 0) {
> dev_err(css->dev, "failed to find suitable binary\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> css->pipes[pipe].bindex = ret;
>
> @@ -1843,7 +1852,8 @@ int imgu_css_fmt_try(struct imgu_css *css,
> IPU3_CSS_QUEUE_TO_FLAGS(i))) {
> dev_err(css->dev,
> "final resolution adjustment failed\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
> *fmts[i] = q[i].fmt.mpix;
> }
> @@ -1859,7 +1869,10 @@ int imgu_css_fmt_try(struct imgu_css *css,
>bds->width, bds->height, gdc->width, gdc->height,
>out->width, out->height, vf->width, vf->height);
>
> - return 0;
> + ret = 0;
> +out:
> + kfree(q);
> + return ret;
> }
>
> int imgu_css_fmt_set(struct imgu_css *css,
> --
> 2.20.0
