Re: [PATCH v8 6/7] platform/x86/dell-laptop: Protect kbd_state against races

[Hans de Goede <hdegoede@xxxxxxxxxx>]

Hi,

On 21-02-17 15:25, Pali Rohár wrote:
> On Tuesday 21 February 2017 15:18:14 Hans de Goede wrote:
> > Hi,
> >
> > On 21-02-17 15:06, Pali Rohár wrote:
> > > On Thursday 09 February 2017 16:44:16 Hans de Goede wrote:
> > > > The kbd led code has multiple entry points each of which modifies the
> > > > kbd_state by reading it, modifying a copy, writing the copy and on
> > > > error setting the modified copy writing back the original state.
> > > >
> > > > This is racy, so add a mutex protection the read-modify-write cycle
> > > > on each of the entry points.
> > >
> > > Is this mutex really needed? kbd_get_state and kbd_set_state are already
> > > locked by mutex. Which situation is trying this patch fix?
> >
> > Yes this is really necessary, between getting the state
> > and storing it writing to another sysfs attribute
> > may cause a change to the state which will then get
> > overwritten by the write of the earlier gotten state.
> >
> > This is a classic read-modify-write race and as such
> > needs protection.
>
> Right, to preserve all changes (by all concurrent modifications) it is
> needed to do that kbd_get_state() + modify + kbd_set_state() atomically.
>
> So this patch fix all concurrent modifications by kernel.
>
> But does not fix race condition when both userspace and kernel want to
> change keyboard brightness settings.

Yes using libsmbios is ALWAYS racy, as said in my previous mails,
people really should use the proper kernel interfaces rather then
directly poking hw from userspace, but just because one path is
racy is not a good reason to not fix the races in another path,
esp, when that other path is the preferred way to do things
and is actually the path which all modern desktop environments use.

Regards,

Hans



