[PATCH] hwmon: amd_energy: modify the visibility of the counters

[PATCH] hwmon: amd_energy: modify the visibility of the counters

[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: stable@vger.kernel.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index d06597303d5a..3197cda7bcd9 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -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)
-- 
2.17.1

Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Guenter Roeck]

On 11/12/20 9:21 AM, Naveen Krishna Chatradhi wrote:
> This patch limits the visibility to owner and groups only for the
> energy counters exposed through the hwmon based amd_energy driver.
> 
> Cc: stable@vger.kernel.org
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>

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

Guenter

> ---
>  drivers/hwmon/amd_energy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
> index d06597303d5a..3197cda7bcd9 100644
> --- a/drivers/hwmon/amd_energy.c
> +++ b/drivers/hwmon/amd_energy.c
> @@ -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)
> 

Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Salvatore Bonaccorso]

Hi,

On Thu, Nov 12, 2020 at 09:24:22AM -0800, Guenter Roeck wrote:
> On 11/12/20 9:21 AM, Naveen Krishna Chatradhi wrote:
> > This patch limits the visibility to owner and groups only for the
> > energy counters exposed through the hwmon based amd_energy driver.
> > 
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
> 
> 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

Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Guenter Roeck]

On 11/13/20 5:58 AM, Salvatore Bonaccorso wrote:
> Hi,
> 
> On Thu, Nov 12, 2020 at 09:24:22AM -0800, Guenter Roeck wrote:
>> On 11/12/20 9:21 AM, Naveen Krishna Chatradhi wrote:
>>> This patch limits the visibility to owner and groups only for the
>>> energy counters exposed through the hwmon based amd_energy driver.
>>>
>>> Cc: stable@vger.kernel.org
>>> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>>
>> 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

RE: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Chatradhi, Naveen Krishna]

[AMD Official Use Only - Approved for External Use]

Hi Guenter, Salvatore

> 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.

>> 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 <salvatore.bonaccorso@gmail.com> On Behalf Of Salvatore Bonaccorso
Sent: Friday, November 13, 2020 7:29 PM
To: Guenter Roeck <linux@roeck-us.net>
Cc: Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>; linux-hwmon@vger.kernel.org; naveenkrishna.ch@gmail.com; stable@vger.kernel.org
Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[CAUTION: External Email]

Hi,

On Thu, Nov 12, 2020 at 09:24:22AM -0800, Guenter Roeck wrote:
> On 11/12/20 9:21 AM, Naveen Krishna Chatradhi wrote:
> > This patch limits the visibility to owner and groups only for the 
> > energy counters exposed through the hwmon based amd_energy driver.
> >
> > Cc: stable@vger.kernel.org
> > Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
>
> 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&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=RCD5UPLJwh4NkUWf2Uq2r0PTYUC0f6DFDWLAQsrRJZI%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsupport.lenovo.com%2Flu%2Fuk%2Fproduct_security%2FLEN-50481&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=qBqjid0icKwjI%2Bz38twQqLUYwDzTfvCTF%2Bxzu0dXivY%3D&amp;reserved=0
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcve.mitre.org%2Fcgi-bin%2Fcvename.cgi%3Fname%3DCVE-2020-12912&amp;data=04%7C01%7CNaveenKrishna.Chatradhi%40amd.com%7C7672335ee2904d59fb5008d887dc381b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637408727764403328%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&amp;sdata=xftV%2FNo3SvC3sHVKzq74m%2B4OmlYXKjSnSHjebcL%2FGQQ%3D&amp;reserved=0

?

Regards,
Salvatore

Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Guenter Roeck]

On Sun, Nov 22, 2020 at 06:56:24AM +0000, Chatradhi, Naveen Krishna wrote:
> [AMD Official Use Only - Approved for External Use]
> 
> Hi Guenter, Salvatore
> 
> > 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.
> 
[ ... ]
> 
> >> ?
> 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

RE: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Chatradhi, Naveen Krishna]

[AMD Official Use Only - Approved for External Use]

Hi Guenter,

> 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.

> 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))

>                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 <linux@roeck-us.net> 
Sent: Sunday, November 22, 2020 7:00 PM
To: Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>
Cc: Salvatore Bonaccorso <carnil@debian.org>; linux-hwmon@vger.kernel.org; naveenkrishna.ch@gmail.com; stable@vger.kernel.org
Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[CAUTION: External Email]

On Sun, Nov 22, 2020 at 06:56:24AM +0000, Chatradhi, Naveen Krishna wrote:
> [AMD Official Use Only - Approved for External Use]
>
> Hi Guenter, Salvatore
>
> > 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.
>
[ ... ]
>
> >> ?
> 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

Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters

[Guenter Roeck]

On Sun, Nov 22, 2020 at 04:42:47PM +0000, Chatradhi, Naveen Krishna wrote:
> [AMD Official Use Only - Approved for External Use]
> 
> Hi Guenter,
> 
> > 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.
> 
> > In accumulate_delta():
> >        accums->next_update = jiffies + HZ / 2 + get_random_int % HZ;

[ and this was supposed to be get_random_int() ]

> 
> > 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

> 
> >                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 <linux@roeck-us.net> 
> Sent: Sunday, November 22, 2020 7:00 PM
> To: Chatradhi, Naveen Krishna <NaveenKrishna.Chatradhi@amd.com>
> Cc: Salvatore Bonaccorso <carnil@debian.org>; linux-hwmon@vger.kernel.org; naveenkrishna.ch@gmail.com; stable@vger.kernel.org
> Subject: Re: [PATCH] hwmon: amd_energy: modify the visibility of the counters
> 
> [CAUTION: External Email]
> 
> On Sun, Nov 22, 2020 at 06:56:24AM +0000, Chatradhi, Naveen Krishna wrote:
> > [AMD Official Use Only - Approved for External Use]
> >
> > Hi Guenter, Salvatore
> >
> > > 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.
> >
> [ ... ]
> >
> > >> ?
> > 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

[PATCH] hwmon: amd_energy: modify the visibility of the counters

[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: stable@vger.kernel.org
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Naveen Krishna Chatradhi <nchatrad@amd.com>
---
 drivers/hwmon/amd_energy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/amd_energy.c b/drivers/hwmon/amd_energy.c
index d06597303d5a..3197cda7bcd9 100644
--- a/drivers/hwmon/amd_energy.c
+++ b/drivers/hwmon/amd_energy.c
@@ -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)
-- 
2.17.1