[PATCH] hwmon: (applesmc) fix UB and udelay overflow

[PATCH] hwmon: (applesmc) fix UB and udelay overflow

[Nick Desaulniers]

Fixes the following 2 issues in the driver:
1. Left shifting a signed integer is undefined behavior. Unsigned
   integral types should be used for bitwise operations.
2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
   will result in a linkage failure when given a constant that's greater
   than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
   loop, resulting in later iterations overflowing the call to udelay.

2 is fixed via splitting the loop in two, iterating the first up to the
point where udelay would overflow, then switching to mdelay, as
suggested in Documentation/timers/timers-howto.rst.

Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/678
Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
 drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 183ff3d25129..2bc12812f52f 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -46,6 +46,7 @@
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
 #define APPLESMC_MAX_WAIT	0x20000
+#define APPLESMC_UDELAY_MAX	20000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
@@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
 static int wait_read(void)
 {
 	u8 status;
-	int us;
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	unsigned int us;
+
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
 	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* read: wait for smc to settle */
+		if (status & 0x01)
+			return 0;
+	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
@@ -177,10 +187,10 @@ static int wait_read(void)
 static int send_byte(u8 cmd, u16 port)
 {
 	u8 status;
-	int us;
+	unsigned int us;
 
 	outb(cmd, port);
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
@@ -196,6 +206,23 @@ static int send_byte(u8 cmd, u16 port)
 		udelay(APPLESMC_RETRY_WAIT);
 		outb(cmd, port);
 	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* write: wait for smc to settle */
+		if (status & 0x02)
+			continue;
+		/* ready: cmd accepted, return */
+		if (status & 0x04)
+			return 0;
+		/* timeout: give up */
+		if (us << 1 == APPLESMC_MAX_WAIT)
+			break;
+		/* busy: long wait and resend */
+		udelay(APPLESMC_RETRY_WAIT);
+		outb(cmd, port);
+	}
 
 	pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
 	return -EIO;
-- 
2.23.0.351.gc4317032e6-goog

Re: [PATCH] hwmon: (applesmc) fix UB and udelay overflow

[Nick Desaulniers]

On Tue, Sep 24, 2019 at 10:37 AM Nick Desaulniers
<ndesaulniers@google.com> wrote:
>
> Fixes the following 2 issues in the driver:
> 1. Left shifting a signed integer is undefined behavior. Unsigned
>    integral types should be used for bitwise operations.
> 2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
>    will result in a linkage failure when given a constant that's greater
>    than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
>    loop, resulting in later iterations overflowing the call to udelay.
>
> 2 is fixed via splitting the loop in two, iterating the first up to the
> point where udelay would overflow, then switching to mdelay, as
> suggested in Documentation/timers/timers-howto.rst.
>
> Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/678
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
>  drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 183ff3d25129..2bc12812f52f 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -46,6 +46,7 @@
>  #define APPLESMC_MIN_WAIT      0x0010
>  #define APPLESMC_RETRY_WAIT    0x0100
>  #define APPLESMC_MAX_WAIT      0x20000
> +#define APPLESMC_UDELAY_MAX    20000
>
>  #define APPLESMC_READ_CMD      0x10
>  #define APPLESMC_WRITE_CMD     0x11
> @@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
>  static int wait_read(void)
>  {
>         u8 status;
> -       int us;
> -       for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +       unsigned int us;
> +
> +       for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>                 udelay(us);
>                 status = inb(APPLESMC_CMD_PORT);
>                 /* read: wait for smc to settle */
>                 if (status & 0x01)
>                         return 0;
>         }
> +       /* switch to mdelay for longer sleeps */
> +       for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +               mdelay(us);
> +               status = inb(APPLESMC_CMD_PORT);
> +               /* read: wait for smc to settle */
> +               if (status & 0x01)
> +                       return 0;
> +       }
>
>         pr_warn("wait_read() fail: 0x%02x\n", status);
>         return -EIO;
> @@ -177,10 +187,10 @@ static int wait_read(void)
>  static int send_byte(u8 cmd, u16 port)
>  {
>         u8 status;
> -       int us;
> +       unsigned int us;
>
>         outb(cmd, port);
> -       for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +       for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>                 udelay(us);
>                 status = inb(APPLESMC_CMD_PORT);
>                 /* write: wait for smc to settle */
> @@ -196,6 +206,23 @@ static int send_byte(u8 cmd, u16 port)
>                 udelay(APPLESMC_RETRY_WAIT);
>                 outb(cmd, port);
>         }
> +       /* switch to mdelay for longer sleeps */
> +       for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +               mdelay(us);
> +               status = inb(APPLESMC_CMD_PORT);
> +               /* write: wait for smc to settle */
> +               if (status & 0x02)
> +                       continue;
> +               /* ready: cmd accepted, return */
> +               if (status & 0x04)
> +                       return 0;
> +               /* timeout: give up */
> +               if (us << 1 == APPLESMC_MAX_WAIT)
> +                       break;

