[PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[Evgeny Novikov]

It looks like pmc_core_get_low_power_modes() mixes up modes and
priorities. In addition to invalid behavior, potentially this can
cause buffer overflows since the driver reads priorities from the
register and then it uses them as indexes for array lpm_priority
that can contain 8 elements at most. The patch swaps modes and
priorities.

Found by Linux Driver Verification project (linuxtesting.org).

Fixes: 005125bfd70e ("platform/x86: intel_pmc_core: Handle sub-states generically")
Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
---
 drivers/platform/x86/intel_pmc_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index b0e486a6bdfb..667b3df03764 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1469,8 +1469,8 @@ static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
 		int pri0 = GENMASK(3, 0) & priority;
 		int pri1 = (GENMASK(7, 4) & priority) >> 4;
 
-		lpm_priority[pri0] = mode;
-		lpm_priority[pri1] = mode + 1;
+		lpm_priority[mode] = pri0;
+		lpm_priority[mode + 1] = pri1;
 	}
 
 	/*
-- 
2.26.2

Re: [PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[Andy Shevchenko]

On Tue, Aug 3, 2021 at 9:21 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>
> It looks like pmc_core_get_low_power_modes() mixes up modes and
> priorities. In addition to invalid behavior, potentially this can
> cause buffer overflows since the driver reads priorities from the
> register and then it uses them as indexes for array lpm_priority
> that can contain 8 elements at most. The patch swaps modes and
> priorities.
>
> Found by Linux Driver Verification project (linuxtesting.org).

Seems legit.
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: 005125bfd70e ("platform/x86: intel_pmc_core: Handle sub-states generically")
> Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b0e486a6bdfb..667b3df03764 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1469,8 +1469,8 @@ static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
>                 int pri0 = GENMASK(3, 0) & priority;
>                 int pri1 = (GENMASK(7, 4) & priority) >> 4;
>
> -               lpm_priority[pri0] = mode;
> -               lpm_priority[pri1] = mode + 1;
> +               lpm_priority[mode] = pri0;

I would write it as + 0, but up to you and maintainers.

> +               lpm_priority[mode + 1] = pri1;
>         }
>
>         /*
> --
> 2.26.2
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[Andy Shevchenko]

On Tue, Aug 3, 2021 at 9:26 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, Aug 3, 2021 at 9:21 PM Evgeny Novikov <novikov@ispras.ru> wrote:
> >
> > It looks like pmc_core_get_low_power_modes() mixes up modes and
> > priorities. In addition to invalid behavior, potentially this can
> > cause buffer overflows since the driver reads priorities from the
> > register and then it uses them as indexes for array lpm_priority
> > that can contain 8 elements at most. The patch swaps modes and
> > priorities.
> >
> > Found by Linux Driver Verification project (linuxtesting.org).
>
> Seems legit.

Hold on, but then it follows with another loop where actually it reads
modes by priority index. Can you elaborate what exactly is the problem
you think?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[Evgeny Novikov]

On 03.08.2021 21:30, Andy Shevchenko wrote:
> On Tue, Aug 3, 2021 at 9:26 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, Aug 3, 2021 at 9:21 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>>> It looks like pmc_core_get_low_power_modes() mixes up modes and
>>> priorities. In addition to invalid behavior, potentially this can
>>> cause buffer overflows since the driver reads priorities from the
>>> register and then it uses them as indexes for array lpm_priority
>>> that can contain 8 elements at most. The patch swaps modes and
>>> priorities.
>>>
>>> Found by Linux Driver Verification project (linuxtesting.org).
>> Seems legit.
> Hold on, but then it follows with another loop where actually it reads
> modes by priority index. Can you elaborate what exactly is the problem
> you think?
>
I agree with you and David that my fix was not valid from the functional

point of view. Indeed, some issues can happen if something unexpected

will be read from the register. For instance, for priority equals to 255 you

will have pri0 = 15 and prio1 = 15. Obviously, you can not access the

lpm_priority array consisting of just 8 elements by these indexes.


Best regards,

Evgeny Novikov

Re: [PATCH] platform/x86: intel_pmc_core: Fix potential buffer overflows

[David E. Box]

Hi,

On Tue, 2021-08-03 at 21:11 +0300, Evgeny Novikov wrote:
> It looks like pmc_core_get_low_power_modes() mixes up modes and
> priorities. In addition to invalid behavior, potentially this can
> cause buffer overflows since the driver reads priorities from the
> register and then it uses them as indexes for array lpm_priority
> that can contain 8 elements at most. The patch swaps modes and
> priorities.
> 
> Found by Linux Driver Verification project (linuxtesting.org).
> 
> Fixes: 005125bfd70e ("platform/x86: intel_pmc_core: Handle sub-states
> generically")
> Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> ---
>  drivers/platform/x86/intel_pmc_core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c
> b/drivers/platform/x86/intel_pmc_core.c
> index b0e486a6bdfb..667b3df03764 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1469,8 +1469,8 @@ static void pmc_core_get_low_power_modes(struct
> pmc_dev *pmcdev)
>                 int pri0 = GENMASK(3, 0) & priority;
>                 int pri1 = (GENMASK(7, 4) & priority) >> 4;
>  
> -               lpm_priority[pri0] = mode;
> -               lpm_priority[pri1] = mode + 1;

Agree with the buffer overflow concern if hardware were to return an
incorrect value. But the assignment and indexing are correct. The list
was made to get the modes in priority order which is the order of
states the hardware will attempt to use if able.

I'll submit a patch for the overflow.

David


> +               lpm_priority[mode] = pri0;
> +               lpm_priority[mode + 1] = pri1;
>         }
>  
>         /*

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

[David E. Box]

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

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

[Evgeny Novikov]

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

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

[David E. Box]

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

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