* [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data @ 2021-04-09 17:48 Guenter Roeck 2021-04-09 17:48 ` [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters Guenter Roeck 2021-04-12 12:14 ` [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Jean Delvare 0 siblings, 2 replies; 8+ messages in thread From: Guenter Roeck @ 2021-04-09 17:48 UTC (permalink / raw) To: Hardware Monitoring Cc: Jean Delvare, Guenter Roeck, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters 2021-04-09 17:48 [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Guenter Roeck @ 2021-04-09 17:48 ` Guenter Roeck 2021-04-12 12:27 ` Jean Delvare 2021-04-12 12:14 ` [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Jean Delvare 1 sibling, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2021-04-09 17:48 UTC (permalink / raw) To: Hardware Monitoring Cc: Jean Delvare, Guenter Roeck, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters 2021-04-09 17:48 ` [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters Guenter Roeck @ 2021-04-12 12:27 ` Jean Delvare 2021-04-12 14:25 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Jean Delvare @ 2021-04-12 12:27 UTC (permalink / raw) To: Guenter Roeck Cc: Hardware Monitoring, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters 2021-04-12 12:27 ` Jean Delvare @ 2021-04-12 14:25 ` Guenter Roeck 2021-04-19 16:29 ` Chatradhi, Naveen Krishna 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2021-04-12 14:25 UTC (permalink / raw) To: Jean Delvare Cc: Hardware Monitoring, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters 2021-04-12 14:25 ` Guenter Roeck @ 2021-04-19 16:29 ` Chatradhi, Naveen Krishna 2021-04-19 17:41 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Chatradhi, Naveen Krishna @ 2021-04-19 16:29 UTC (permalink / raw) To: Guenter Roeck, Jean Delvare; +Cc: Hardware Monitoring, Greg Kroah-Hartman [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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters 2021-04-19 16:29 ` Chatradhi, Naveen Krishna @ 2021-04-19 17:41 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-04-19 17:41 UTC (permalink / raw) To: Chatradhi, Naveen Krishna Cc: Jean Delvare, Hardware Monitoring, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data 2021-04-09 17:48 [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Guenter Roeck 2021-04-09 17:48 ` [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters Guenter Roeck @ 2021-04-12 12:14 ` Jean Delvare 2021-04-12 14:17 ` Guenter Roeck 1 sibling, 1 reply; 8+ messages in thread From: Jean Delvare @ 2021-04-12 12:14 UTC (permalink / raw) To: Guenter Roeck Cc: Hardware Monitoring, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data 2021-04-12 12:14 ` [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Jean Delvare @ 2021-04-12 14:17 ` Guenter Roeck 0 siblings, 0 replies; 8+ messages in thread From: Guenter Roeck @ 2021-04-12 14:17 UTC (permalink / raw) To: Jean Delvare Cc: Hardware Monitoring, Naveen Krishna Chatradhi, Greg Kroah-Hartman 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-04-19 17:41 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-09 17:48 [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Guenter Roeck 2021-04-09 17:48 ` [PATCH v2 2/2] hwmon: (amd_energy) Restore visibility of energy counters Guenter Roeck 2021-04-12 12:27 ` Jean Delvare 2021-04-12 14:25 ` Guenter Roeck 2021-04-19 16:29 ` Chatradhi, Naveen Krishna 2021-04-19 17:41 ` Guenter Roeck 2021-04-12 12:14 ` [PATCH v2 1/2] hwmon: (amd_energy) Use unified function to read energy data Jean Delvare 2021-04-12 14:17 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.