diff mbox series

[v2] drm/imx: imx-ldb: fix out of bounds array access warning

Message ID 20210324121832.3714570-1-arnd@kernel.org
State Superseded
Headers show
Series [v2] drm/imx: imx-ldb: fix out of bounds array access warning | expand

Commit Message

Arnd Bergmann March 24, 2021, 12:17 p.m. UTC 
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
---
 drivers/gpu/drm/imx/imx-ldb.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

-- 
2.29.2

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Comments

Joe Perches March 24, 2021, 2:20 p.m. UTC | #1 
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.
kernel test robot March 24, 2021, 4:14 p.m. UTC | #2 
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Arnd Bergmann March 24, 2021, 4:42 p.m. UTC | #3 
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
Joe Perches March 24, 2021, 4:52 p.m. UTC | #4 
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.
Rasmus Villemoes March 24, 2021, 5:33 p.m. UTC | #5 
On 24/03/2021 18.20, Joe Perches wrote:
> On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
>> 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.
>>

> 
> Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> sort of code would work.

No, because that would leak the pointer value when somebody has
accidentally passed a real kernel pointer to %pe.

If the code wants a cute -EFOO string explaining what's wrong, what
about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
messages

if (mux < 0)
  ...
else if (mux >= ARRAY_SIZE())
  ...

Rasmus
Joe Perches March 24, 2021, 7:24 p.m. UTC | #6 
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 18.20, Joe Perches wrote:
> > On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
> > > 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.
> > > 
> 
> > 
> > Maybe it's better to output non PTR_ERR %pe uses as decimal so this
> > sort of code would work.
> 
> No, because that would leak the pointer value when somebody has
> accidentally passed a real kernel pointer to %pe.

I think it's not really an issue.

_All_ code that uses %p<foo> extensions need inspection anyway.

It's already possible to intentionally 'leak' the ptr value
by using %pe, -ptr so I think that's not really an issue.

> 
> If the code wants a cute -EFOO string explaining what's wrong, what
> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
> messages
> 
> if (mux < 0)
>   ...
> else if (mux >= ARRAY_SIZE())
>   ...

Multiple tests, more unnecessary code, multiple format strings, etc...
Rasmus Villemoes March 24, 2021, 9:27 p.m. UTC | #7 
On 24/03/2021 20.24, Joe Perches wrote:
> On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
>> On 24/03/2021 18.20, Joe Perches wrote:
>>
>>>
>>> Maybe it's better to output non PTR_ERR %pe uses as decimal so this
>>> sort of code would work.
>>
>> No, because that would leak the pointer value when somebody has
>> accidentally passed a real kernel pointer to %pe.
> 
> I think it's not really an issue.
> 
> _All_ code that uses %p<foo> extensions need inspection anyway.

There are now a bunch of sanity checks in place that catch e.g. an
ERR_PTR passed to an extension that would derefence the pointer;
enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)
is another of those safeguards.

> It's already possible to intentionally 'leak' the ptr value
> by using %pe, -ptr so I think that's not really an issue.
> 

Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.
How would that leak the value if ptr is an ordinary kernel pointer?
That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.

If you want to print the pointer value just do %px. No need for silly
games. What I'm talking about is preventing _un_intentionally leaking a
valid kernel pointer value. So no, a non-ERR_PTR passed to %pe is not
going to be printed as-is, not in decimal or hexadecimal or roman numerals.

>> If the code wants a cute -EFOO string explaining what's wrong, what
>> about "%pe", ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error
>> messages
>>
>> if (mux < 0)
>>   ...
>> else if (mux >= ARRAY_SIZE())
>>   ...
> 
> Multiple tests, more unnecessary code, multiple format strings, etc...

Agreed, I'm not really advocating for the latter; the former suggestion
is IMO a pretty concise way of providing useful information in dmesg.

Rasmus
Joe Perches March 24, 2021, 10:18 p.m. UTC | #8 
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 20.24, Joe Perches wrote:

> > On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:

> > > On 24/03/2021 18.20, Joe Perches wrote:

> > > 

> > > > 

