| Message ID | 20190924173717.198637-1-ndesaulniers@google.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (applesmc) fix UB and udelay overflow | expand |
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
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;
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(-) -- 2.23.0.351.gc4317032e6-goog