[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@xxxxxxxxx> On Behalf Of Guenter Roeck Sent: Monday, April 12, 2021 7:56 PM To: Jean Delvare <jdelvare@xxxxxxx> Cc: Hardware Monitoring <linux-hwmon@xxxxxxxxxxxxxxx>; Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@xxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxx> >> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >> Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx> >> --- >> 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@xxxxxxx> > I made the suggested changes. Thanks a lot for the review! Guenter
