| Message ID | 20230214102827.920927-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: imx-jpeg: avoid array overflow | expand |
Hi Arnd, Thank you for the patch. On Tue, Feb 14, 2023 at 11:28:20AM +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > gcc-9 (unlike newer versions) reports a possible array overflow > in mxc_jpeg_dec_irq(): > > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c: In function 'mxc_jpeg_dec_irq': > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c:641:28: error: array subscript 2 is above array bounds of 'u32[2]' {aka 'unsigned int[2]'} [-Werror=array-bounds] > 641 | size += q_data->sizeimage[i]; > | ~~~~~~~~~~~~~~~~~^~~ > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c:641:28: error: array subscript 3 is above array bounds of 'u32[2]' {aka 'unsigned int[2]'} [-Werror=array-bounds] > > The compiler clearly deduces that fmt->mem_planes is at least '2' if > this code line is reached, and that fmt->comp_planes must be at least > one more for the loop to make sense. However, this does not actually > seem to be the case in the initialized values, so I would guess that > this part of the function is never reached in practice. > > As a workaround, add a compile-time condition that skips any out-of-range > array indices. > > Fixes: ccc9f1db9c6b ("media: imx-jpeg: Support contiguous and non contiguous format") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > index f085f14d676a..7a667bfc2424 100644 > --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c > @@ -638,7 +638,8 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no) > > size = q_data->sizeimage[fmt->mem_planes - 1]; > for (i = fmt->mem_planes; i < fmt->comp_planes; i++) > - size += q_data->sizeimage[i]; > + if (i < MXC_JPEG_MAX_PLANES) > + size += q_data->sizeimage[i]; There's a risk someone will try to optimize this later, realizing that the check isn't needed, and we'll get the warning back. A comment would thus be useful. Also, how about writing it as for (i = fmt->mem_planes; i < min(fmt->comp_planes, MXC_JPEG_MAX_PLANES); i++) size += q_data->sizeimage[i]; > > return size; > }
diff --git a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c index f085f14d676a..7a667bfc2424 100644 --- a/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c +++ b/drivers/media/platform/nxp/imx-jpeg/mxc-jpeg.c @@ -638,7 +638,8 @@ static u32 mxc_jpeg_get_plane_size(struct mxc_jpeg_q_data *q_data, u32 plane_no) size = q_data->sizeimage[fmt->mem_planes - 1]; for (i = fmt->mem_planes; i < fmt->comp_planes; i++) - size += q_data->sizeimage[i]; + if (i < MXC_JPEG_MAX_PLANES) + size += q_data->sizeimage[i]; return size; }