Sorry, I need to modify the first for loop in this function to break
out properly. v2 inbound.

> +               /* busy: long wait and resend */
> +               udelay(APPLESMC_RETRY_WAIT);
> +               outb(cmd, port);
> +       }
>
>         pr_warn("send_byte(0x%02x, 0x%04x) fail: 0x%02x\n", cmd, port, status);
>         return -EIO;
> --
> 2.23.0.351.gc4317032e6-goog
>


-- 
Thanks,
~Nick Desaulniers

[PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Nick Desaulniers]

Fixes the following 2 issues in the driver:
1. Left shifting a signed integer is undefined behavior. Unsigned
   integral types should be used for bitwise operations.
2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
   will result in a linkage failure when given a constant that's greater
   than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
   loop, resulting in later iterations overflowing the call to udelay.

2 is fixed via splitting the loop in two, iterating the first up to the
point where udelay would overflow, then switching to mdelay, as
suggested in Documentation/timers/timers-howto.rst.

Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/678
Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1 -> V2:
* The first loop in send_byte() needs to break out on the same condition
  now. Technically, the loop condition could even be removed. The diff
  looks funny because of the duplicated logic between existing and newly
  added for loops.

 drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 183ff3d25129..c76adb504dff 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -46,6 +46,7 @@
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
 #define APPLESMC_MAX_WAIT	0x20000
