[PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data

[PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data

[Guenter Roeck]

The driver implements two separate functions to read and update
energy values. One function is called periodically and updates
cached enery information to avoid loss of data, the other reads
energy data on demand to report it to userspace. The second
function reads current energy data, adds the difference to the
data previously obtained by the first function, and then discards
the updated data.

Simplify the code and always call the first function, then report
the energy data obtained by this function to userspace.

Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Added patch, simplification

 drivers/hwmon/amd_energy.c | 31 ++++++-------------------------
 1 file changed, 6 insertions(+), 25 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index a86cc8d6d93d..93bad64039f1 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -118,35 +118,12 @@ static void read_accumulate(struct amd_energy_data *data)
 	data->core_id++;
 }
 
-static void amd_add_delta(struct amd_energy_data *data, int ch,
-			  int cpu, long *val, u32 reg)
-{
-	struct sensor_accumulator *accum;
-	u64 input;
-
-	mutex_lock(&data->lock);
-	rdmsrl_safe_on_cpu(cpu, reg, &input);
-	input &= AMD_ENERGY_MASK;
-
-	accum = &data->accums[ch];
-	if (input >= accum->prev_value)
-		input += accum->energy_ctr -
-				accum->prev_value;
-	else
-		input += UINT_MAX - accum->prev_value +
-				accum->energy_ctr;
-
-	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
-	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
-
-	mutex_unlock(&data->lock);
-}
-
 static int amd_energy_read(struct device *dev,
 			   enum hwmon_sensor_types type,
 			   u32 attr, int channel, long *val)
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
+	struct sensor_accumulator *accum;
 	u32 reg;
 	int cpu;
 
@@ -162,7 +139,11 @@ static int amd_energy_read(struct device *dev,
 
 		reg = ENERGY_CORE_MSR;
 	}
-	amd_add_delta(data, channel, cpu, val, reg);
+
+	accumulate_delta(data, channel, cpu, reg);
+	accum = &data->accums[channel];
+
+	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[Guenter Roeck]

Commit 60268b0e8258 ("hwmon: (amd_energy) modify the visibility of
the counters") restricted visibility of AMD energy counters to work
around a side-channel attack using energy data to determine which
instructions are executed. The attack is described in 'PLATYPUS:
Software-based Power Side-Channel Attacks on x86'. It relies on quick
and accurate energy readings.

Limiting energy readings to privileged users is annoying. A much better
solution is to make energy readings unusable for attacks by randomizing
the time between updates. We can do that by caching energy values for
a short and randomized period of time.

Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
v2: Simplified code by using unified function to accumulate energy data

 drivers/hwmon/amd_energy.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index 93bad64039f1..1bf0afc2740c 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -18,6 +18,7 @@
 #include <linux/mutex.h>
 #include <linux/processor.h>
 #include <linux/platform_device.h>
+#include <linux/random.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
 #include <linux/topology.h>
@@ -35,6 +36,7 @@
 struct sensor_accumulator {
 	u64 energy_ctr;
 	u64 prev_value;
+	unsigned long cache_timeout;
 };
 
 struct amd_energy_data {
@@ -74,17 +76,14 @@ static void get_energy_units(struct amd_energy_data *data)
 	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
 }
 
-static void accumulate_delta(struct amd_energy_data *data,
-			     int channel, int cpu, u32 reg)
+static void __accumulate_delta(struct sensor_accumulator *accum,
+			       int cpu, u32 reg)
 {
-	struct sensor_accumulator *accum;
 	u64 input;
 
-	mutex_lock(&data->lock);
 	rdmsrl_safe_on_cpu(cpu, reg, &input);
 	input &= AMD_ENERGY_MASK;
 
-	accum = &data->accums[channel];
 	if (input >= accum->prev_value)
 		accum->energy_ctr +=
 			input - accum->prev_value;
@@ -93,6 +92,14 @@ static void accumulate_delta(struct amd_energy_data *data,
 			accum->prev_value + input;
 
 	accum->prev_value = input;
+	accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;
+}
+
+static void accumulate_delta(struct amd_energy_data *data,
+			     int channel, int cpu, u32 reg)
+{
+	mutex_lock(&data->lock);
+	__accumulate_delta(&data->accums[channel], cpu, reg);
 	mutex_unlock(&data->lock);
 }
 
@@ -124,6 +131,7 @@ static int amd_energy_read(struct device *dev,
 {
 	struct amd_energy_data *data = dev_get_drvdata(dev);
 	struct sensor_accumulator *accum;
+	u64 energy;
 	u32 reg;
 	int cpu;
 
@@ -140,10 +148,15 @@ static int amd_energy_read(struct device *dev,
 		reg = ENERGY_CORE_MSR;
 	}
 
-	accumulate_delta(data, channel, cpu, reg);
 	accum = &data->accums[channel];
 
-	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
+	mutex_lock(&data->lock);
+	if (!accum->energy_ctr || time_after(jiffies, accum->cache_timeout))
+		__accumulate_delta(accum, cpu, reg);
+	energy = accum->energy_ctr;
+	mutex_unlock(&data->lock);
+
+	*val = div64_ul(energy * 1000000UL, BIT(data->energy_units));
 
 	return 0;
 }
@@ -152,7 +165,7 @@ static umode_t amd_energy_is_visible(const void *_data,
 				     enum hwmon_sensor_types type,
 				     u32 attr, int channel)
 {
-	return 0440;
+	return 0444;
 }
 
 static int energy_accumulator(void *p)
-- 
2.17.1

Re: [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data

[Jean Delvare]

Hi Guenter,

On Fri,  9 Apr 2021 10:48:51 -0700, Guenter Roeck wrote:
> The driver implements two separate functions to read and update
> energy values. One function is called periodically and updates
> cached enery information to avoid loss of data, the other reads
> energy data on demand to report it to userspace. The second
> function reads current energy data, adds the difference to the
> data previously obtained by the first function, and then discards
> the updated data.
> 
> Simplify the code and always call the first function, then report
> the energy data obtained by this function to userspace.

I like the idea.

> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Added patch, simplification
> 
>  drivers/hwmon/amd_energy.c | 31 ++++++-------------------------
>  1 file changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index a86cc8d6d93d..93bad64039f1 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -118,35 +118,12 @@ static void read_accumulate(struct amd_energy_data *data)
>  	data->core_id++;
>  }
>  
> -static void amd_add_delta(struct amd_energy_data *data, int ch,
> -			  int cpu, long *val, u32 reg)
> -{
> -	struct sensor_accumulator *accum;
> -	u64 input;
> -
> -	mutex_lock(&data->lock);
> -	rdmsrl_safe_on_cpu(cpu, reg, &input);
> -	input &= AMD_ENERGY_MASK;
> -
> -	accum = &data->accums[ch];
> -	if (input >= accum->prev_value)
> -		input += accum->energy_ctr -
> -				accum->prev_value;
> -	else
> -		input += UINT_MAX - accum->prev_value +
> -				accum->energy_ctr;
> -
> -	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
> -	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
> -
> -	mutex_unlock(&data->lock);
> -}
> -
>  static int amd_energy_read(struct device *dev,
>  			   enum hwmon_sensor_types type,
>  			   u32 attr, int channel, long *val)
>  {
>  	struct amd_energy_data *data = dev_get_drvdata(dev);
> +	struct sensor_accumulator *accum;
>  	u32 reg;
>  	int cpu;
>  
> @@ -162,7 +139,11 @@ static int amd_energy_read(struct device *dev,
>  
>  		reg = ENERGY_CORE_MSR;
>  	}
> -	amd_add_delta(data, channel, cpu, val, reg);
> +
> +	accumulate_delta(data, channel, cpu, reg);
> +	accum = &data->accums[channel];
> +
> +	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));

Is it considered safe to read accum->energy_ctr while not holding
data->lock?

>  
>  	return 0;
>  }

If the answer to the question above is "yes" then:

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[Jean Delvare]

On Fri,  9 Apr 2021 10:48:52 -0700, Guenter Roeck wrote:
> Commit 60268b0e8258 ("hwmon: (amd_energy) modify the visibility of
> the counters") restricted visibility of AMD energy counters to work
> around a side-channel attack using energy data to determine which
> instructions are executed. The attack is described in 'PLATYPUS:
> Software-based Power Side-Channel Attacks on x86'. It relies on quick
> and accurate energy readings.
> 
> Limiting energy readings to privileged users is annoying. A much better
> solution is to make energy readings unusable for attacks by randomizing
> the time between updates. We can do that by caching energy values for
> a short and randomized period of time.
> 
> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> v2: Simplified code by using unified function to accumulate energy data
> 
>  drivers/hwmon/amd_energy.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index 93bad64039f1..1bf0afc2740c 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -18,6 +18,7 @@
>  #include <linux/mutex.h>
>  #include <linux/processor.h>
>  #include <linux/platform_device.h>
> +#include <linux/random.h>
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/topology.h>
> @@ -35,6 +36,7 @@
>  struct sensor_accumulator {
>  	u64 energy_ctr;
>  	u64 prev_value;
> +	unsigned long cache_timeout;
>  };
>  
>  struct amd_energy_data {
> @@ -74,17 +76,14 @@ static void get_energy_units(struct amd_energy_data *data)
>  	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
>  }
>  

Add a comment stating that this function must be called with accum's
&data->lock held?

> -static void accumulate_delta(struct amd_energy_data *data,
> -			     int channel, int cpu, u32 reg)
> +static void __accumulate_delta(struct sensor_accumulator *accum,
> +			       int cpu, u32 reg)
>  {
> -	struct sensor_accumulator *accum;
>  	u64 input;
>  
> -	mutex_lock(&data->lock);
>  	rdmsrl_safe_on_cpu(cpu, reg, &input);
>  	input &= AMD_ENERGY_MASK;
>  
> -	accum = &data->accums[channel];
>  	if (input >= accum->prev_value)
>  		accum->energy_ctr +=
>  			input - accum->prev_value;
> @@ -93,6 +92,14 @@ static void accumulate_delta(struct amd_energy_data *data,
>  			accum->prev_value + input;
>  
>  	accum->prev_value = input;
> +	accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;

Needs #include <linux/jiffies.h> maybe?

> +}
> +
> +static void accumulate_delta(struct amd_energy_data *data,
> +			     int channel, int cpu, u32 reg)
> +{
> +	mutex_lock(&data->lock);
> +	__accumulate_delta(&data->accums[channel], cpu, reg);
>  	mutex_unlock(&data->lock);
>  }
>  
> @@ -124,6 +131,7 @@ static int amd_energy_read(struct device *dev,
>  {
>  	struct amd_energy_data *data = dev_get_drvdata(dev);
>  	struct sensor_accumulator *accum;
> +	u64 energy;
>  	u32 reg;
>  	int cpu;
>  
> @@ -140,10 +148,15 @@ static int amd_energy_read(struct device *dev,
>  		reg = ENERGY_CORE_MSR;
>  	}
>  
> -	accumulate_delta(data, channel, cpu, reg);
>  	accum = &data->accums[channel];
>  
> -	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
> +	mutex_lock(&data->lock);
> +	if (!accum->energy_ctr || time_after(jiffies, accum->cache_timeout))
> +		__accumulate_delta(accum, cpu, reg);
> +	energy = accum->energy_ctr;
> +	mutex_unlock(&data->lock);
> +
> +	*val = div64_ul(energy * 1000000UL, BIT(data->energy_units));
>  
>  	return 0;
>  }
> @@ -152,7 +165,7 @@ static umode_t amd_energy_is_visible(const void *_data,
>  				     enum hwmon_sensor_types type,
>  				     u32 attr, int channel)
>  {
> -	return 0440;
> +	return 0444;
>  }
>  
>  static int energy_accumulator(void *p)

Very nice. This will make the driver useful again :-)

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data

[Guenter Roeck]

On 4/12/21 5:14 AM, Jean Delvare wrote:
> Hi Guenter,
> 
> On Fri,  9 Apr 2021 10:48:51 -0700, Guenter Roeck wrote:
>> The driver implements two separate functions to read and update
>> energy values. One function is called periodically and updates
>> cached enery information to avoid loss of data, the other reads
>> energy data on demand to report it to userspace. The second
>> function reads current energy data, adds the difference to the
>> data previously obtained by the first function, and then discards
>> the updated data.
>>
>> Simplify the code and always call the first function, then report
>> the energy data obtained by this function to userspace.
> 
> I like the idea.
> 
>> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Added patch, simplification
>>
>>  drivers/hwmon/amd_energy.c | 31 ++++++-------------------------
>>  1 file changed, 6 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>> index a86cc8d6d93d..93bad64039f1 100644
>> --- a/drivers/hwmon/amd_energy.c
>> +++ b/drivers/hwmon/amd_energy.c
>> @@ -118,35 +118,12 @@ static void read_accumulate(struct amd_energy_data *data)
>>  	data->core_id++;
>>  }
>>  
>> -static void amd_add_delta(struct amd_energy_data *data, int ch,
>> -			  int cpu, long *val, u32 reg)
>> -{
>> -	struct sensor_accumulator *accum;
>> -	u64 input;
>> -
>> -	mutex_lock(&data->lock);
>> -	rdmsrl_safe_on_cpu(cpu, reg, &input);
>> -	input &= AMD_ENERGY_MASK;
>> -
>> -	accum = &data->accums[ch];
>> -	if (input >= accum->prev_value)
>> -		input += accum->energy_ctr -
>> -				accum->prev_value;
>> -	else
>> -		input += UINT_MAX - accum->prev_value +
>> -				accum->energy_ctr;
>> -
>> -	/* Energy consumed = (1/(2^ESU) * RAW * 1000000UL) μJoules */
>> -	*val = div64_ul(input * 1000000UL, BIT(data->energy_units));
>> -
>> -	mutex_unlock(&data->lock);
>> -}
>> -
>>  static int amd_energy_read(struct device *dev,
>>  			   enum hwmon_sensor_types type,
>>  			   u32 attr, int channel, long *val)
>>  {
>>  	struct amd_energy_data *data = dev_get_drvdata(dev);
>> +	struct sensor_accumulator *accum;
>>  	u32 reg;
>>  	int cpu;
>>  
>> @@ -162,7 +139,11 @@ static int amd_energy_read(struct device *dev,
>>  
>>  		reg = ENERGY_CORE_MSR;
>>  	}
>> -	amd_add_delta(data, channel, cpu, val, reg);
>> +
>> +	accumulate_delta(data, channel, cpu, reg);
>> +	accum = &data->accums[channel];
>> +
>> +	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
> 
> Is it considered safe to read accum->energy_ctr while not holding
> data->lock?
> 

You mean because it is a 64-bit value ? Good question; it might not be if compiled
on a 32-bit system. Good news is that I moved reading accum->energy_ctr under the lock
in the next patch, so we should be good.

>>  
>>  	return 0;
>>  }
> 
> If the answer to the question above is "yes" then:
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
Thanks!

Guenter

Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[Guenter Roeck]

On 4/12/21 5:27 AM, Jean Delvare wrote:
> On Fri,  9 Apr 2021 10:48:52 -0700, Guenter Roeck wrote:
>> Commit 60268b0e8258 ("hwmon: (amd_energy) modify the visibility of
>> the counters") restricted visibility of AMD energy counters to work
>> around a side-channel attack using energy data to determine which
>> instructions are executed. The attack is described in 'PLATYPUS:
>> Software-based Power Side-Channel Attacks on x86'. It relies on quick
>> and accurate energy readings.
>>
>> Limiting energy readings to privileged users is annoying. A much better
>> solution is to make energy readings unusable for attacks by randomizing
>> the time between updates. We can do that by caching energy values for
>> a short and randomized period of time.
>>
>> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Simplified code by using unified function to accumulate energy data
>>
>>  drivers/hwmon/amd_energy.c | 29 +++++++++++++++++++++--------
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
>> index 93bad64039f1..1bf0afc2740c 100644
>> --- a/drivers/hwmon/amd_energy.c
>> +++ b/drivers/hwmon/amd_energy.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/processor.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/random.h>
>>  #include <linux/sched.h>
>>  #include <linux/slab.h>
>>  #include <linux/topology.h>
>> @@ -35,6 +36,7 @@
>>  struct sensor_accumulator {
>>  	u64 energy_ctr;
>>  	u64 prev_value;
>> +	unsigned long cache_timeout;
>>  };
>>  
>>  struct amd_energy_data {
>> @@ -74,17 +76,14 @@ static void get_energy_units(struct amd_energy_data *data)
>>  	data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;
>>  }
>>  
> 
> Add a comment stating that this function must be called with accum's
> &data->lock held?
> 
>> -static void accumulate_delta(struct amd_energy_data *data,
>> -			     int channel, int cpu, u32 reg)
>> +static void __accumulate_delta(struct sensor_accumulator *accum,
>> +			       int cpu, u32 reg)
>>  {
>> -	struct sensor_accumulator *accum;
>>  	u64 input;
>>  
>> -	mutex_lock(&data->lock);
>>  	rdmsrl_safe_on_cpu(cpu, reg, &input);
>>  	input &= AMD_ENERGY_MASK;
>>  
>> -	accum = &data->accums[channel];
>>  	if (input >= accum->prev_value)
>>  		accum->energy_ctr +=
>>  			input - accum->prev_value;
>> @@ -93,6 +92,14 @@ static void accumulate_delta(struct amd_energy_data *data,
>>  			accum->prev_value + input;
>>  
>>  	accum->prev_value = input;
>> +	accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;
> 
> Needs #include <linux/jiffies.h> maybe?
> 
>> +}
>> +
>> +static void accumulate_delta(struct amd_energy_data *data,
>> +			     int channel, int cpu, u32 reg)
>> +{
>> +	mutex_lock(&data->lock);
>> +	__accumulate_delta(&data->accums[channel], cpu, reg);
>>  	mutex_unlock(&data->lock);
>>  }
>>  
>> @@ -124,6 +131,7 @@ static int amd_energy_read(struct device *dev,
>>  {
>>  	struct amd_energy_data *data = dev_get_drvdata(dev);
>>  	struct sensor_accumulator *accum;
>> +	u64 energy;
>>  	u32 reg;
>>  	int cpu;
>>  
>> @@ -140,10 +148,15 @@ static int amd_energy_read(struct device *dev,
>>  		reg = ENERGY_CORE_MSR;
>>  	}
>>  
>> -	accumulate_delta(data, channel, cpu, reg);
>>  	accum = &data->accums[channel];
>>  
>> -	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
>> +	mutex_lock(&data->lock);
>> +	if (!accum->energy_ctr || time_after(jiffies, accum->cache_timeout))
>> +		__accumulate_delta(accum, cpu, reg);
>> +	energy = accum->energy_ctr;
>> +	mutex_unlock(&data->lock);
>> +
>> +	*val = div64_ul(energy * 1000000UL, BIT(data->energy_units));
>>  
>>  	return 0;
>>  }
>> @@ -152,7 +165,7 @@ static umode_t amd_energy_is_visible(const void *_data,
>>  				     enum hwmon_sensor_types type,
>>  				     u32 attr, int channel)
>>  {
>> -	return 0440;
>> +	return 0444;
>>  }
>>  
>>  static int energy_accumulator(void *p)
> 
> Very nice. This will make the driver useful again :-)
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> 
I made the suggested changes.

Thanks a lot for the review!

Guenter

RE: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[Chatradhi, Naveen Krishna]

[AMD Official Use Only - Internal Distribution Only]

Hi Guenter,

>>      accum->prev_value = input;
>> +    accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;

I've noticed this change is reviewed and accepted, please note “AMD guidance remains to restrict the RAPL MSR access to privilege users for the CVE-2020-12912. See 2020 tab in https://www.amd.com/en/corporate/product-security#paragraph-313561”

Regards,
Naveenk

-----Original Message-----
From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
Sent: Monday, April 12, 2021 7:56 PM
To: Jean Delvare <jdelvare@suse.de>
Cc: Hardware Monitoring <linux-hwmon@vger.kernel.org>; Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Subject: Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[CAUTION: External Email]

On 4/12/21 5:27 AM, Jean Delvare wrote:
> On Fri,  9 Apr 2021 10:48:52 -0700, Guenter Roeck wrote:
>> Commit 60268b0e8258 ("hwmon: (amd_energy) modify the visibility of 
>> the counters") restricted visibility of AMD energy counters to work 
>> around a side-channel attack using energy data to determine which 
>> instructions are executed. The attack is described in 'PLATYPUS:
>> Software-based Power Side-Channel Attacks on x86'. It relies on quick 
>> and accurate energy readings.
>>
>> Limiting energy readings to privileged users is annoying. A much 
>> better solution is to make energy readings unusable for attacks by 
>> randomizing the time between updates. We can do that by caching 
>> energy values for a short and randomized period of time.
>>
>> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
>> ---
>> v2: Simplified code by using unified function to accumulate energy 
>> data
>>
>>  drivers/hwmon/amd_energy.c | 29 +++++++++++++++++++++--------
>>  1 file changed, 21 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c 
>> index 93bad64039f1..1bf0afc2740c 100644
>> --- a/drivers/hwmon/amd_energy.c
>> +++ b/drivers/hwmon/amd_energy.c
>> @@ -18,6 +18,7 @@
>>  #include <linux/mutex.h>
>>  #include <linux/processor.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/random.h>
>>  #include <linux/sched.h>
>>  #include <linux/slab.h>
>>  #include <linux/topology.h>
>> @@ -35,6 +36,7 @@
>>  struct sensor_accumulator {
>>      u64 energy_ctr;
>>      u64 prev_value;
>> +    unsigned long cache_timeout;
>>  };
>>
>>  struct amd_energy_data {
>> @@ -74,17 +76,14 @@ static void get_energy_units(struct amd_energy_data *data)
>>      data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;  
>> }
>>
>
> Add a comment stating that this function must be called with accum's 
> &data->lock held?
>
>> -static void accumulate_delta(struct amd_energy_data *data,
>> -                         int channel, int cpu, u32 reg)
>> +static void __accumulate_delta(struct sensor_accumulator *accum,
>> +                           int cpu, u32 reg)
>>  {
>> -    struct sensor_accumulator *accum;
>>      u64 input;
>>
>> -    mutex_lock(&data->lock);
>>      rdmsrl_safe_on_cpu(cpu, reg, &input);
>>      input &= AMD_ENERGY_MASK;
>>
>> -    accum = &data->accums[channel];
>>      if (input >= accum->prev_value)
>>              accum->energy_ctr +=
>>                      input - accum->prev_value; @@ -93,6 +92,14 @@ 
>> static void accumulate_delta(struct amd_energy_data *data,
>>                      accum->prev_value + input;
>>
>>      accum->prev_value = input;
>> +    accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;
>
> Needs #include <linux/jiffies.h> maybe?
>
>> +}
>> +
>> +static void accumulate_delta(struct amd_energy_data *data,
>> +                         int channel, int cpu, u32 reg) {
>> +    mutex_lock(&data->lock);
>> +    __accumulate_delta(&data->accums[channel], cpu, reg);
>>      mutex_unlock(&data->lock);
>>  }
>>
>> @@ -124,6 +131,7 @@ static int amd_energy_read(struct device *dev,  {
>>      struct amd_energy_data *data = dev_get_drvdata(dev);
>>      struct sensor_accumulator *accum;
>> +    u64 energy;
>>      u32 reg;
>>      int cpu;
>>
>> @@ -140,10 +148,15 @@ static int amd_energy_read(struct device *dev,
>>              reg = ENERGY_CORE_MSR;
>>      }
>>
>> -    accumulate_delta(data, channel, cpu, reg);
>>      accum = &data->accums[channel];
>>
>> -    *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
>> +    mutex_lock(&data->lock);
>> +    if (!accum->energy_ctr || time_after(jiffies, accum->cache_timeout))
>> +            __accumulate_delta(accum, cpu, reg);
>> +    energy = accum->energy_ctr;
>> +    mutex_unlock(&data->lock);
>> +
>> +    *val = div64_ul(energy * 1000000UL, BIT(data->energy_units));
>>
>>      return 0;
>>  }
>> @@ -152,7 +165,7 @@ static umode_t amd_energy_is_visible(const void *_data,
>>                                   enum hwmon_sensor_types type,
>>                                   u32 attr, int channel)  {
>> -    return 0440;
>> +    return 0444;
>>  }
>>
>>  static int energy_accumulator(void *p)
>
> Very nice. This will make the driver useful again :-)
>
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
>
I made the suggested changes.

Thanks a lot for the review!

Guenter

Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters

[Guenter Roeck]

On Mon, Apr 19, 2021 at 04:29:59PM +0000, Chatradhi, Naveen Krishna wrote:
> [AMD Official Use Only - Internal Distribution Only]
> 
> Hi Guenter,
> 
> >>      accum->prev_value = input;
> >> +    accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;
> 
> I've noticed this change is reviewed and accepted, please note “AMD guidance remains to restrict the RAPL MSR access to privilege users for the CVE-2020-12912. See 2020 tab in https://www.amd.com/en/corporate/product-security#paragraph-313561”
> 

This has been on the list for a while, so your feedback is a tiny
bit late.

Please feel free to send a NACK to the patch. If my suggested solution
is not acceptable, I'll remove the driver entirely and add a note
to the sysfs ABI stating that attributes MUST be world readable
for a driver to be acceptable. After all, this patch forces users
of the hwmon ABI to run previously restricted applications as
super-user (or to revert the patch introducing the restrictions
in their private builds), which completely defeats the purpose
of the patch and opens up additional unnecessary attack surface. 

Thanks,
Guenter

> Regards,
> Naveenk
> 
> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, April 12, 2021 7:56 PM
> To: Jean Delvare <jdelvare@suse.de>
> Cc: Hardware Monitoring <linux-hwmon@vger.kernel.org>; Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Subject: Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters
> 
> [CAUTION: External Email]
> 
> On 4/12/21 5:27 AM, Jean Delvare wrote:
> > On Fri,  9 Apr 2021 10:48:52 -0700, Guenter Roeck wrote:
> >> Commit 60268b0e8258 ("hwmon: (amd_energy) modify the visibility of 
> >> the counters") restricted visibility of AMD energy counters to work 
> >> around a side-channel attack using energy data to determine which 
> >> instructions are executed. The attack is described in 'PLATYPUS:
> >> Software-based Power Side-Channel Attacks on x86'. It relies on quick 
> >> and accurate energy readings.
> >>
> >> Limiting energy readings to privileged users is annoying. A much 
> >> better solution is to make energy readings unusable for attacks by 
> >> randomizing the time between updates. We can do that by caching 
> >> energy values for a short and randomized period of time.
> >>
> >> Cc: Naveen Krishna Chatradhi <nchatrad@amd.com>
> >> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> >> ---
> >> v2: Simplified code by using unified function to accumulate energy 
> >> data
> >>
> >>  drivers/hwmon/amd_energy.c | 29 +++++++++++++++++++++--------
> >>  1 file changed, 21 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c 
> >> index 93bad64039f1..1bf0afc2740c 100644
> >> --- a/drivers/hwmon/amd_energy.c
> >> +++ b/drivers/hwmon/amd_energy.c
> >> @@ -18,6 +18,7 @@
> >>  #include <linux/mutex.h>
> >>  #include <linux/processor.h>
> >>  #include <linux/platform_device.h>
> >> +#include <linux/random.h>
> >>  #include <linux/sched.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/topology.h>
> >> @@ -35,6 +36,7 @@
> >>  struct sensor_accumulator {
> >>      u64 energy_ctr;
> >>      u64 prev_value;
> >> +    unsigned long cache_timeout;
> >>  };
> >>
> >>  struct amd_energy_data {
> >> @@ -74,17 +76,14 @@ static void get_energy_units(struct amd_energy_data *data)
> >>      data->energy_units = (rapl_units & AMD_ENERGY_UNIT_MASK) >> 8;  
> >> }
> >>
> >
> > Add a comment stating that this function must be called with accum's 
> > &data->lock held?
> >
> >> -static void accumulate_delta(struct amd_energy_data *data,
> >> -                         int channel, int cpu, u32 reg)
> >> +static void __accumulate_delta(struct sensor_accumulator *accum,
> >> +                           int cpu, u32 reg)
> >>  {
> >> -    struct sensor_accumulator *accum;
> >>      u64 input;
> >>
> >> -    mutex_lock(&data->lock);
> >>      rdmsrl_safe_on_cpu(cpu, reg, &input);
> >>      input &= AMD_ENERGY_MASK;
> >>
> >> -    accum = &data->accums[channel];
> >>      if (input >= accum->prev_value)
> >>              accum->energy_ctr +=
> >>                      input - accum->prev_value; @@ -93,6 +92,14 @@ 
> >> static void accumulate_delta(struct amd_energy_data *data,
> >>                      accum->prev_value + input;
> >>
> >>      accum->prev_value = input;
> >> +    accum->cache_timeout = jiffies + HZ + get_random_int() % HZ;
> >
> > Needs #include <linux/jiffies.h> maybe?
> >
> >> +}
> >> +
> >> +static void accumulate_delta(struct amd_energy_data *data,
> >> +                         int channel, int cpu, u32 reg) {
> >> +    mutex_lock(&data->lock);
> >> +    __accumulate_delta(&data->accums[channel], cpu, reg);
> >>      mutex_unlock(&data->lock);
> >>  }
> >>
> >> @@ -124,6 +131,7 @@ static int amd_energy_read(struct device *dev,  {
> >>      struct amd_energy_data *data = dev_get_drvdata(dev);
> >>      struct sensor_accumulator *accum;
> >> +    u64 energy;
> >>      u32 reg;
> >>      int cpu;
> >>
> >> @@ -140,10 +148,15 @@ static int amd_energy_read(struct device *dev,
> >>              reg = ENERGY_CORE_MSR;
> >>      }
> >>
> >> -    accumulate_delta(data, channel, cpu, reg);
> >>      accum = &data->accums[channel];
> >>
> >> -    *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
> >> +    mutex_lock(&data->lock);
> >> +    if (!accum->energy_ctr || time_after(jiffies, accum->cache_timeout))
> >> +            __accumulate_delta(accum, cpu, reg);
> >> +    energy = accum->energy_ctr;
> >> +    mutex_unlock(&data->lock);
> >> +
> >> +    *val = div64_ul(energy * 1000000UL, BIT(data->energy_units));
> >>
> >>      return 0;
> >>  }
> >> @@ -152,7 +165,7 @@ static umode_t amd_energy_is_visible(const void *_data,
> >>                                   enum hwmon_sensor_types type,
> >>                                   u32 attr, int channel)  {
> >> -    return 0440;
> >> +    return 0444;
> >>  }
> >>
> >>  static int energy_accumulator(void *p)
> >
> > Very nice. This will make the driver useful again :-)
> >
> > Reviewed-by: Jean Delvare <jdelvare@suse.de>
> >
> I made the suggested changes.
> 
> Thanks a lot for the review!
> 
> Guenter