drm/imx: imx-ldb: fix out of bounds array access warning
Subsystems (5)
PATCH v2 1/1 drm/imx: imx-ldb: fix out of bounds array access warning
From: Arnd Bergmann <[email protected]>
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.
---
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(+)
drivers/gpu/drm/imx/imx-ldb.c +12 -0
@@ -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__);
Signature · 6 lines
2.29.2 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 16 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When
CONFIG_OFis 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
Kconfigdepedency onCONFIG_OF, but Liu Ying points out that the driver may hit the out-of-bounds problem at runtime anyway.
Quoted 6 lines
Signed-off-by Arnd Bergmann <[email protected]>---
v2: fix subject line
expand patch description
print mux number
check upper bound as well
[]
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0
[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 37 lines · Joe Perches
Quoted 16 lines · Arnd Bergmann
From: Arnd Bergmann <[email protected]>
When
CONFIG_OFis 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
Kconfigdepedency onCONFIG_OF, but Liu Ying points out that the driver may hit the out-of-bounds problem at runtime anyway.Quoted 6 lines
Signed-off-by Arnd Bergmann <[email protected]>---
v2: fix subject line
expand patch description
print mux number
check upper bound as well[]
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
Quoted 8 lines
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 asERR_PTRis 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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 1 line · Arnd Bergmann
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <[email protected]> wrote:
[]
Quoted 21 lines
Quoted 12 lines
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
Quoted 8 lines
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 asERR_PTRis converting an int a void * to decode the error type and emit it as a string.
Quoted 7 lines
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.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
RFC PATCH 1/1 vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 30 lines · Joe Perches
Quoted 1 line · Arnd Bergmann
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <[email protected]> wrote:
[]
Quoted 27 lines
Quoted 12 lines
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
Quoted 8 lines
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 asERR_PTRis 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.
Quoted 9 lines
Quoted 1 line
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_PTRwith mux >ARRAY_SIZEso perhaps %d without theERR_PTRuse makes the most sense.
Maybe it's better to output non PTR_ERR %pe uses as decimal so this sort of code would work.
--- lib/vsprintf.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)
lib/vsprintf.c +13 -11
@@ -619,19 +619,23 @@ static char *string_nocheck(char *buf, char *end, const char *s,return widen_string (buf, len, end, spec); } -static char *err_ptr (char *buf,char *end,void *ptr, -struct printf_spec spec) +static noinline_for_stack +char *err_ptr(char *buf, char *end, void *ptr, struct printf_spec spec) {int err =PTR_ERR (ptr); -const char *sym =errname (err); -if (sym) -return string_nocheck (buf, end, sym, spec); +if (IS_ERR (ptr)) { +const char *sym =errname (err); + +if (sym) +return string_nocheck (buf, end, sym, spec); + }/* -* Somebody passed ERR_PTR(-1234) or some other non-existing -* Efoo - or perhaps CONFIG_SYMBOLIC_ERRNAME=n. Fall back to -* printing it as its decimal representation. +* Somebody passed ERR_PTR(-1234) or some other non-existing -E<FOO> +* or perhaps CONFIG_SYMBOLIC_ERRNAME=n +* or perhaps a positive number like an array index +* Fall back to printing it as its decimal representation. */ spec.flags |=SIGN ; spec.base =10 ;
@@ -2407,9 +2411,7 @@ char *pointer(const char *fmt, char *buf, char *end, void *ptr,case 'x' :return pointer_string (buf, end, ptr, spec);case 'e' : -/* %pe with a non-ERR_PTR gets treated as plain %p */ -if (!IS_ERR (ptr)) -break ; +/* %pe with a non-ERR_PTR(ptr) gets treated as %ld */ return err_ptr (buf, end, ptr, spec);case 'u' :case 'k' :
---
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Rasmus Villemoes
reviewerRe: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 35 lines · Joe Perches
Quoted 34 lines · Joe Perches
Quoted 1 line · Arnd Bergmann
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <[email protected]> wrote:
[]
Quoted 21 lines
Quoted 12 lines
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
Quoted 8 lines
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 asERR_PTRis converting an int a void * to decode the error type and emit it as a string.Quoted 7 lines
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.
Quoted 8 lines
Quoted 4 lines
Up to you, vsprintf would emit the positive mux as a funky hashed hex value by default if you use
ERR_PTRwith mux >ARRAY_SIZEso perhaps %d without theERR_PTRuse 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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 42 lines · Rasmus Villemoes
Quoted 41 lines · Joe Perches
Quoted 38 lines · Joe Perches
Quoted 1 line · Arnd Bergmann
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches <[email protected]> wrote:
[]
Quoted 21 lines
Quoted 12 lines
drivers/gpu/drm/imx/imx-ldb.c
Quoted 1 line
drivers/gpu/drm/imx/imx-ldb.c +0
-0[]
drivers/gpu/drm/imx/imx-ldb.c static void imx_ldb_encoder_enable(struct drm_encoder *encoder) · line 202
Quoted 7 lines
drivers/gpu/drm/imx/imx-ldb.c +3
-0@@ -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.
Quoted 8 lines
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 asERR_PTRis converting an int a void * to decode the error type and emit it as a string.Quoted 7 lines
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_PTRwith mux >ARRAY_SIZEso perhaps %d without theERR_PTRuse makes the most sense.
Quoted 5 lines
Quoted 2 lines
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.
Quoted 9 lines
If the code wants a cute
-EFOOstring explaining what's wrong, what about "%pe",ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error messagesif (mux < 0)
...
else if (mux >=ARRAY_SIZE())
...
Multiple tests, more unnecessary code, multiple format strings, etc...
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Rasmus Villemoes
reviewerRe: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 7 lines · Joe Perches
Quoted 6 lines · Rasmus Villemoes
Quoted 3 lines · Joe Perches
Maybe it's better to output non
PTR_ERR%pe uses as decimal so this sort of code would work.
Quoted 6 lines
Quoted 2 lines
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.
Quoted 3 lines
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.
Quoted 10 lines
Quoted 8 lines
If the code wants a cute
-EFOOstring explaining what's wrong, what about "%pe",ERR_PTR(mux < 0 : mux : -ERANGE)? Or two separate error messagesif (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
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 20 lines · Rasmus Villemoes
Quoted 7 lines · Joe Perches
Quoted 6 lines · Rasmus Villemoes
Quoted 3 lines · Joe Perches
Maybe it's better to output non
PTR_ERR%pe uses as decimal so this sort of code would work.Quoted 6 lines
Quoted 2 lines
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_PTRpassed to an extension that would derefence the pointer; enforcing that onlyERR_PTRsare passed to %pe (or falling back to %p) is another of those safeguards.
Quoted 7 lines
Quoted 3 lines
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_PTRunless (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
Quoted 2 lines
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Rasmus Villemoes
reviewerRe: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 25 lines · Joe Perches
Quoted 24 lines · Rasmus Villemoes
Quoted 7 lines · Joe Perches
Quoted 6 lines · Rasmus Villemoes
Quoted 3 lines · Joe Perches
Maybe it's better to output non
PTR_ERR%pe uses as decimal so this sort of code would work.Quoted 6 lines
Quoted 2 lines
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_PTRpassed to an extension that would derefence the pointer; enforcing that onlyERR_PTRsare passed to %pe (or falling back to %p) is another of those safeguards.Quoted 3 lines
It's already possible to intentionally 'leak' the ptr value by using %pe, -ptr so I think that's not really an issue.
Quoted 5 lines
Quoted 3 lines
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_PTRunless (unsigned long)ptr is < 4095 or so.You are confusing
ERR_PTRwithIS_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?
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal
Quoted 4 lines · Rasmus Villemoes
Quoted 1 line · Joe Perches
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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
kernel test robot
build botI love your patch! Perhaps something to improve:
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
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
| %pKconfig 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(a)lists.01.org