[PATCH] hwmon: (lm90) Fix error return value from detect function

[PATCH] hwmon: (lm90) Fix error return value from detect function

[Guenter Roeck]

lm90_detect_nuvoton() is supposed to return NULL if it can not detect
a chip, or a pointer to the chip name if it does. Under some circumstances
it returns an error pointer instead. Some versions of gcc interpret an
ERR_PTR as region of size 0 and generate an error message.

  In function ‘__fortify_strlen’,
      inlined from ‘strlcpy’ at ./include/linux/fortify-string.h:159:10,
      inlined from ‘lm90_detect’ at drivers/hwmon/lm90.c:2550:2:
  ./include/linux/fortify-string.h:50:33: error:
      ‘__builtin_strlen’ reading 1 or more bytes from a region of size 0
     50 | #define __underlying_strlen     __builtin_strlen
        |                                 ^
  ./include/linux/fortify-string.h:141:24: note:
      in expansion of macro ‘__underlying_strlen’
    141 |                 return __underlying_strlen(p);
        |                        ^~~~~~~~~~~~~~~~~~~

Returning NULL instead of ERR_PTR() fixes the problem.

Fixes: c7cebce984a2 ("hwmon: (lm90) Rework detect function")
Reported-by: Ingo Molnar <mingo@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
It is interesting that some versions of gcc interpret an ERR_PTR this way.
It did find a real bug, though the error message is quite confusing.
Would it be possible to enhance the fortify functions to detect a constant
ERR_PTR at compile time ? I think that might be quite useful.

 drivers/hwmon/lm90.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/lm90.c b/drivers/hwmon/lm90.c
