Re: [PATCH] platform/x86: intel_pmc_core: Prevent possibile overflow

[Evgeny Novikov]

Hi David,

On 05.08.2021 00:51, David E. Box wrote:
> Hi Evgeny,
>
> On Wed, 2021-08-04 at 13:48 +0300, Evgeny Novikov wrote:
> > Hi David,
> >
> > Your patch fixes the out of bound issue, but I have another concern
> > regarding possible incomplete initialization of first 8 elements of
> > the
> > lpm_priority array that is declared on the stack and is not
> > initialized,
> > say, with zeroes. Yet again due to some invalid values coming from
> > the
> > register, it is not guaranteed that something meaningful will be
> > assigned for all first 8 elements of lpm_priority in the first cycle
> > in
> > pmc_core_get_low_power_modes(). In the second cycle this function
> > accesses all these elements from lpm_priority. Though there is test
> > "!(BIT(mode) & lpm_en)", it can pass accidentally, thus some
> > unexpected
> > values can be stored to "pmcdev->lpm_en_modes[i++]" and exposed
> > later.
> I sent out a v2 that validates the priority levels are within bounds
> and meaningful before reordering them to set the lpm_en_modes. Thanks.

Now it looks that you fixed both issues. Our verification framework does 
not report warnings after application of the patch. I can not reason 
about functional correctness of this code since I am not familiar with 
the corresponding documentation and, thus, expected behavior.

Likely there is a small misprint in the comment "contains gives".

Best regards,
Evgeny Novikov

> David
>
> > Best regards,
> > Evgeny Novikov
> >
> >
> > On 04.08.2021 03:30, David E. Box wrote:
> > > Low Power Mode (LPM) priority is encoded in 4 bits. Yet, this value
> > > is used
> > > as an index to an array whose element size was less than 16,
> > > leading to the
> > > possibility of overflow should we read a larger than expected
> > > priority. Set
> > > the array size to 16 to prevent this.
> > >
> > > Reported-by: Evgeny Novikov <novikov@xxxxxxxxx>
> > > Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> > > ---
> > >    drivers/platform/x86/intel_pmc_core.c | 2 +-
> > >    drivers/platform/x86/intel_pmc_core.h | 1 +
> > >    2 files changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/platform/x86/intel_pmc_core.c
> > > b/drivers/platform/x86/intel_pmc_core.c
> > > index b0e486a6bdfb..2a761fe98277 100644
> > > --- a/drivers/platform/x86/intel_pmc_core.c
> > > +++ b/drivers/platform/x86/intel_pmc_core.c
> > > @@ -1451,7 +1451,7 @@ DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
> > >    
> > >    static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> > >    {
> > > -       u8 lpm_priority[LPM_MAX_NUM_MODES];
> > > +       u8 lpm_priority[LPM_MAX_PRI];
> > >          u32 lpm_en;
> > >          int mode, i, p;
> > >    
> > > diff --git a/drivers/platform/x86/intel_pmc_core.h
> > > b/drivers/platform/x86/intel_pmc_core.h
> > > index e8dae9c6c45f..b98c2b44c938 100644
> > > --- a/drivers/platform/x86/intel_pmc_core.h
> > > +++ b/drivers/platform/x86/intel_pmc_core.h
> > > @@ -190,6 +190,7 @@ enum ppfear_regs {
> > >    #define LPM_MAX_NUM_MODES                     8
> > >    #define GET_X2_COUNTER(v)                     ((v) >> 1)
> > >    #define LPM_STS_LATCH_MODE                    BIT(31)
> > > +#define LPM_MAX_PRI                            16      /* size of
> > > 4 bits */
> > >    
> > >    #define TGL_PMC_SLP_S0_RES_COUNTER_STEP               0x7A
> > >    #define TGL_PMC_LTR_THC0                      0x1C04