+#define APPLESMC_UDELAY_MAX	20000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
@@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
 static int wait_read(void)
 {
 	u8 status;
-	int us;
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	unsigned int us;
+
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
 	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* read: wait for smc to settle */
+		if (status & 0x01)
+			return 0;
+	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
@@ -177,10 +187,10 @@ static int wait_read(void)
 static int send_byte(u8 cmd, u16 port)
 {
 	u8 status;
-	int us;
+	unsigned int us;
 
 	outb(cmd, port);
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
@@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
 		if (status & 0x04)
 			return 0;
 		/* timeout: give up */
+		if (us << 1 == APPLESMC_UDELAY_MAX)
+			break;
+		/* busy: long wait and resend */
+		udelay(APPLESMC_RETRY_WAIT);
+		outb(cmd, port);
+	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* write: wait for smc to settle */
+		if (status & 0x02)
+			continue;
+		/* ready: cmd accepted, return */
+		if (status & 0x04)
+			return 0;
+		/* timeout: give up */
 		if (us << 1 == APPLESMC_MAX_WAIT)
 			break;
 		/* busy: long wait and resend */
-- 
2.23.0.351.gc4317032e6-goog

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Nathan Chancellor]

On Tue, Sep 24, 2019 at 10:47:28AM -0700, Nick Desaulniers wrote:
> Fixes the following 2 issues in the driver:
> 1. Left shifting a signed integer is undefined behavior. Unsigned
>    integral types should be used for bitwise operations.
> 2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
>    will result in a linkage failure when given a constant that's greater
>    than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
>    loop, resulting in later iterations overflowing the call to udelay.
> 
> 2 is fixed via splitting the loop in two, iterating the first up to the
> point where udelay would overflow, then switching to mdelay, as
> suggested in Documentation/timers/timers-howto.rst.
> 
> Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/678
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V1 -> V2:
> * The first loop in send_byte() needs to break out on the same condition
>   now. Technically, the loop condition could even be removed. The diff
>   looks funny because of the duplicated logic between existing and newly
>   added for loops.
> 
>  drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
>  1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 183ff3d25129..c76adb504dff 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -46,6 +46,7 @@
>  #define APPLESMC_MIN_WAIT	0x0010
>  #define APPLESMC_RETRY_WAIT	0x0100
>  #define APPLESMC_MAX_WAIT	0x20000
> +#define APPLESMC_UDELAY_MAX	20000
>  
>  #define APPLESMC_READ_CMD	0x10
>  #define APPLESMC_WRITE_CMD	0x11
> @@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
>  static int wait_read(void)
>  {
>  	u8 status;
> -	int us;
> -	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +	unsigned int us;
> +
> +	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>  		udelay(us);
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* read: wait for smc to settle */
>  		if (status & 0x01)
>  			return 0;
>  	}
> +	/* switch to mdelay for longer sleeps */
> +	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +		mdelay(us);
> +		status = inb(APPLESMC_CMD_PORT);
> +		/* read: wait for smc to settle */
> +		if (status & 0x01)
> +			return 0;
> +	}
>  
>  	pr_warn("wait_read() fail: 0x%02x\n", status);
>  	return -EIO;
> @@ -177,10 +187,10 @@ static int wait_read(void)
>  static int send_byte(u8 cmd, u16 port)
>  {
>  	u8 status;
> -	int us;
> +	unsigned int us;
>  
>  	outb(cmd, port);
> -	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>  		udelay(us);
>  		status = inb(APPLESMC_CMD_PORT);
>  		/* write: wait for smc to settle */
> @@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
>  		if (status & 0x04)
>  			return 0;
>  		/* timeout: give up */
> +		if (us << 1 == APPLESMC_UDELAY_MAX)
> +			break;
> +		/* busy: long wait and resend */
> +		udelay(APPLESMC_RETRY_WAIT);
> +		outb(cmd, port);
> +	}
> +	/* switch to mdelay for longer sleeps */
> +	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +		mdelay(us);
> +		status = inb(APPLESMC_CMD_PORT);
> +		/* write: wait for smc to settle */
> +		if (status & 0x02)
> +			continue;
> +		/* ready: cmd accepted, return */
> +		if (status & 0x04)
> +			return 0;
> +		/* timeout: give up */
>  		if (us << 1 == APPLESMC_MAX_WAIT)
>  			break;
>  		/* busy: long wait and resend */
> -- 
> 2.23.0.351.gc4317032e6-goog
> 

This resolves the __bad_udelay appearance at -O3 for me. I am not
familiar enough with this code to give a reviewed by though!

Also, for some odd reason, I couldn't apply your patch with 'git apply':

% curl -LSs https://lore.kernel.org/lkml/20190924174728.201464-1-ndesaulniers@google.com/raw | git apply
error: corrupt patch at line 117

It looks like some of the '=' got changed into =3D and some spaces got
changed into =20. Weird encoding glitch?

Cheers,
Nathan

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Nick Desaulniers]

