hwmon: amd_energy: modify the visibility of the counters

PATCH In review Naveen Krishna Chatradhi 4 participants 8 messages Nov 12, 2020 – Nov 22, 2020

Subsystems (2)

Stable branch Hardware monitoring

lore

Share & tools

lore

Naveen Krishna Chatradhi

author

2020-11-12 17:22 UTC #0 lore raw

PATCH 1/1 hwmon: amd_energy: modify the visibility of the counters

This patch limits the visibility to owner and groups only for the energy counters exposed through the hwmon based amd_energy driver.

Cc [email protected]

Reviewed-by Greg Kroah-Hartman <[email protected]>

Signed-off-by Naveen Krishna Chatradhi <[email protected]>

---
 drivers/hwmon/amd_energy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hwmon/amd_energy.c +1 -1

index d06597303d5a..3197cda7bcd9 100644
@@ -171,7 +171,7 @@ static umode_t amd_energy_is_visible(const void *_data,
 				     enum hwmon_sensor_types type,
 				     u32 attr, int channel)
 {
-	return 0444;
+	return 0440;
 }
 
 static int energy_accumulator(void *p)

Signature · 1 line

2.17.1

Guenter Roeck

maintainer

2020-11-12 17:24 UTC #1 lore raw

Quoted 6 lines · Naveen Krishna Chatradhi

This patch limits the visibility to owner and groups only for the energy counters exposed through the hwmon based amd_energy driver.
Cc [email protected]
Reviewed-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>

This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?

Guenter

Quoted 18 lines

---
 drivers/hwmon/amd_energy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/hwmon/amd_energy.c +1 -1

index d06597303d5a..3197cda7bcd9 100644
@@ -171,7 +171,7 @@ static umode_t amd_energy_is_visible(const void *_data,
 				     enum hwmon_sensor_types type,
 				     u32 attr, int channel)
 {
-	return 0444;
+	return 0440;
 }
 
 static int energy_accumulator(void *p)

Salvatore Bonaccorso

2020-11-13 13:58 UTC #2 lore raw

Hi,

Quoted 10 lines · Guenter Roeck

Quoted 6 lines · Naveen Krishna Chatradhi
This patch limits the visibility to owner and groups only for the energy counters exposed through the hwmon based amd_energy driver.
Cc [email protected]
Reviewed-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?

Is this related to

https://bugzilla.redhat.com/show_bug.cgi?id=1897402
https://support.lenovo.com/lu/uk/product_security/LEN-50481
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-12912

Regards,
Salvatore

Guenter Roeck

maintainer

2020-11-13 14:47 UTC #3 lore raw

Quoted 11 lines · Salvatore Bonaccorso

Hi,
Quoted 8 lines · Guenter Roeck
Quoted 6 lines · Naveen Krishna Chatradhi
This patch limits the visibility to owner and groups only for the energy counters exposed through the hwmon based amd_energy driver.
Cc [email protected]
Reviewed-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>

Quoted 9 lines

Quoted 2 lines
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?
Is this related to
https://bugzilla.redhat.com/show_bug.cgi?id=1897402
https://support.lenovo.com/lu/uk/product_security/LEN-50481
https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2020-12912

I guess so. The real fix would presumably be to read the power in the background. Of course, that won't work because reading it continuously or frequently causes power fluctuations. I'll apply the patch, but if there are complaints from users afterwards that "sensors" is broken I'll simply revert the entire driver.

Guenter

Chatradhi, Naveen Krishna

2020-11-22 06:56 UTC #4 lore raw

In reply to Salvatore Bonaccorso

[AMD Official Use Only - Approved for External Use]

Hi Guenter, Salvatore

Quoted 2 lines

This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?

Guenter, sorry for the delayed response.
This fix is required to address the possible side channel attack reported in CVE-2020-12912.

Quoted 5 lines

Quoted 5 lines
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1897402&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=RCD5UPLJwh4NkUWf2Uq2r0PTYUC0f6DFDWLAQsrRJZI%3D&reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsupport.lenovo.com%2Flu%2Fuk%2Fproduct_security%2FLEN-50481&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=qBqjid0icKwjI%2Bz38twQqLUYwDzTfvCTF%2Bxzu0dXivY%3D&reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcve.mitre.org%2Fcgi-bin%2Fcvename.cgi%3Fname%3DCVE-2020-12912&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=xftV%2FNo3SvC3sHVKzq74m%2B4OmlYXKjSnSHjebcL%2FGQQ%3D&reserved=0
?

Yes, Salvatore, thanks for bringing the links.

Regards,
Naveenk

-----Original Message-----
From: Salvatore Bonaccorso <[email protected]> On Behalf Of Salvatore Bonaccorso
Sent: Friday, November 13, 2020 7:29 PM
To: Guenter Roeck <[email protected]>
Cc: Chatradhi, Naveen Krishna <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[CAUTION: External Email]

Hi,

Quoted 10 lines · Guenter Roeck

Quoted 6 lines · Naveen Krishna Chatradhi
This patch limits the visibility to owner and groups only for the energy counters exposed through the hwmon based amd_energy driver.
Cc [email protected]
Reviewed-by Greg Kroah-Hartman <[email protected]>
Signed-off-by Naveen Krishna Chatradhi <[email protected]>
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?

