| Message ID | 20220121053917.GB27293@kili (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (adt7470) Prevent divide by zero in adt7470_fan_write() | expand |
On 1/20/22 9:39 PM, Dan Carpenter wrote: > The "val" variable is controlled by the user and comes from > hwmon_attr_store(). The FAN_RPM_TO_PERIOD() macro divides by "val" > so a zero will crash the system. Check for that and return -EINVAL. > > Fixes: fc958a61ff6d ("hwmon: (adt7470) Convert to devm_hwmon_device_register_with_info API") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > drivers/hwmon/adt7470.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c > index d519aca4a9d6..cd474584dc0b 100644 > --- a/drivers/hwmon/adt7470.c > +++ b/drivers/hwmon/adt7470.c > @@ -662,6 +662,9 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val > struct adt7470_data *data = dev_get_drvdata(dev); > int err; > > + if (!val) > + return -EINVAL; > + Technically that restores old (pre-fc958a61ff6d) behavior, but it is still bad: Userspace can provide a value of -1 (or any other negative number), and it will translate to 5400000 RPM. So it should either be if (val <= 0) return -EINVAL; or if (val <= 0) val = 1; Thanks, Guenter
On Thu, Jan 20, 2022 at 10:10:45PM -0800, Guenter Roeck wrote: > On 1/20/22 9:39 PM, Dan Carpenter wrote: > > The "val" variable is controlled by the user and comes from > > hwmon_attr_store(). The FAN_RPM_TO_PERIOD() macro divides by "val" > > so a zero will crash the system. Check for that and return -EINVAL. > > > > Fixes: fc958a61ff6d ("hwmon: (adt7470) Convert to devm_hwmon_device_register_with_info API") > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > --- > > drivers/hwmon/adt7470.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c > > index d519aca4a9d6..cd474584dc0b 100644 > > --- a/drivers/hwmon/adt7470.c > > +++ b/drivers/hwmon/adt7470.c > > @@ -662,6 +662,9 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val > > struct adt7470_data *data = dev_get_drvdata(dev); > > int err; > > + if (!val) > > + return -EINVAL; > > + > > Technically that restores old (pre-fc958a61ff6d) behavior, but it is still bad: > Userspace can provide a value of -1 (or any other negative number), and it will > translate to 5400000 RPM. So it should either be > > if (val <= 0) > return -EINVAL; > > or > if (val <= 0) > val = 1; There is a clamp() which does already turn invalid values into something valid. val = FAN_RPM_TO_PERIOD(val); val = clamp_val(val, 1, 65534); But I will make the <= 0 return -EINVAL change and resend. regards, dan carpenter
On 1/20/22 11:37 PM, Dan Carpenter wrote: > On Thu, Jan 20, 2022 at 10:10:45PM -0800, Guenter Roeck wrote: >> On 1/20/22 9:39 PM, Dan Carpenter wrote: >>> The "val" variable is controlled by the user and comes from >>> hwmon_attr_store(). The FAN_RPM_TO_PERIOD() macro divides by "val" >>> so a zero will crash the system. Check for that and return -EINVAL. >>> >>> Fixes: fc958a61ff6d ("hwmon: (adt7470) Convert to devm_hwmon_device_register_with_info API") >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >>> --- >>> drivers/hwmon/adt7470.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c >>> index d519aca4a9d6..cd474584dc0b 100644 >>> --- a/drivers/hwmon/adt7470.c >>> +++ b/drivers/hwmon/adt7470.c >>> @@ -662,6 +662,9 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val >>> struct adt7470_data *data = dev_get_drvdata(dev); >>> int err; >>> + if (!val) >>> + return -EINVAL; >>> + >> >> Technically that restores old (pre-fc958a61ff6d) behavior, but it is still bad: >> Userspace can provide a value of -1 (or any other negative number), and it will >> translate to 5400000 RPM. So it should either be >> >> if (val <= 0) >> return -EINVAL; >> >> or >> if (val <= 0) >> val = 1; > > There is a clamp() which does already turn invalid values into something > valid. > Yes, but -1 -> -5400000 -> 1, which translates to 5400000 rpm. This is in contrast to 1 -> 5400000 -> 65534 which translates to a more reasonable 82 rpm. > val = FAN_RPM_TO_PERIOD(val); > val = clamp_val(val, 1, 65534); > > But I will make the <= 0 return -EINVAL change and resend. > Thanks, Guenter
diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c index d519aca4a9d6..cd474584dc0b 100644 --- a/drivers/hwmon/adt7470.c +++ b/drivers/hwmon/adt7470.c @@ -662,6 +662,9 @@ static int adt7470_fan_write(struct device *dev, u32 attr, int channel, long val struct adt7470_data *data = dev_get_drvdata(dev); int err; + if (!val) + return -EINVAL; + val = FAN_RPM_TO_PERIOD(val); val = clamp_val(val, 1, 65534);
The "val" variable is controlled by the user and comes from hwmon_attr_store(). The FAN_RPM_TO_PERIOD() macro divides by "val" so a zero will crash the system. Check for that and return -EINVAL. Fixes: fc958a61ff6d ("hwmon: (adt7470) Convert to devm_hwmon_device_register_with_info API") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- drivers/hwmon/adt7470.c | 3 +++ 1 file changed, 3 insertions(+)