[-- Attachment #1: Type: text/plain, Size: 5281 bytes --]

On Tue, Sep 24, 2019 at 11:38 AM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Sep 24, 2019 at 10:47:28AM -0700, Nick Desaulniers wrote:
> > Fixes the following 2 issues in the driver:
> > 1. Left shifting a signed integer is undefined behavior. Unsigned
> >    integral types should be used for bitwise operations.
> > 2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
> >    will result in a linkage failure when given a constant that's greater
> >    than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
> >    loop, resulting in later iterations overflowing the call to udelay.
> >
> > 2 is fixed via splitting the loop in two, iterating the first up to the
> > point where udelay would overflow, then switching to mdelay, as
> > suggested in Documentation/timers/timers-howto.rst.
> >
> > Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> > Link: https://github.com/ClangBuiltLinux/linux/issues/678
> > Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > ---
> > Changes V1 -> V2:
> > * The first loop in send_byte() needs to break out on the same condition
> >   now. Technically, the loop condition could even be removed. The diff
> >   looks funny because of the duplicated logic between existing and newly
> >   added for loops.
> >
> >  drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
> >  1 file changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > index 183ff3d25129..c76adb504dff 100644
> > --- a/drivers/hwmon/applesmc.c
> > +++ b/drivers/hwmon/applesmc.c
> > @@ -46,6 +46,7 @@
> >  #define APPLESMC_MIN_WAIT    0x0010
> >  #define APPLESMC_RETRY_WAIT  0x0100
> >  #define APPLESMC_MAX_WAIT    0x20000
> > +#define APPLESMC_UDELAY_MAX  20000
> >
> >  #define APPLESMC_READ_CMD    0x10
> >  #define APPLESMC_WRITE_CMD   0x11
> > @@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
> >  static int wait_read(void)
> >  {
> >       u8 status;
> > -     int us;
> > -     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > +     unsigned int us;
> > +
> > +     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
> >               udelay(us);
> >               status = inb(APPLESMC_CMD_PORT);
> >               /* read: wait for smc to settle */
> >               if (status & 0x01)
> >                       return 0;
> >       }
> > +     /* switch to mdelay for longer sleeps */
> > +     for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > +             mdelay(us);
> > +             status = inb(APPLESMC_CMD_PORT);
> > +             /* read: wait for smc to settle */
> > +             if (status & 0x01)
> > +                     return 0;
> > +     }
> >
> >       pr_warn("wait_read() fail: 0x%02x\n", status);
> >       return -EIO;
> > @@ -177,10 +187,10 @@ static int wait_read(void)
> >  static int send_byte(u8 cmd, u16 port)
> >  {
> >       u8 status;
> > -     int us;
> > +     unsigned int us;
> >
> >       outb(cmd, port);
> > -     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > +     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
> >               udelay(us);
> >               status = inb(APPLESMC_CMD_PORT);
> >               /* write: wait for smc to settle */
> > @@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
> >               if (status & 0x04)
> >                       return 0;
> >               /* timeout: give up */
> > +             if (us << 1 == APPLESMC_UDELAY_MAX)
> > +                     break;
> > +             /* busy: long wait and resend */
> > +             udelay(APPLESMC_RETRY_WAIT);
> > +             outb(cmd, port);
> > +     }
> > +     /* switch to mdelay for longer sleeps */
> > +     for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > +             mdelay(us);
> > +             status = inb(APPLESMC_CMD_PORT);
> > +             /* write: wait for smc to settle */
> > +             if (status & 0x02)
> > +                     continue;
> > +             /* ready: cmd accepted, return */
> > +             if (status & 0x04)
> > +                     return 0;
> > +             /* timeout: give up */
> >               if (us << 1 == APPLESMC_MAX_WAIT)
> >                       break;
> >               /* busy: long wait and resend */
> > --
> > 2.23.0.351.gc4317032e6-goog
> >
>
> This resolves the __bad_udelay appearance at -O3 for me. I am not
> familiar enough with this code to give a reviewed by though!

Does that constitute a Tested-by tag?

>
> Also, for some odd reason, I couldn't apply your patch with 'git apply':
>
> % curl -LSs https://lore.kernel.org/lkml/20190924174728.201464-1-ndesaulniers@google.com/raw | git apply
> error: corrupt patch at line 117
>
> It looks like some of the '=' got changed into =3D and some spaces got
> changed into =20. Weird encoding glitch?

The text in my email client shows no encoding error; the link above
shows the issue.  Attaching a copy here, in case git-send-email
related.
-- 
Thanks,
~Nick Desaulniers

