| Message ID | 20210324121832.3714570-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/imx: imx-ldb: fix out of bounds array access warning | expand |
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote: > From: Arnd Bergmann <arnd@arndb.de> > > 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 <arnd@arndb.de> > --- > 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.
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 <lkp@intel.com>
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",
| ~^
| |
| int
| %p
Kconfig warnings: (for reference only)
WARNING: unmet direct dependencies detected for FRAME_POINTER
Depends on DEBUG_KERNEL && (M68K || UML || SUPERH) || ARCH_WANT_FRAME_POINTERS
Selected by
- FAULT_INJECTION_STACKTRACE_FILTER && FAULT_INJECTION_DEBUG_FS && STACKTRACE_SUPPORT && !X86_64 && !MIPS && !PPC && !S390 && !MICROBLAZE && !ARM && !ARC && !X86
vim +201 drivers/gpu/drm/imx/imx-ldb.c
192
193 static void imx_ldb_encoder_enable(struct drm_encoder *encoder)
194 {
195 struct imx_ldb_channel *imx_ldb_ch = enc_to_imx_ldb_ch(encoder);
196 struct imx_ldb *ldb = imx_ldb_ch->ldb;
197 int dual = ldb->ldb_ctrl & LDB_SPLIT_MODE_EN;
198 int mux = drm_of_encoder_active_port_id(imx_ldb_ch->child, encoder);
199
200 if (mux < 0 || mux >= ARRAY_SIZE(ldb->clk_sel)) {
> 201 dev_warn(ldb->dev, "%s: invalid mux %d\n",
202 __func__, ERR_PTR(mux));
203 return;
204 }
205
206 drm_panel_prepare(imx_ldb_ch->panel);
207
208 if (dual) {
209 clk_set_parent(ldb->clk_sel[mux], ldb->clk[0]);
210 clk_set_parent(ldb->clk_sel[mux], ldb->clk[1]);
211
212 clk_prepare_enable(ldb->clk[0]);
213 clk_prepare_enable(ldb->clk[1]);
214 } else {
215 clk_set_parent(ldb->clk_sel[mux], ldb->clk[imx_ldb_ch->chno]);
216 }
217
218 if (imx_ldb_ch == &ldb->channel[0] || dual) {
219 ldb->ldb_ctrl &= ~LDB_CH0_MODE_EN_MASK;
220 if (mux == 0 || ldb->lvds_mux)
221 ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI0;
222 else if (mux == 1)
223 ldb->ldb_ctrl |= LDB_CH0_MODE_EN_TO_DI1;
224 }
225 if (imx_ldb_ch == &ldb->channel[1] || dual) {
226 ldb->ldb_ctrl &= ~LDB_CH1_MODE_EN_MASK;
227 if (mux == 1 || ldb->lvds_mux)
228 ldb->ldb_ctrl |= LDB_CH1_MODE_EN_TO_DI1;
229 else if (mux == 0)
230 ldb->ldb_ctrl |= LDB_CH1_MODE_EN_TO_DI0;
231 }
232
233 if (ldb->lvds_mux) {
234 const struct bus_mux *lvds_mux = NULL;
235
236 if (imx_ldb_ch == &ldb->channel[0])
237 lvds_mux = &ldb->lvds_mux[0];
238 else if (imx_ldb_ch == &ldb->channel[1])
239 lvds_mux = &ldb->lvds_mux[1];
240
241 regmap_update_bits(ldb->regmap, lvds_mux->reg, lvds_mux->mask,
242 mux << lvds_mux->shift);
243 }
244
245 regmap_write(ldb->regmap, IOMUXC_GPR2, ldb->ldb_ctrl);
246
247 drm_panel_enable(imx_ldb_ch->panel);
248 }
249
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <joe@perches.com> wrote: > > On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > > > 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 <arnd@arndb.de> > > --- > > 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
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote: > On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <joe@perches.com> 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.
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 > 170000) { dev_warn(ldb->dev, "%s: mode exceeds 170 MHz pixel clock\n", __func__);