> > > > Maybe it's better to output non PTR_ERR %pe uses as decimal so this

> > > > sort of code would work.

> > > 

> > > No, because that would leak the pointer value when somebody has

> > > accidentally passed a real kernel pointer to %pe.

> > 

> > I think it's not really an issue.

> > 

> > _All_ code that uses %p<foo> extensions need inspection anyway.

> 

> There are now a bunch of sanity checks in place that catch e.g. an

> ERR_PTR passed to an extension that would derefence the pointer;

> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)

> is another of those safeguards.

> 

> > It's already possible to intentionally 'leak' the ptr value

> > by using %pe, -ptr so I think that's not really an issue.

> > 

> 

> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.

> How would that leak the value if ptr is an ordinary kernel pointer?

> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.


You are confusing ERR_PTR with IS_ERR

ERR_PTR is just

include/linux/err.h:static inline void * __must_check ERR_PTR(long error)
include/linux/err.h-{
include/linux/err.h-    return (void *) error;
include/linux/err.h-}f 

> If you want to print the pointer value just do %px. No need for silly

> games.


There's no silly game here.  %pe would either print a string or a value.
It already does that in 2 cases.


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Rasmus Villemoes March 24, 2021, 10:36 p.m. UTC | #9 
On 24/03/2021 23.18, Joe Perches wrote:
> On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:

>> On 24/03/2021 20.24, Joe Perches wrote:

>>> On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:

>>>> On 24/03/2021 18.20, Joe Perches wrote:

>>>>

>>>>>

>>>>> Maybe it's better to output non PTR_ERR %pe uses as decimal so this

>>>>> sort of code would work.

>>>>

>>>> No, because that would leak the pointer value when somebody has

>>>> accidentally passed a real kernel pointer to %pe.

>>>

>>> I think it's not really an issue.

>>>

>>> _All_ code that uses %p<foo> extensions need inspection anyway.

>>

>> There are now a bunch of sanity checks in place that catch e.g. an

>> ERR_PTR passed to an extension that would derefence the pointer;

>> enforcing that only ERR_PTRs are passed to %pe (or falling back to %p)

>> is another of those safeguards.

>>

>>> It's already possible to intentionally 'leak' the ptr value

>>> by using %pe, -ptr so I think that's not really an issue.

>>>

>>

>> Huh, what? I assume -ptr is shorthand for (void*)-(unsigned long)ptr.

>> How would that leak the value if ptr is an ordinary kernel pointer?

>> That's not an ERR_PTR unless (unsigned long)ptr is < 4095 or so.

> 

> You are confusing ERR_PTR with IS_ERR


No I'm not, I'm just being slightly sloppy - obviously when I say "not
an ERR_PTR" I mean "not the result of ERR_PTR applied to a negative
errno value", or "not the result of a valid invocation of ERR_PTR". But
yes, feel free to read "not an ERR_PTR" as "something for which IS_ERR
is false".

Can you expand on why you think %pe, -ptr  would leak the value of ptr?

>> If you want to print the pointer value just do %px. No need for silly

>> games.

> 

> There's no silly game here.  %pe would either print a string or a value.


A hashed value, that is, never the raw value.

> It already does that in 2 cases.


Yes, if you pass it ERR_PTR(-1234) (where no E symbol exists) or
ERR_PTR(-EINVAL) but CONFIG_SYMBOLIC_ERRNAME=n, it prints the value in
decimal, because people will probably recognize "-22" and values in that
range don't reveal anything about the kernel image. Anything outside
[-4095,0] or so is hashed.

Rasmus
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Joe Perches March 24, 2021, 10:46 p.m. UTC | #10 
On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote:
> On 24/03/2021 23.18, Joe Perches wrote:

> > There's no silly game here.  %pe would either print a string or a value.

> 

> A hashed value, that is, never the raw value.


There is value in printing the raw value.
As discussed, it can simplify the code.

The worry about exposing a ptr value is IMO overstated.

It's trivial to inspect the uses and _all_ %p<FOO> uses need inspection
and validation at acceptance anyway.


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff mbox series

Patch

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__);