[-- Attachment #2: v2-0001-hwmon-applesmc-fix-UB-and-udelay-overflow.patch --]
[-- Type: text/x-patch, Size: 3718 bytes --]

From e112935cb4361aa820b89588e5764bfc3dfd7f9f Mon Sep 17 00:00:00 2001
From: Nick Desaulniers <ndesaulniers@google.com>
Date: Tue, 24 Sep 2019 10:26:34 -0700
Subject: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixes the following 2 issues in the driver:
1. Left shifting a signed integer is undefined behavior. Unsigned
   integral types should be used for bitwise operations.
2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
   will result in a linkage failure when given a constant that's greater
   than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
   loop, resulting in later iterations overflowing the call to udelay.

2 is fixed via splitting the loop in two, iterating the first up to the
point where udelay would overflow, then switching to mdelay, as
suggested in Documentation/timers/timers-howto.rst.

Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
Link: https://github.com/ClangBuiltLinux/linux/issues/678
Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
---
Changes V1 -> V2:
* The first loop in send_byte() needs to break out on the same condition
  now. Technically, the loop condition could even be removed. The diff
  looks funny because of the duplicated logic between existing and newly
  added for loops.

 drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
 1 file changed, 31 insertions(+), 4 deletions(-)

diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
index 183ff3d25129..c76adb504dff 100644
--- a/drivers/hwmon/applesmc.c
+++ b/drivers/hwmon/applesmc.c
@@ -46,6 +46,7 @@
 #define APPLESMC_MIN_WAIT	0x0010
 #define APPLESMC_RETRY_WAIT	0x0100
 #define APPLESMC_MAX_WAIT	0x20000
+#define APPLESMC_UDELAY_MAX	20000
 
 #define APPLESMC_READ_CMD	0x10
 #define APPLESMC_WRITE_CMD	0x11
@@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
 static int wait_read(void)
 {
 	u8 status;
-	int us;
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	unsigned int us;
+
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* read: wait for smc to settle */
 		if (status & 0x01)
 			return 0;
 	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* read: wait for smc to settle */
+		if (status & 0x01)
+			return 0;
+	}
 
 	pr_warn("wait_read() fail: 0x%02x\n", status);
 	return -EIO;
@@ -177,10 +187,10 @@ static int wait_read(void)
 static int send_byte(u8 cmd, u16 port)
 {
 	u8 status;
-	int us;
+	unsigned int us;
 
 	outb(cmd, port);
-	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
+	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
 		udelay(us);
 		status = inb(APPLESMC_CMD_PORT);
 		/* write: wait for smc to settle */
@@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
 		if (status & 0x04)
 			return 0;
 		/* timeout: give up */
+		if (us << 1 == APPLESMC_UDELAY_MAX)
+			break;
+		/* busy: long wait and resend */
+		udelay(APPLESMC_RETRY_WAIT);
+		outb(cmd, port);
+	}
+	/* switch to mdelay for longer sleeps */
+	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
+		mdelay(us);
+		status = inb(APPLESMC_CMD_PORT);
+		/* write: wait for smc to settle */
+		if (status & 0x02)
+			continue;
+		/* ready: cmd accepted, return */
+		if (status & 0x04)
+			return 0;
+		/* timeout: give up */
 		if (us << 1 == APPLESMC_MAX_WAIT)
 			break;
 		/* busy: long wait and resend */
-- 
2.23.0.351.gc4317032e6-goog

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Nathan Chancellor]