Is this related to

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.redhat.com%2Fshow_bug.cgi%3Fid%3D1897402&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=RCD5UPLJwh4NkUWf2Uq2r0PTYUC0f6DFDWLAQsrRJZI%3D&reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsupport.lenovo.com%2Flu%2Fuk%2Fproduct_security%2FLEN-50481&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=qBqjid0icKwjI%2Bz38twQqLUYwDzTfvCTF%2Bxzu0dXivY%3D&reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcve.mitre.org%2Fcgi-bin%2Fcvename.cgi%3Fname%3DCVE-2020-12912&data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=xftV%2FNo3SvC3sHVKzq74m%2B4OmlYXKjSnSHjebcL%2FGQQ%3D&reserved=0

Regards,
Salvatore

Guenter Roeck

maintainer

2020-11-22 13:30 UTC #5 lore raw

Quoted 9 lines · Naveen Krishna

[AMD Official Use Only - Approved for External Use]
Hi Guenter, Salvatore
Quoted 2 lines
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?
Guenter, sorry for the delayed response.
This fix is required to address the possible side channel attack reported in CVE-2020-12912.

[ ... ]

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
?
Yes, Salvatore, thanks for bringing the links.

A much better fix would have been to cache RAPL data for a short period of time. To avoid any possibility of attacks, maybe add some random interval. Something like this:

In accumulate_delta():

	accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;

In amd_energy_read():

	accum = &data->accums[channel];
	if (time_after(accum->next_update))
		accumulate_delta(data, channel, cpu, reg);
	*val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));

and drop amd_add_delta().

Guenter

Chatradhi, Naveen Krishna

2020-11-22 16:43 UTC #6 lore raw

[AMD Official Use Only - Approved for External Use]

Hi Guenter,

Quoted 1 line

A much better fix would have been to cache RAPL data for a short period of time. To avoid any possibility of attacks, maybe add some random interval. Something like this:

Thanks for the tip, I will check this out.

Quoted 6 lines

In accumulate_delta():

       accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;

In amd_energy_read():

       accum = &data->accums[channel];
       if (time_after(accum->next_update))

Do you mean if (time_after(jiffies, accum->next_update))

Quoted 4 lines

               accumulate_delta(data, channel, cpu, reg);
       *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));

and drop amd_add_delta().

Regards,
Naveenk

-----Original Message-----
From: Guenter Roeck <[email protected]>
Sent: Sunday, November 22, 2020 7:00 PM
To: Chatradhi, Naveen Krishna <[email protected]>
Cc: Salvatore Bonaccorso <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[CAUTION: External Email]

Quoted 9 lines · Naveen Krishna

[AMD Official Use Only - Approved for External Use]
Hi Guenter, Salvatore
Quoted 2 lines
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?
Guenter, sorry for the delayed response.
This fix is required to address the possible side channel attack reported in CVE-2020-12912.

[ ... ]

Quoted 4 lines

Quoted 2 lines
Quoted 2 lines
?
Yes, Salvatore, thanks for bringing the links.

A much better fix would have been to cache RAPL data for a short period of time. To avoid any possibility of attacks, maybe add some random interval. Something like this:

In accumulate_delta():

        accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;

In amd_energy_read():

        accum = &data->accums[channel];
        if (time_after(accum->next_update))
                accumulate_delta(data, channel, cpu, reg);
        *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));

and drop amd_add_delta().

Guenter

Guenter Roeck

maintainer

2020-11-22 18:10 UTC #7 lore raw

Quoted 9 lines · Naveen Krishna

[AMD Official Use Only - Approved for External Use]
Hi Guenter,
Quoted 1 line
A much better fix would have been to cache RAPL data for a short period of time. To avoid any possibility of attacks, maybe add some random interval. Something like this:
Thanks for the tip, I will check this out.
Quoted 2 lines
In accumulate_delta():
       accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;

[ and this was supposed to be get_random_int() ]

Quoted 5 lines

Quoted 4 lines
In amd_energy_read():
       accum = &data->accums[channel];
       if (time_after(accum->next_update))
Do you mean if (time_after(jiffies, accum->next_update))

yes ...

Guenter

Quoted 47 lines

Quoted 5 lines
               accumulate_delta(data, channel, cpu, reg);
       *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
and drop amd_add_delta().
Regards,
Naveenk
-----Original Message-----
From: Guenter Roeck <[email protected]>
Sent: Sunday, November 22, 2020 7:00 PM
To: Chatradhi, Naveen Krishna <[email protected]>
Cc: Salvatore Bonaccorso <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters
[CAUTION: External Email]
Quoted 9 lines · Naveen Krishna
[AMD Official Use Only - Approved for External Use]
Hi Guenter, Salvatore
Quoted 2 lines
This is very unusual, and may mess up the "sensors" command. What problem is this trying to solve ?
Guenter, sorry for the delayed response.
This fix is required to address the possible side channel attack reported in CVE-2020-12912.
[ ... ]
Quoted 4 lines
Quoted 2 lines
Quoted 2 lines
?
Yes, Salvatore, thanks for bringing the links.
A much better fix would have been to cache RAPL data for a short period of time. To avoid any possibility of attacks, maybe add some random interval. Something like this:
In accumulate_delta():
        accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;
In amd_energy_read():
        accum = &data->accums[channel];
        if (time_after(accum->next_update))
                accumulate_delta(data, channel, cpu, reg);
        *val = div64_ul(accum->energy_ctr * 1000000UL, BIT(data->energy_units));
and drop amd_add_delta().
Guenter