index 03d07da8c2dc..221de01a327a 100644
--- a/drivers/hwmon/lm90.c
+++ b/drivers/hwmon/lm90.c
@@ -2321,7 +2321,7 @@ static const char *lm90_detect_nuvoton(struct i2c_client *client, int chip_id,
 	const char *name = NULL;
 
 	if (config2 < 0)
-		return ERR_PTR(-ENODEV);
+		return NULL;
 
 	if (address == 0x4c && !(config1 & 0x2a) && !(config2 & 0xf8)) {
 		if (chip_id == 0x01 && convrate <= 0x09) {
-- 
2.36.2

Re: [PATCH] hwmon: (lm90) Fix error return value from detect function

[Ingo Molnar]

* Guenter Roeck <linux@roeck-us.net> wrote:

> lm90_detect_nuvoton() is supposed to return NULL if it can not detect
> a chip, or a pointer to the chip name if it does. Under some circumstances
> it returns an error pointer instead. Some versions of gcc interpret an
> ERR_PTR as region of size 0 and generate an error message.
> 
>   In function ‘__fortify_strlen’,
>       inlined from ‘strlcpy’ at ./include/linux/fortify-string.h:159:10,
>       inlined from ‘lm90_detect’ at drivers/hwmon/lm90.c:2550:2:
>   ./include/linux/fortify-string.h:50:33: error:
>       ‘__builtin_strlen’ reading 1 or more bytes from a region of size 0
>      50 | #define __underlying_strlen     __builtin_strlen
>         |                                 ^
>   ./include/linux/fortify-string.h:141:24: note:
>       in expansion of macro ‘__underlying_strlen’
>     141 |                 return __underlying_strlen(p);
>         |                        ^~~~~~~~~~~~~~~~~~~
> 
> Returning NULL instead of ERR_PTR() fixes the problem.
> 
> Fixes: c7cebce984a2 ("hwmon: (lm90) Rework detect function")
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Tested-by: Ingo Molnar <mingo@kernel.org>

Thanks,

	Ingo

Re: [PATCH] hwmon: (lm90) Fix error return value from detect function

[Kees Cook]

On Mon, Aug 08, 2022 at 03:15:04AM -0700, Guenter Roeck wrote:
> lm90_detect_nuvoton() is supposed to return NULL if it can not detect
> a chip, or a pointer to the chip name if it does. Under some circumstances
> it returns an error pointer instead. Some versions of gcc interpret an
> ERR_PTR as region of size 0 and generate an error message.
> 
>   In function ‘__fortify_strlen’,
>       inlined from ‘strlcpy’ at ./include/linux/fortify-string.h:159:10,
>       inlined from ‘lm90_detect’ at drivers/hwmon/lm90.c:2550:2:
>   ./include/linux/fortify-string.h:50:33: error:
>       ‘__builtin_strlen’ reading 1 or more bytes from a region of size 0
>      50 | #define __underlying_strlen     __builtin_strlen
>         |                                 ^
>   ./include/linux/fortify-string.h:141:24: note:
>       in expansion of macro ‘__underlying_strlen’
>     141 |                 return __underlying_strlen(p);
>         |                        ^~~~~~~~~~~~~~~~~~~
> 
> Returning NULL instead of ERR_PTR() fixes the problem.
> 
> Fixes: c7cebce984a2 ("hwmon: (lm90) Rework detect function")
> Reported-by: Ingo Molnar <mingo@kernel.org>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> It is interesting that some versions of gcc interpret an ERR_PTR this way.
> It did find a real bug, though the error message is quite confusing.
> Would it be possible to enhance the fortify functions to detect a constant
> ERR_PTR at compile time ? I think that might be quite useful.

Yeah, that should be possible. I suspect something like this might work:

	BUILD_BUG_ON(__builtin_constant_p(src) && IS_ERR_VALUE(src));
	BUILD_BUG_ON(__builtin_constant_p(dst) && IS_ERR_VALUE(dst));

Though I'm not sure how it'd play with GCC value range checker.

-- 
Kees Cook

Re: [PATCH] hwmon: (lm90) Fix error return value from detect function

[Kees Cook]

On Tue, Aug 09, 2022 at 03:24:04PM -0700, Kees Cook wrote:
> On Mon, Aug 08, 2022 at 03:15:04AM -0700, Guenter Roeck wrote:
> > lm90_detect_nuvoton() is supposed to return NULL if it can not detect
> > a chip, or a pointer to the chip name if it does. Under some circumstances
> > it returns an error pointer instead. Some versions of gcc interpret an
> > ERR_PTR as region of size 0 and generate an error message.
> > 
> >   In function ‘__fortify_strlen’,
> >       inlined from ‘strlcpy’ at ./include/linux/fortify-string.h:159:10,
> >       inlined from ‘lm90_detect’ at drivers/hwmon/lm90.c:2550:2:
> >   ./include/linux/fortify-string.h:50:33: error:
> >       ‘__builtin_strlen’ reading 1 or more bytes from a region of size 0
> >      50 | #define __underlying_strlen     __builtin_strlen
> >         |                                 ^
> >   ./include/linux/fortify-string.h:141:24: note:
> >       in expansion of macro ‘__underlying_strlen’
> >     141 |                 return __underlying_strlen(p);
> >         |                        ^~~~~~~~~~~~~~~~~~~
> > 
> > Returning NULL instead of ERR_PTR() fixes the problem.
> > 
> > Fixes: c7cebce984a2 ("hwmon: (lm90) Rework detect function")
> > Reported-by: Ingo Molnar <mingo@kernel.org>
> > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > Cc: Kees Cook <keescook@chromium.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > ---
> > It is interesting that some versions of gcc interpret an ERR_PTR this way.
> > It did find a real bug, though the error message is quite confusing.
> > Would it be possible to enhance the fortify functions to detect a constant
> > ERR_PTR at compile time ? I think that might be quite useful.
> 
> Yeah, that should be possible. I suspect something like this might work:
> 
> 	BUILD_BUG_ON(__builtin_constant_p(src) && IS_ERR_VALUE(src));
> 	BUILD_BUG_ON(__builtin_constant_p(dst) && IS_ERR_VALUE(dst));
> 
> Though I'm not sure how it'd play with GCC value range checker.

Yeah, looks like this doesn't work. These are all only able to check for
a single value. The GCC diagnostics depend on its internal value range
checking. This tripped because of the (sometimes buggy) "void * cast of
a literal value is always a NULL pointer dereference, so its size must
always be zero" which we've had to repeatedly work around. In this case,
it was a real error, though. :P

I'm hoping we can teach future GCC "treat literal casts in range
$foo-$bar to be NULL derefs", and we can hand it the ERR_PTR range.

-Kees

-- 
Kees Cook

Re: [PATCH] hwmon: (lm90) Fix error return value from detect function

[Guenter Roeck]

On Tue, Aug 09, 2022 at 03:39:59PM -0700, Kees Cook wrote:
> On Tue, Aug 09, 2022 at 03:24:04PM -0700, Kees Cook wrote:
> > On Mon, Aug 08, 2022 at 03:15:04AM -0700, Guenter Roeck wrote:
> > > lm90_detect_nuvoton() is supposed to return NULL if it can not detect
> > > a chip, or a pointer to the chip name if it does. Under some circumstances
> > > it returns an error pointer instead. Some versions of gcc interpret an
> > > ERR_PTR as region of size 0 and generate an error message.
> > > 
> > >   In function ‘__fortify_strlen’,
> > >       inlined from ‘strlcpy’ at ./include/linux/fortify-string.h:159:10,
> > >       inlined from ‘lm90_detect’ at drivers/hwmon/lm90.c:2550:2:
> > >   ./include/linux/fortify-string.h:50:33: error:
> > >       ‘__builtin_strlen’ reading 1 or more bytes from a region of size 0
> > >      50 | #define __underlying_strlen     __builtin_strlen
> > >         |                                 ^
> > >   ./include/linux/fortify-string.h:141:24: note:
> > >       in expansion of macro ‘__underlying_strlen’
> > >     141 |                 return __underlying_strlen(p);
> > >         |                        ^~~~~~~~~~~~~~~~~~~
> > > 
> > > Returning NULL instead of ERR_PTR() fixes the problem.
> > > 
> > > Fixes: c7cebce984a2 ("hwmon: (lm90) Rework detect function")
> > > Reported-by: Ingo Molnar <mingo@kernel.org>
> > > Cc: Linus Torvalds <torvalds@linux-foundation.org>
> > > Cc: Kees Cook <keescook@chromium.org>
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> > > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> > > ---
> > > It is interesting that some versions of gcc interpret an ERR_PTR this way.
> > > It did find a real bug, though the error message is quite confusing.
> > > Would it be possible to enhance the fortify functions to detect a constant
> > > ERR_PTR at compile time ? I think that might be quite useful.
> > 
> > Yeah, that should be possible. I suspect something like this might work:
> > 
> > 	BUILD_BUG_ON(__builtin_constant_p(src) && IS_ERR_VALUE(src));
> > 	BUILD_BUG_ON(__builtin_constant_p(dst) && IS_ERR_VALUE(dst));
> > 
> > Though I'm not sure how it'd play with GCC value range checker.
> 
> Yeah, looks like this doesn't work. These are all only able to check for
> a single value. The GCC diagnostics depend on its internal value range
> checking. This tripped because of the (sometimes buggy) "void * cast of
> a literal value is always a NULL pointer dereference, so its size must
> always be zero" which we've had to repeatedly work around. In this case,
> it was a real error, though. :P
> 
Guess it would have been too easy. I wonder if it might be able to come up
with a coccinelle script to find such issues. Anyway, thanks for trying and
for the review.

Guenter