On Tue, Sep 24, 2019 at 12:36:44PM -0700, Nick Desaulniers wrote:
> On Tue, Sep 24, 2019 at 11:38 AM Nathan Chancellor
> <natechancellor@gmail.com> wrote:
> >
> > On Tue, Sep 24, 2019 at 10:47:28AM -0700, Nick Desaulniers wrote:
> > > Fixes the following 2 issues in the driver:
> > > 1. Left shifting a signed integer is undefined behavior. Unsigned
> > >    integral types should be used for bitwise operations.
> > > 2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
> > >    will result in a linkage failure when given a constant that's greater
> > >    than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
> > >    loop, resulting in later iterations overflowing the call to udelay.
> > >
> > > 2 is fixed via splitting the loop in two, iterating the first up to the
> > > point where udelay would overflow, then switching to mdelay, as
> > > suggested in Documentation/timers/timers-howto.rst.
> > >
> > > Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> > > Link: https://github.com/ClangBuiltLinux/linux/issues/678
> > > Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> > > Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> > > ---
> > > Changes V1 -> V2:
> > > * The first loop in send_byte() needs to break out on the same condition
> > >   now. Technically, the loop condition could even be removed. The diff
> > >   looks funny because of the duplicated logic between existing and newly
> > >   added for loops.
> > >
> > >  drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
> > >  1 file changed, 31 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> > > index 183ff3d25129..c76adb504dff 100644
> > > --- a/drivers/hwmon/applesmc.c
> > > +++ b/drivers/hwmon/applesmc.c
> > > @@ -46,6 +46,7 @@
> > >  #define APPLESMC_MIN_WAIT    0x0010
> > >  #define APPLESMC_RETRY_WAIT  0x0100
> > >  #define APPLESMC_MAX_WAIT    0x20000
> > > +#define APPLESMC_UDELAY_MAX  20000
> > >
> > >  #define APPLESMC_READ_CMD    0x10
> > >  #define APPLESMC_WRITE_CMD   0x11
> > > @@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
> > >  static int wait_read(void)
> > >  {
> > >       u8 status;
> > > -     int us;
> > > -     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > > +     unsigned int us;
> > > +
> > > +     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
> > >               udelay(us);
> > >               status = inb(APPLESMC_CMD_PORT);
> > >               /* read: wait for smc to settle */
> > >               if (status & 0x01)
> > >                       return 0;
> > >       }
> > > +     /* switch to mdelay for longer sleeps */
> > > +     for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > > +             mdelay(us);
> > > +             status = inb(APPLESMC_CMD_PORT);
> > > +             /* read: wait for smc to settle */
> > > +             if (status & 0x01)
> > > +                     return 0;
> > > +     }
> > >
> > >       pr_warn("wait_read() fail: 0x%02x\n", status);
> > >       return -EIO;
> > > @@ -177,10 +187,10 @@ static int wait_read(void)
> > >  static int send_byte(u8 cmd, u16 port)
> > >  {
> > >       u8 status;
> > > -     int us;
> > > +     unsigned int us;
> > >
> > >       outb(cmd, port);
> > > -     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > > +     for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
> > >               udelay(us);
> > >               status = inb(APPLESMC_CMD_PORT);
> > >               /* write: wait for smc to settle */
> > > @@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
> > >               if (status & 0x04)
> > >                       return 0;
> > >               /* timeout: give up */
> > > +             if (us << 1 == APPLESMC_UDELAY_MAX)
> > > +                     break;
> > > +             /* busy: long wait and resend */
> > > +             udelay(APPLESMC_RETRY_WAIT);
> > > +             outb(cmd, port);
> > > +     }
> > > +     /* switch to mdelay for longer sleeps */
> > > +     for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> > > +             mdelay(us);
> > > +             status = inb(APPLESMC_CMD_PORT);
> > > +             /* write: wait for smc to settle */
> > > +             if (status & 0x02)
> > > +                     continue;
> > > +             /* ready: cmd accepted, return */
> > > +             if (status & 0x04)
> > > +                     return 0;
> > > +             /* timeout: give up */
> > >               if (us << 1 == APPLESMC_MAX_WAIT)
> > >                       break;
> > >               /* busy: long wait and resend */
> > > --
> > > 2.23.0.351.gc4317032e6-goog
> > >
> >
> > This resolves the __bad_udelay appearance at -O3 for me. I am not
> > familiar enough with this code to give a reviewed by though!
> 
> Does that constitute a Tested-by tag?

Given that this could have real implications for the hardware, I would
think tested by would imply running this on said hardware. FWIW:

Build-tested-by: Nathan Chancellor <natechancellor@gmail.com>

> >
> > Also, for some odd reason, I couldn't apply your patch with 'git apply':
> >
> > % curl -LSs https://lore.kernel.org/lkml/20190924174728.201464-1-ndesaulniers@google.com/raw | git apply
> > error: corrupt patch at line 117
> >
> > It looks like some of the '=' got changed into =3D and some spaces got
> > changed into =20. Weird encoding glitch?
> 
> The text in my email client shows no encoding error; the link above
> shows the issue.  Attaching a copy here, in case git-send-email
> related.

Yes, appears git send-email related because your attached patch applies
just fine.

Cheers,
Nathan

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Cengiz Can]

I spent quite some time to rewrite applesmc but I couldn't finish.

Thanks for the loop fix. I did my reviewing part, if that matters.

> Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/678
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> Build-tested-by: Nathan Chancellor <natechancellor@gmail.com>

Reviewed-by: Cengiz Can <cengiz@kernel.wtf>

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Guenter Roeck]

