| Message ID | 20210804003039.359138-1-david.e.box@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | platform/x86: intel_pmc_core: Prevent possibile overflow | expand |
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. 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@ispras.ru> > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > --- > 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
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. 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@ispras.ru> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com> > > --- > > 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
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@ispras.ru> >>> Signed-off-by: David E. Box <david.e.box@linux.intel.com> >>> --- >>> 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 >
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
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@ispras.ru> Signed-off-by: David E. Box <david.e.box@linux.intel.com> --- drivers/platform/x86/intel_pmc_core.c | 2 +- drivers/platform/x86/intel_pmc_core.h | 1 + 2 files changed, 2 insertions(+), 1 deletion(-)