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

[Guenter Roeck <linux@roeck-us.net>]

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