On 9/24/19 10:47 AM, Nick Desaulniers wrote:
> Fixes the following 2 issues in the driver:
> 1. Left shifting a signed integer is undefined behavior. Unsigned
>     integral types should be used for bitwise operations.
> 2. The delay scales from 0x0010 to 0x20000 by powers of 2, but udelay
>     will result in a linkage failure when given a constant that's greater
>     than 20000 (0x4E20). Agressive loop unrolling can fully unroll the
>     loop, resulting in later iterations overflowing the call to udelay.
> 
> 2 is fixed via splitting the loop in two, iterating the first up to the
> point where udelay would overflow, then switching to mdelay, as
> suggested in Documentation/timers/timers-howto.rst.
> 
> Reported-by: Tomasz Paweł Gajc <tpgxyz@gmail.com>
> Link: https://github.com/ClangBuiltLinux/linux/issues/678
> Debugged-by: Nathan Chancellor <natechancellor@gmail.com>
> Signed-off-by: Nick Desaulniers <ndesaulniers@google.com>
> ---
> Changes V1 -> V2:
> * The first loop in send_byte() needs to break out on the same condition
>    now. Technically, the loop condition could even be removed. The diff
>    looks funny because of the duplicated logic between existing and newly
>    added for loops.
> 

That is a delay()-internal dependency, and completely undocumented. This code
will fall apart if the implementation of udelay() is ever changed. This
also depends on the architecture - in some cases, mdelay() is implemented
as udelay(n * 1000).

>   drivers/hwmon/applesmc.c | 35 +++++++++++++++++++++++++++++++----
>   1 file changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hwmon/applesmc.c b/drivers/hwmon/applesmc.c
> index 183ff3d25129..c76adb504dff 100644
> --- a/drivers/hwmon/applesmc.c
> +++ b/drivers/hwmon/applesmc.c
> @@ -46,6 +46,7 @@
>   #define APPLESMC_MIN_WAIT	0x0010
>   #define APPLESMC_RETRY_WAIT	0x0100
>   #define APPLESMC_MAX_WAIT	0x20000
> +#define APPLESMC_UDELAY_MAX	20000
>   

This is not really a problem in this driver; it is a system problem.
Anyone can call udelay() with a parameter longer than 20,000 us.
We can't add code like this all over the place because the implementation
of delay() is broken.

Besides, calling delay() with a parameter of 20,000 or more is a strong
indication that something is really wrong with the code. More on that
see below.

>   #define APPLESMC_READ_CMD	0x10
>   #define APPLESMC_WRITE_CMD	0x11
> @@ -157,14 +158,23 @@ static struct workqueue_struct *applesmc_led_wq;
>   static int wait_read(void)
>   {
>   	u8 status;
> -	int us;
> -	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +	unsigned int us;
> +
> +	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>   		udelay(us);

>   		status = inb(APPLESMC_CMD_PORT);
>   		/* read: wait for smc to settle */
>   		if (status & 0x01)
>   			return 0;
>   	}
> +	/* switch to mdelay for longer sleeps */
> +	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +		mdelay(us);

Shouldn't that be us / 1000 ? Seems to me the above will wait for
at least 20000 ms, which is a a tiny bit long.

Also, mdelay(n) is by default implemented as udelay(n * 1000).

Also, at the very least, this should be something like

		if (us < limit)
			delay(us);
		else
			mdelay(us / 1000);

instead of introducing a second loop. But more on that below.

