[BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[Li Zhong]

Hello,

My static analysis tool reports a possible bug in the adm9240 driver in Linux
v6.0:

drivers/hwmon/adm9240.c:

adm9240_read()
    adm9240_fan_read() --> Line 509
        adm9240_write_fan_div()

adm9240_write_fan_div() says 'callers must hold
data->update_lock'. However, it seems like the context does
not hold the lock it requires. So it may cause data race when
setting new fan div.

I am not quite sure whether this is a real bug. Any feedback would be
appreciated!

Thanks,
Li Zhong

Re: [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[Guenter Roeck]

On 9/21/22 16:31, Li Zhong wrote:
> Hello,
> 
> My static analysis tool reports a possible bug in the adm9240 driver in Linux
> v6.0:
> 
> drivers/hwmon/adm9240.c:
> 
> adm9240_read()
>      adm9240_fan_read() --> Line 509
>          adm9240_write_fan_div()
> 
> adm9240_write_fan_div() says 'callers must hold
> data->update_lock'. However, it seems like the context does
> not hold the lock it requires. So it may cause data race when
> setting new fan div.
> 
> I am not quite sure whether this is a real bug. Any feedback would be
> appreciated!
> 

You are correct, the code in adm9240_fan_read() should acquire
the mutex before calling adm9240_write_fan_div() and while
manipulating data->fan_div[channel].

Guenter

Re: [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[Li Zhong]

On Wed, Sep 21, 2022 at 8:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/21/22 16:31, Li Zhong wrote:
> > Hello,
> >
> > My static analysis tool reports a possible bug in the adm9240 driver in Linux
> > v6.0:
> >
> > drivers/hwmon/adm9240.c:
> >
> > adm9240_read()
> >      adm9240_fan_read() --> Line 509
> >          adm9240_write_fan_div()
> >
> > adm9240_write_fan_div() says 'callers must hold
> > data->update_lock'. However, it seems like the context does
> > not hold the lock it requires. So it may cause data race when
> > setting new fan div.
> >
> > I am not quite sure whether this is a real bug. Any feedback would be
> > appreciated!
> >
>
> You are correct, the code in adm9240_fan_read() should acquire
> the mutex before calling adm9240_write_fan_div() and while
> manipulating data->fan_div[channel].
>
> Guenter

Thanks for your patient reply! Can I submit a patch on this? The draft will
be something like:

+  mutex_lock(&data->update_lock)
    err = adm9240_write_fan_div(data, channel, ++data->fan_div[channel]);
    if (err)
        return err;
+  mutex_unlock(&data->update_lock);

Let me know if you have any suggestions!

Re: [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[Guenter Roeck]

On 9/22/22 13:37, Li Zhong wrote:
> On Wed, Sep 21, 2022 at 8:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>
>> On 9/21/22 16:31, Li Zhong wrote:
>>> Hello,
>>>
>>> My static analysis tool reports a possible bug in the adm9240 driver in Linux
>>> v6.0:
>>>
>>> drivers/hwmon/adm9240.c:
>>>
>>> adm9240_read()
>>>       adm9240_fan_read() --> Line 509
>>>           adm9240_write_fan_div()
>>>
>>> adm9240_write_fan_div() says 'callers must hold
>>> data->update_lock'. However, it seems like the context does
>>> not hold the lock it requires. So it may cause data race when
>>> setting new fan div.
>>>
>>> I am not quite sure whether this is a real bug. Any feedback would be
>>> appreciated!
>>>
>>
>> You are correct, the code in adm9240_fan_read() should acquire
>> the mutex before calling adm9240_write_fan_div() and while
>> manipulating data->fan_div[channel].
>>
>> Guenter
> 
> Thanks for your patient reply! Can I submit a patch on this? The draft will
> be something like:
> 
> +  mutex_lock(&data->update_lock)
>      err = adm9240_write_fan_div(data, channel, ++data->fan_div[channel]);
>      if (err)
>          return err;
> +  mutex_unlock(&data->update_lock);
> 
> Let me know if you have any suggestions!

That would leave the mutex in locked state after an error, and it does not
take into account that data->fan_div[channel] might still change after being
checked but before being used. The lock has to be around the if() statement,
and the lock must be released after an error was observed.

At the very least, the code has to be something like

	...
	mutex_lock(&data->update_lock);
         if (regval == 255 && data->fan_div[channel] < 3) {
                 /* adjust fan clock divider on overflow */
                 err = adm9240_write_fan_div(data, channel,
                                             ++data->fan_div[channel]);
                 if (err) {
			mutex_unlock(&data->update_lock);
                         return err;
		}
         }
	mutex_unlock(&data->update_lock);
	...

However, that isn't perfect since the fan divisor and the fan speed
register value are not in sync. Technically it needs to be something like

	u8 fan_div;
	...

	mutex_lock(&data->update_lock);
	err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), &regval);
	if (err) {
		mutex_unlock(&data->update_lock);
		return err;
	}
	fan_div = data->fan_div[channel];
	if (regval == 255 && fan_div < 3) {
		err = adm9240_write_fan_div(data, channel, fan_div + 1);
		if (err) {
			mutex_unlock(&data->update_lock);
			return err;
		}
		data->fan_div[channel] = fan_div + 1;
	}
	mutex_unlock(&data->update_lock);
	*val = FAN_FROM_REG(regval, BIT(fan_div));
	break;

Thanks,
Guenter

Re: [BUG] drivers: adm9240: possible data race bug in adm9240_fan_read()

[Li Zhong]

On Thu, Sep 22, 2022 at 2:53 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 9/22/22 13:37, Li Zhong wrote:
> > On Wed, Sep 21, 2022 at 8:16 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >>
> >> On 9/21/22 16:31, Li Zhong wrote:
> >>> Hello,
> >>>
> >>> My static analysis tool reports a possible bug in the adm9240 driver in Linux
> >>> v6.0:
> >>>
> >>> drivers/hwmon/adm9240.c:
> >>>
> >>> adm9240_read()
> >>>       adm9240_fan_read() --> Line 509
> >>>           adm9240_write_fan_div()
> >>>
> >>> adm9240_write_fan_div() says 'callers must hold
> >>> data->update_lock'. However, it seems like the context does
> >>> not hold the lock it requires. So it may cause data race when
> >>> setting new fan div.
> >>>
> >>> I am not quite sure whether this is a real bug. Any feedback would be
> >>> appreciated!
> >>>
> >>
> >> You are correct, the code in adm9240_fan_read() should acquire
> >> the mutex before calling adm9240_write_fan_div() and while
> >> manipulating data->fan_div[channel].
> >>
> >> Guenter
> >
> > Thanks for your patient reply! Can I submit a patch on this? The draft will
> > be something like:
> >
> > +  mutex_lock(&data->update_lock)
> >      err = adm9240_write_fan_div(data, channel, ++data->fan_div[channel]);
> >      if (err)
> >          return err;
> > +  mutex_unlock(&data->update_lock);
> >
> > Let me know if you have any suggestions!
>
> That would leave the mutex in locked state after an error, and it does not
> take into account that data->fan_div[channel] might still change after being
> checked but before being used. The lock has to be around the if() statement,
> and the lock must be released after an error was observed.
>
> At the very least, the code has to be something like
>
>         ...
>         mutex_lock(&data->update_lock);
>          if (regval == 255 && data->fan_div[channel] < 3) {
>                  /* adjust fan clock divider on overflow */
>                  err = adm9240_write_fan_div(data, channel,
>                                              ++data->fan_div[channel]);
>                  if (err) {
>                         mutex_unlock(&data->update_lock);
>                          return err;
>                 }
>          }
>         mutex_unlock(&data->update_lock);
>         ...
>
> However, that isn't perfect since the fan divisor and the fan speed
> register value are not in sync. Technically it needs to be something like
>
>         u8 fan_div;
>         ...
>
>         mutex_lock(&data->update_lock);
>         err = regmap_read(data->regmap, ADM9240_REG_FAN(channel), &regval);
>         if (err) {
>                 mutex_unlock(&data->update_lock);
>                 return err;
>         }
>         fan_div = data->fan_div[channel];
>         if (regval == 255 && fan_div < 3) {
>                 err = adm9240_write_fan_div(data, channel, fan_div + 1);
>                 if (err) {
>                         mutex_unlock(&data->update_lock);
>                         return err;
>                 }
>                 data->fan_div[channel] = fan_div + 1;
>         }
>         mutex_unlock(&data->update_lock);
>         *val = FAN_FROM_REG(regval, BIT(fan_div));
>         break;
>
> Thanks,
> Guenter

Thanks for your suggestions and drafts! I submit the patch.