> > Regards,
> >
> > Hans
> >
> >
> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > ---
> > > > Changes in v8:
> > > > -New patch in v8 of this patch-set
> > > > ---
> > > > drivers/platform/x86/dell-laptop.c | 112 +++++++++++++++++++++++++------------
> > > > 1 file changed, 76 insertions(+), 36 deletions(-)
> > > >
> > > > diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
> > > > index a2913a5..70951f3 100644
> > > > --- a/drivers/platform/x86/dell-laptop.c
> > > > +++ b/drivers/platform/x86/dell-laptop.c
> > > > @@ -1133,6 +1133,7 @@ static u8 kbd_previous_level;
> > > > static u8 kbd_previous_mode_bit;
> > > >
> > > > static bool kbd_led_present;
> > > > +static DEFINE_MUTEX(kbd_led_mutex);
> > > >
> > > > /*
> > > >  * NOTE: there are three ways to set the keyboard backlight level.
> > > > @@ -1562,9 +1563,11 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> > > > 		}
> > > > 	}
> > > >
> > > > +	mutex_lock(&kbd_led_mutex);
> > > > +
> > > > 	ret = kbd_get_state(&state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > 	new_state = state;
> > > > 	new_state.timeout_value = value;
> > > > @@ -1572,9 +1575,12 @@ static ssize_t kbd_led_timeout_store(struct device *dev,
> > > >
> > > > 	ret = kbd_set_state_safe(&new_state, &state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > -	return count;
> > > > +	ret = count;
> > > > +out:
> > > > +	mutex_unlock(&kbd_led_mutex);
> > > > +	return ret;
> > > > }
> > > >
> > > > static ssize_t kbd_led_timeout_show(struct device *dev,
> > > > @@ -1634,9 +1640,11 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> > > > 	if (trigger[0] != '+' && trigger[0] != '-')
> > > > 		return -EINVAL;
> > > >
> > > > +	mutex_lock(&kbd_led_mutex);
> > > > +
> > > > 	ret = kbd_get_state(&state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > 	if (kbd_triggers_supported)
> > > > 		triggers_enabled = kbd_is_trigger_mode_bit(state.mode_bit);
> > > > @@ -1650,18 +1658,24 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> > > > 			if (strcmp(trigger+1, kbd_led_triggers[i]) != 0)
> > > > 				continue;
> > > > 			if (trigger[0] == '+' &&
> > > > -			    triggers_enabled && (state.triggers & BIT(i)))
> > > > -				return count;
> > > > +			    triggers_enabled && (state.triggers & BIT(i))) {
> > > > +				ret = count;
> > > > +				goto out;
> > > > +			}
> > > > 			if (trigger[0] == '-' &&
> > > > -			    (!triggers_enabled || !(state.triggers & BIT(i))))
> > > > -				return count;
> > > > +			    (!triggers_enabled || !(state.triggers & BIT(i)))) {
> > > > +				ret = count;
> > > > +				goto out;
> > > > +			}
> > > > 			trigger_bit = i;
> > > > 			break;
> > > > 		}
> > > > 	}
> > > >
> > > > -	if (trigger_bit == -1)
> > > > -		return -EINVAL;
> > > > +	if (trigger_bit == -1) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > >
> > > > 	new_state = state;
> > > > 	if (trigger[0] == '+')
> > > > @@ -1677,22 +1691,29 @@ static ssize_t kbd_led_triggers_store(struct device *dev,
> > > > 			new_state.triggers &= ~BIT(2);
> > > > 	}
> > > > 	if ((kbd_info.triggers & new_state.triggers) !=
> > > > -	    new_state.triggers)
> > > > -		return -EINVAL;
> > > > +	    new_state.triggers) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > 	if (new_state.triggers && !triggers_enabled) {
> > > > 		new_state.mode_bit = KBD_MODE_BIT_TRIGGER;
> > > > 		kbd_set_level(&new_state, kbd_previous_level);
> > > > 	} else if (new_state.triggers == 0) {
> > > > 		kbd_set_level(&new_state, 0);
> > > > 	}
> > > > -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> > > > -		return -EINVAL;
> > > > +	if (!(kbd_info.modes & BIT(new_state.mode_bit))) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > 	ret = kbd_set_state_safe(&new_state, &state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > > 	if (new_state.mode_bit != KBD_MODE_BIT_OFF)
> > > > 		kbd_previous_mode_bit = new_state.mode_bit;
> > > > -	return count;
> > > > +	ret = count;
> > > > +out:
> > > > +	mutex_unlock(&kbd_led_mutex);
> > > > +	return ret;
> > > > }
> > > >
> > > > static ssize_t kbd_led_triggers_show(struct device *dev,
> > > > @@ -1749,12 +1770,16 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> > > > 	if (ret)
> > > > 		return ret;
> > > >
> > > > +	mutex_lock(&kbd_led_mutex);
> > > > +
> > > > 	ret = kbd_get_state(&state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > -	if (enable == kbd_is_als_mode_bit(state.mode_bit))
> > > > -		return count;
> > > > +	if (enable == kbd_is_als_mode_bit(state.mode_bit)) {
> > > > +		ret = count;
> > > > +		goto out;
> > > > +	}
> > > >
> > > > 	new_state = state;
> > > >
> > > > @@ -1774,15 +1799,20 @@ static ssize_t kbd_led_als_enabled_store(struct device *dev,
> > > > 			new_state.mode_bit = KBD_MODE_BIT_ON;
> > > > 		}
> > > > 	}
> > > > -	if (!(kbd_info.modes & BIT(new_state.mode_bit)))
> > > > -		return -EINVAL;
> > > > +	if (!(kbd_info.modes & BIT(new_state.mode_bit)))  {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > >
> > > > 	ret = kbd_set_state_safe(&new_state, &state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > > 	kbd_previous_mode_bit = new_state.mode_bit;
> > > >
> > > > -	return count;
> > > > +	ret = count;
> > > > +out:
> > > > +	mutex_unlock(&kbd_led_mutex);
> > > > +	return ret;
> > > > }
> > > >
> > > > static ssize_t kbd_led_als_enabled_show(struct device *dev,
> > > > @@ -1817,18 +1847,23 @@ static ssize_t kbd_led_als_setting_store(struct device *dev,
> > > > 	if (ret)
> > > > 		return ret;
> > > >
> > > > +	mutex_lock(&kbd_led_mutex);
> > > > +
> > > > 	ret = kbd_get_state(&state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > 	new_state = state;
> > > > 	new_state.als_setting = setting;
> > > >
> > > > 	ret = kbd_set_state_safe(&new_state, &state);
> > > > 	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >
> > > > -	return count;
> > > > +	ret = count;
> > > > +out:
> > > > +	mutex_unlock(&kbd_led_mutex);
> > > > +	return ret;
> > > > }
> > > >
> > > > static ssize_t kbd_led_als_setting_show(struct device *dev,
> > > > @@ -1913,27 +1948,32 @@ static int kbd_led_level_set(struct led_classdev *led_cdev,
> > > > 	u16 num;
> > > > 	int ret;
> > > >
> > > > +	mutex_lock(&kbd_led_mutex);
> > > > +
> > > > 	if (kbd_get_max_level()) {
> > > > 		ret = kbd_get_state(&state);
> > > > 		if (ret)
> > > > -			return ret;
> > > > +			goto out;
> > > > 		new_state = state;
> > > > 		ret = kbd_set_level(&new_state, value);
> > > > 		if (ret)
> > > > -			return ret;
> > > > -		return kbd_set_state_safe(&new_state, &state);
> > > > -	}
> > > > -
> > > > -	if (kbd_get_valid_token_counts()) {
> > > > +			goto out;
> > > > +		ret = kbd_set_state_safe(&new_state, &state);
> > > > +	} else if (kbd_get_valid_token_counts()) {
> > > > 		for (num = kbd_token_bits; num != 0 && value > 0; --value)
> > > > 			num &= num - 1; /* clear the first bit set */
> > > > 		if (num == 0)
> > > > -			return 0;
> > > > -		return kbd_set_token_bit(ffs(num) - 1);
> > > > +			ret = 0;
> > > > +		else
> > > > +			ret = kbd_set_token_bit(ffs(num) - 1);
> > > > +	} else {
> > > > +		pr_warn("Keyboard brightness level control not supported\n");
> > > > +		ret = -ENXIO;
> > > > 	}
> > > >
> > > > -	pr_warn("Keyboard brightness level control not supported\n");
> > > > -	return -ENXIO;
> > > > +out:
> > > > +	mutex_unlock(&kbd_led_mutex);
> > > > +	return ret;
> > > > }
> > > >
> > > > static struct led_classdev kbd_led = {