> +		status = inb(APPLESMC_CMD_PORT);
> +		/* read: wait for smc to settle */
> +		if (status & 0x01)
> +			return 0;
> +	}
>   
>   	pr_warn("wait_read() fail: 0x%02x\n", status);
>   	return -EIO;
> @@ -177,10 +187,10 @@ static int wait_read(void)
>   static int send_byte(u8 cmd, u16 port)
>   {
>   	u8 status;
> -	int us;
> +	unsigned int us;
>   
>   	outb(cmd, port);
> -	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +	for (us = APPLESMC_MIN_WAIT; us < APPLESMC_UDELAY_MAX; us <<= 1) {
>   		udelay(us);
>   		status = inb(APPLESMC_CMD_PORT);
>   		/* write: wait for smc to settle */
> @@ -190,6 +200,23 @@ static int send_byte(u8 cmd, u16 port)
>   		if (status & 0x04)
>   			return 0;
>   		/* timeout: give up */
> +		if (us << 1 == APPLESMC_UDELAY_MAX)
> +			break;
> +		/* busy: long wait and resend */
> +		udelay(APPLESMC_RETRY_WAIT);
> +		outb(cmd, port);
> +	}
> +	/* switch to mdelay for longer sleeps */
> +	for (; us < APPLESMC_MAX_WAIT; us <<= 1) {
> +		mdelay(us);

Again, I fail to understand why waiting for a multiple of 20 seconds
under any circumstances would make any sense. Maybe the idea was
to divide us by 1000 before entering the second loop ?

Looking into the code, there is no need to use udelay() in the first
place. It should be possible to replace the longer waits with
usleep_range(). Something like

		if (us < some_low_value)	// eg. 0x80
			delay(us)
		else
			usleep_range(us, us * 2);

should do, and at the same time prevent the system from turning
into a space heater.

Thanks,
Guenter

> +		status = inb(APPLESMC_CMD_PORT);
> +		/* write: wait for smc to settle */
> +		if (status & 0x02)
> +			continue;
> +		/* ready: cmd accepted, return */
> +		if (status & 0x04)
> +			return 0;
> +		/* timeout: give up */
>   		if (us << 1 == APPLESMC_MAX_WAIT)
>   			break;
>   		/* busy: long wait and resend */
> 

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Nick Desaulniers]

On Mon, Sep 30, 2019 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> Again, I fail to understand why waiting for a multiple of 20 seconds
> under any circumstances would make any sense. Maybe the idea was
> to divide us by 1000 before entering the second loop ?

Yes, that's very clearly a mistake of mine.

>
> Looking into the code, there is no need to use udelay() in the first
> place. It should be possible to replace the longer waits with
> usleep_range(). Something like
>
>                 if (us < some_low_value)        // eg. 0x80
>                         delay(us)

Did you mean udelay here?

>                 else
>                         usleep_range(us, us * 2);
>
> should do, and at the same time prevent the system from turning
> into a space heater.

The issue would persist with the above if udelay remains in a loop
that gets fully unrolled.  That's while I "peel" the loop into two
loops over different ranges with different bodies.

I think I should iterate in the first loop until the number of `us` is
greater than 1000 (us per ms)(which is less of a magical constant and
doesn't expose internal implementation details of udelay), then start
the second loop (dividing us by 1000).  What do you think, Guenter?

--
Thanks,
~Nick Desaulniers

Re: [PATCH v2] hwmon: (applesmc) fix UB and udelay overflow

[Guenter Roeck]

On 10/2/19 2:43 PM, Nick Desaulniers wrote:
> On Mon, Sep 30, 2019 at 5:01 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> Again, I fail to understand why waiting for a multiple of 20 seconds
>> under any circumstances would make any sense. Maybe the idea was
>> to divide us by 1000 before entering the second loop ?
> 
> Yes, that's very clearly a mistake of mine.
> 
>>
>> Looking into the code, there is no need to use udelay() in the first
>> place. It should be possible to replace the longer waits with
>> usleep_range(). Something like
>>
>>                  if (us < some_low_value)        // eg. 0x80
>>                          delay(us)
> 
> Did you mean udelay here?
> 
Yes

>>                  else
>>                          usleep_range(us, us * 2);
>>
>> should do, and at the same time prevent the system from turning
>> into a space heater.
> 
> The issue would persist with the above if udelay remains in a loop
> that gets fully unrolled.  That's while I "peel" the loop into two
> loops over different ranges with different bodies.
> 

Sorry, you lost me. If calls to udelay() with even small delay
parameters for some compiler-related reason no longer work, trying
to fix the problem with some odd driver code is most definitely not
a real solution.

> I think I should iterate in the first loop until the number of `us` is
> greater than 1000 (us per ms)(which is less of a magical constant and
> doesn't expose internal implementation details of udelay), then start
> the second loop (dividing us by 1000).  What do you think, Guenter?
> 

We should have no second loop, period.

Again, a hot delay loop of 128 ms (actually, more like 245 ms,
adding all delays together) is clearly wrong. Those udelay() calls
in the driver should really be replaced with usleep_range().

Guenter