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

Subsystems: drm drivers, drm drivers and misc gpu patches, the rest

STALE1957d

11 messages, 3 authors, 2021-03-24 · open the first message on its own page

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

From: Arnd Bergmann <arnd@kernel.org>
Date: 2021-03-24 12:19:24

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(+)
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__);
-- 
2.29.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 14:21:34

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
[]
quoted hunk
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted hunk
@@ -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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

From: Arnd Bergmann <arnd@kernel.org>
Date: 2021-03-24 16:43:50

On Wed, Mar 24, 2021 at 3:20 PM Joe Perches [off-list ref] wrote:
On Wed, 2021-03-24 at 13:17 +0100, Arnd Bergmann wrote:
quoted
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
[]
quoted
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted
@@ -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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 16:52:56

On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches [off-list ref] wrote:
[]
quoted
quoted
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted
@@ -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.




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 17:21:31

On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
quoted
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches [off-list ref] wrote:
[]
quoted
quoted
quoted
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted
@@ -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.
---
 lib/vsprintf.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 3600db686fa4..debdd1c62038 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -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':

---


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: 2021-03-24 17:34:07

On 24/03/2021 18.20, Joe Perches wrote:
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
quoted
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
quoted
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches [off-list ref] wrote:
[]
quoted
quoted
quoted
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted
@@ -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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 19:24:59

On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
On 24/03/2021 18.20, Joe Perches wrote:
quoted
On Wed, 2021-03-24 at 09:52 -0700, Joe Perches wrote:
quoted
On Wed, 2021-03-24 at 17:42 +0100, Arnd Bergmann wrote:
quoted
On Wed, Mar 24, 2021 at 3:20 PM Joe Perches [off-list ref] wrote:
[]
quoted
quoted
quoted
diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
[]
quoted
@@ -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.
quoted
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...



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: 2021-03-24 21:28:51

On 24/03/2021 20.24, Joe Perches wrote:
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
quoted
On 24/03/2021 18.20, Joe Perches wrote:
quoted
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.
quoted
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 22:19:28

On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
On 24/03/2021 20.24, Joe Perches wrote:
quoted
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
quoted
On 24/03/2021 18.20, Joe Perches wrote:
quoted
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.
quoted
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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Date: 2021-03-24 22:37:35

On 24/03/2021 23.18, Joe Perches wrote:
On Wed, 2021-03-24 at 22:27 +0100, Rasmus Villemoes wrote:
quoted
On 24/03/2021 20.24, Joe Perches wrote:
quoted
On Wed, 2021-03-24 at 18:33 +0100, Rasmus Villemoes wrote:
quoted
On 24/03/2021 18.20, Joe Perches wrote:
quoted
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.
quoted
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?
quoted
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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFC patch] vsprintf: Allow %pe to print non PTR_ERR %pe uses as decimal

From: Joe Perches <joe@perches.com>
Date: 2021-03-24 22:46:55

On Wed, 2021-03-24 at 23:36 +0100, Rasmus Villemoes wrote:
On 24/03/2021 23.18, Joe Perches wrote:
quoted
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.



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help