Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
> On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
[]
> > > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> > []
> > > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct
> > > drm_encoder *encoder)
> > > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> > >
> > > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > > + __func__, ERR_PTR(mux));
> >
> > This does not compile without warnings.
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> > drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> > of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> > 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > | ^~
> >
> > If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> > is converting an int a void * to decode the error type and
> > emit it as a string.
>
> Sorry about that.
>
> I decided against using ERR_PTR() in order to also check for
> positive array overflow, but the version I tested was different from
> the version I sent.
>
> v3 coming.
Thanks. No worries.
Up to you, vsprintf would emit the positive mux as a funky hashed
hex value by default if you use ERR_PTR with mux > ARRAY_SIZE so
perhaps %d without the ERR_PTR use makes the most sense.
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches wrote:
>
> On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann
> >
> > When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> > about out of bounds array access:
> >
> > drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> > drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> > array bounds of 'struct clk *[4]' [-Werror=array-bounds]
> >
> > Add an error check before the index is used, which helps with the
> > warning, as well as any possible other error condition that may be
> > triggered at runtime.
> >
> > The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> > but Liu Ying points out that the driver may hit the out-of-bounds
> > problem at runtime anyway.
> >
> > Signed-off-by: Arnd Bergmann
> > ---
> > v2: fix subject line
> > expand patch description
> > print mux number
> > check upper bound as well
> []
> > diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> []
> > @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> > *encoder)
> > int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> > int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
> >
> > + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> > + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> > + __func__, ERR_PTR(mux));
>
> This does not compile without warnings.
>
> drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
> drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument
> of type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
> 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
> | ^~
>
> If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
> is converting an int a void * to decode the error type and
> emit it as a string.
Sorry about that.
I decided against using ERR_PTR() in order to also check for
positive array overflow, but the version I tested was different from
the version I sent.
v3 coming.
Arnd
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
Hi Arnd, I love your patch! Perhaps something to improve: [auto build test WARNING on shawnguo/for-next] [also build test WARNING on pza/reset/next drm-intel/for-linux-next drm-tip/drm-tip v5.12-rc4 next-20210324] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/drm-imx-imx-ldb-fix-out-of-bounds-array-access-warning/20210324-202112 base: https://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git for-next config: ia64-randconfig-r021-20210323 (attached as .config) compiler: ia64-linux-gcc (GCC) 9.3.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/1921451dcfc3ce8072884c286da96759e18ad102 git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Arnd-Bergmann/drm-imx-imx-ldb-fix-out-of-bounds-array-access-warning/20210324-202112 git checkout 1921451dcfc3ce8072884c286da96759e18ad102 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=ia64 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot All warnings (new ones prefixed by >>): In file included from arch/ia64/include/asm/pgtable.h:154, from include/linux/pgtable.h:6, from arch/ia64/include/asm/uaccess.h:40, from include/linux/uaccess.h:11, from arch/ia64/include/asm/sections.h:11, from include/linux/interrupt.h:20, from include/linux/trace_recursion.h:5, from include/linux/ftrace.h:10, from include/linux/kprobes.h:29, from include/linux/kgdb.h:19, from include/linux/fb.h:5, from include/drm/drm_crtc.h:31, from include/drm/drm_atomic.h:31, from drivers/gpu/drm/imx/imx-ldb.c:21: arch/ia64/include/asm/mmu_context.h: In function 'reload_context': arch/ia64/include/asm/mmu_context.h:127:41: warning: variable 'old_rr4' set but not used [-Wunused-but-set-variable] 127 | unsigned long rr0, rr1, rr2, rr3, rr4, old_rr4; | ^~~ In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/of_device.h:5, from drivers/gpu/drm/imx/imx-ldb.c:13: drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_encoder_enable': >> drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format '%d' expects argument >> of type 'int', but argument 4 has type 'void *' [-Wformat=] 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/gpu/drm/imx/imx-ldb.c:201:3: note: in expansion of macro 'dev_warn' 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~~~ drivers/gpu/drm/imx/imx-ldb.c:201:40: note: format string is defined here 201 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ~^ || |int | %p In file included from include/linux/device.h:15, from include/linux/node.h:18, from include/linux/cpu.h:17, from include/linux/of_device.h:5, from drivers/gpu/drm/imx/imx-ldb.c:13: drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_encoder_atomic_mode_set': drivers/gpu/drm/imx/imx-ldb.c:265:22: warning: format '%d' expects argument of type 'int', but argument 4 has type 'void *' [-Wformat=] 265 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~ include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt' 19 | #define dev_fmt(fmt) fmt | ^~~ drivers/gpu/drm/imx/imx-ldb.c:265:3: note: in expansion of macro 'dev_warn' 265 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ^~~~ drivers/gpu/drm/imx/imx-ldb.c:265:40: note: format string is defined here 265 | dev_warn(ldb->dev, "%s: invalid mux %d\n", | ~^ || |
Re: [PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_OF is disabled, building with 'make W=1' produces warnings
> about out of bounds array access:
>
> drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
> drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below
> array bounds of 'struct clk *[4]' [-Werror=array-bounds]
>
> Add an error check before the index is used, which helps with the
> warning, as well as any possible other error condition that may be
> triggered at runtime.
>
> The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
> but Liu Ying points out that the driver may hit the out-of-bounds
> problem at runtime anyway.
>
> Signed-off-by: Arnd Bergmann
> ---
> v2: fix subject line
> expand patch description
> print mux number
> check upper bound as well
[]
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
> @@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
> *encoder)
> int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
> int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
>
> + if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> + dev_warn(ldb->dev, "%s: invalid mux %d\n",
> + __func__, ERR_PTR(mux));
This does not compile without warnings.
drivers/gpu/drm/imx/imx-ldb.c: In function ‘imx_ldb_encoder_enable’:
drivers/gpu/drm/imx/imx-ldb.c:201:22: warning: format ‘%d’ expects argument of
type ‘int’, but argument 4 has type ‘void *’ [-Wformat=]
201 | dev_warn(ldb->dev, "%s: invalid mux %d\n",
| ^~
If you want to use ERR_PTR, the %d should be %pe as ERR_PTR
is converting an int a void * to decode the error type and
emit it as a string.
[PATCH] [v2] drm/imx: imx-ldb: fix out of bounds array access warning
From: Arnd Bergmann
When CONFIG_OF is disabled, building with 'make W=1' produces warnings
about out of bounds array access:
drivers/gpu/drm/imx/imx-ldb.c: In function 'imx_ldb_set_clock.constprop':
drivers/gpu/drm/imx/imx-ldb.c:186:8: error: array subscript -22 is below array
bounds of 'struct clk *[4]' [-Werror=array-bounds]
Add an error check before the index is used, which helps with the
warning, as well as any possible other error condition that may be
triggered at runtime.
The warning could be fixed by adding a Kconfig depedency on CONFIG_OF,
but Liu Ying points out that the driver may hit the out-of-bounds
problem at runtime anyway.
Signed-off-by: Arnd Bergmann
---
v2: fix subject line
expand patch description
print mux number
check upper bound as well
---
drivers/gpu/drm/imx/imx-ldb.c | 12
1 file changed, 12 insertions(+)
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index dbfe39e2f7f6..40310327fa76 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -197,6 +197,12 @@ static void imx_ldb_encoder_enable(struct drm_encoder
*encoder)
int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
+ if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
+ dev_warn(ldb->dev, "%s: invalid mux %d\n",
+__func__, ERR_PTR(mux));
+ return;
+ }
+
drm_panel_prepare(imx_ldb_ch->panel);
if (dual) {
@@ -255,6 +261,12 @@ imx_ldb_encoder_atomic_mode_set(struct drm_encoder
*encoder,
int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
u32 bus_format = imx_ldb_ch->bus_format;
+ if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
+ dev_warn(ldb->dev, "%s: invalid mux %d\n",
+__func__, ERR_PTR(mux));
+ return;
+ }
+
if (mode->clock > 17) {
dev_warn(ldb->dev,
"%s: mode exceeds 170 MHz pixel clock\n", __func__);
--
2.29.2
