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

Commit Message

David E. Box Aug. 14, 2021, 1:47 a.m. UTC

Substate priority levels are encoded in 4 bits in the LPM_PRI register.
This value was 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.  In addition to the overflow, bad values could lead
to incorrect state reporting.  So rework the priority code to prevent the
overflow and perform some validation of the register. Use the priority
register values if they give an ordering of unique numbers between 0 and
the maximum number of states.  Otherwise, use a default ordering instead.

Reported-by: Evgeny Novikov <[e-mail]>
Signed-off-by: David E. Box <[e-mail]>
---
v3:	Modifying Andy's suggestion, just place the entire verification
	in a separate function. If it fails, then keep the default
	ordering. If it passes, overwrite with the verified ordering.

	Fix error in default order array.

	Also fix spelling noted by Andy drop the size comment since
	the array size is set when declared.

v2:	Remove lpm_priority size increase. Instead, remove that array and
	create 2 new local arrays, one to save priority levels in mode order,
	and one to save modes in priority order. Use the mode_order list to
	validate that no priority level is above the maximum and to validate
	that they are all unique values. Then we can safely create a
	priority_order list that will be the basis of how we report substate
	information.

 drivers/platform/x86/intel_pmc_core.c | 65 +++++++++++++++++++++------
 drivers/platform/x86/intel_pmc_core.h |  2 +
 2 files changed, 53 insertions(+), 14 deletions(-)


base-commit: e4ec7a49ef8bb4edc85a0eee005d59fa65c94a0e

Comments

Andy Shevchenko Aug. 14, 2021, 9:50 a.m. UTC | #1

On Sat, Aug 14, 2021 at 4:49 AM David E. Box
<[e-mail]> wrote:
>
> Substate priority levels are encoded in 4 bits in the LPM_PRI register.
> This value was 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.  In addition to the overflow, bad values could lead
> to incorrect state reporting.  So rework the priority code to prevent the
> overflow and perform some validation of the register. Use the priority
> register values if they give an ordering of unique numbers between 0 and
> the maximum number of states.  Otherwise, use a default ordering instead.

Thanks! Looks much better now. A couple of minor comments below. With
or without them being addressed
Reviewed-by: Andy Shevchenko <[e-mail]>

> Reported-by: Evgeny Novikov <[e-mail]>
> Signed-off-by: David E. Box <[e-mail]>
> ---
> v3:     Modifying Andy's suggestion, just place the entire verification
>         in a separate function. If it fails, then keep the default
>         ordering. If it passes, overwrite with the verified ordering.
>
>         Fix error in default order array.
>
>         Also fix spelling noted by Andy drop the size comment since
>         the array size is set when declared.
>
> v2:     Remove lpm_priority size increase. Instead, remove that array and
>         create 2 new local arrays, one to save priority levels in mode order,
>         and one to save modes in priority order. Use the mode_order list to
>         validate that no priority level is above the maximum and to validate
>         that they are all unique values. Then we can safely create a
>         priority_order list that will be the basis of how we report substate
>         information.
>
>  drivers/platform/x86/intel_pmc_core.c | 65 +++++++++++++++++++++------
>  drivers/platform/x86/intel_pmc_core.h |  2 +
>  2 files changed, 53 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b0e486a6bdfb..ae410a358ffe 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1449,9 +1449,42 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
>
> -static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> +static bool pmc_core_pri_verify(u32 lpm_pri, u8 *mode_order)
>  {
> -       u8 lpm_priority[LPM_MAX_NUM_MODES];
> +       int i, j;
> +
> +       if (!lpm_pri)
> +               return false;
> +       /*
> +        * Each byte contains the priority level for 2 modes (7:4 and 3:0).
> +        * In a 32 bit register this allows for describing 8 modes. Store the
> +        * levels and look for values out of range.
> +        */
> +       for (i = 0; i < 8; i++) {
> +               int level = lpm_pri & GENMASK(3, 0);
> +
> +               if (level >= LPM_MAX_NUM_MODES)
> +                       return false;
> +
> +               mode_order[i] = level;
> +               lpm_pri >>= 4;
> +       }

> +       /* Check that we have unique values */
> +       for (i = 0; i < LPM_MAX_NUM_MODES - 1; i++)
> +               for (j = i + 1; j < LPM_MAX_NUM_MODES; j++)
> +                       if (mode_order[i] == mode_order[j])
> +                               return false;

There are ways to optimize this from O(n^2) to O(n), but it's not critical here.

> +       return true;
> +}
> +
> +static void pmc_core_get_low_power_modes(struct platform_device *pdev)
> +{
> +       struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +       u8 pri_order[LPM_MAX_NUM_MODES] = LPM_DEFAULT_PRI;
> +       u8 mode_order[LPM_MAX_NUM_MODES];
> +       u32 lpm_pri;
>         u32 lpm_en;
>         int mode, i, p;
>
> @@ -1462,24 +1495,28 @@ static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
>         lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
>         pmcdev->num_lpm_modes = hweight32(lpm_en);
>
> -       /* Each byte contains information for 2 modes (7:4 and 3:0) */
> -       for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
> -               u8 priority = pmc_core_reg_read_byte(pmcdev,
> -                               pmcdev->map->lpm_priority_offset + (mode / 2));
> -               int pri0 = GENMASK(3, 0) & priority;
> -               int pri1 = (GENMASK(7, 4) & priority) >> 4;
> +       /* Read 32 bit LPM_PRI register */
> +       lpm_pri = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_priority_offset);
>
> -               lpm_priority[pri0] = mode;
> -               lpm_priority[pri1] = mode + 1;
> -       }
>
>         /*
> -        * Loop though all modes from lowest to highest priority,
> +        * If lpm_pri value passes verification, then override the default
> +        * modes here. Otherwise stick with the default.
> +        */
> +       if (pmc_core_pri_verify(lpm_pri, mode_order))
> +               /* Get list of modes in priority order */
> +               for (mode = 0; mode < LPM_MAX_NUM_MODES; mode++)
> +                       pri_order[mode_order[mode]] = mode;
> +       else
> +               dev_warn(&pdev->dev, "Assuming a default substate order for this platform\n");
> +
> +       /*
> +        * Loop through all modes from lowest to highest priority,
>          * and capture all enabled modes in order
>          */
>         i = 0;
>         for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
> -               int mode = lpm_priority[p];
> +               int mode = pri_order[p];
>
>                 if (!(BIT(mode) & lpm_en))
>                         continue;
> @@ -1675,7 +1712,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>         mutex_init(&pmcdev->lock);
>
>         pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> -       pmc_core_get_low_power_modes(pmcdev);
> +       pmc_core_get_low_power_modes(pdev);
>         pmc_core_do_dmi_quirks(pmcdev);
>
>         if (pmcdev->map == &tgl_reg_map)
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index e8dae9c6c45f..b9bf3d3d6f7a 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -188,6 +188,8 @@ enum ppfear_regs {
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP                0x64
>
>  #define LPM_MAX_NUM_MODES                      8
> +#define LPM_DEFAULT_PRI                                { 7, 6, 2, 5, 4, 1, 3, 0 }

What I meant here is to add static_assert() to avoid ARRAY_SIZE() != NUM_MODES.

>  #define GET_X2_COUNTER(v)                      ((v) >> 1)
>  #define LPM_STS_LATCH_MODE                     BIT(31)

Hans de Goede Aug. 17, 2021, 6:56 p.m. UTC | #2

Hi,

On 8/14/21 3:47 AM, David E. Box wrote:
> Substate priority levels are encoded in 4 bits in the LPM_PRI register.
> This value was 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.  In addition to the overflow, bad values could lead
> to incorrect state reporting.  So rework the priority code to prevent the
> overflow and perform some validation of the register. Use the priority
> register values if they give an ordering of unique numbers between 0 and
> the maximum number of states.  Otherwise, use a default ordering instead.
> 
> Reported-by: Evgeny Novikov <[e-mail]>
> Signed-off-by: David E. Box <[e-mail]>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
> v3:	Modifying Andy's suggestion, just place the entire verification
> 	in a separate function. If it fails, then keep the default
> 	ordering. If it passes, overwrite with the verified ordering.
> 
> 	Fix error in default order array.
> 
> 	Also fix spelling noted by Andy drop the size comment since
> 	the array size is set when declared.
> 
> v2:	Remove lpm_priority size increase. Instead, remove that array and
> 	create 2 new local arrays, one to save priority levels in mode order,
> 	and one to save modes in priority order. Use the mode_order list to
> 	validate that no priority level is above the maximum and to validate
> 	that they are all unique values. Then we can safely create a
> 	priority_order list that will be the basis of how we report substate
> 	information.
> 
>  drivers/platform/x86/intel_pmc_core.c | 65 +++++++++++++++++++++------
>  drivers/platform/x86/intel_pmc_core.h |  2 +
>  2 files changed, 53 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
> index b0e486a6bdfb..ae410a358ffe 100644
> --- a/drivers/platform/x86/intel_pmc_core.c
> +++ b/drivers/platform/x86/intel_pmc_core.c
> @@ -1449,9 +1449,42 @@ static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
>  }
>  DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
>  
> -static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
> +static bool pmc_core_pri_verify(u32 lpm_pri, u8 *mode_order)
>  {
> -	u8 lpm_priority[LPM_MAX_NUM_MODES];
> +	int i, j;
> +
> +	if (!lpm_pri)
> +		return false;
> +	/*
> +	 * Each byte contains the priority level for 2 modes (7:4 and 3:0).
> +	 * In a 32 bit register this allows for describing 8 modes. Store the
> +	 * levels and look for values out of range.
> +	 */
> +	for (i = 0; i < 8; i++) {
> +		int level = lpm_pri & GENMASK(3, 0);
> +
> +		if (level >= LPM_MAX_NUM_MODES)
> +			return false;
> +
> +		mode_order[i] = level;
> +		lpm_pri >>= 4;
> +	}
> +
> +	/* Check that we have unique values */
> +	for (i = 0; i < LPM_MAX_NUM_MODES - 1; i++)
> +		for (j = i + 1; j < LPM_MAX_NUM_MODES; j++)
> +			if (mode_order[i] == mode_order[j])
> +				return false;
> +
> +	return true;
> +}
> +
> +static void pmc_core_get_low_power_modes(struct platform_device *pdev)
> +{
> +	struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
> +	u8 pri_order[LPM_MAX_NUM_MODES] = LPM_DEFAULT_PRI;
> +	u8 mode_order[LPM_MAX_NUM_MODES];
> +	u32 lpm_pri;
>  	u32 lpm_en;
>  	int mode, i, p;
>  
> @@ -1462,24 +1495,28 @@ static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
>  	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
>  	pmcdev->num_lpm_modes = hweight32(lpm_en);
>  
> -	/* Each byte contains information for 2 modes (7:4 and 3:0) */
> -	for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
> -		u8 priority = pmc_core_reg_read_byte(pmcdev,
> -				pmcdev->map->lpm_priority_offset + (mode / 2));
> -		int pri0 = GENMASK(3, 0) & priority;
> -		int pri1 = (GENMASK(7, 4) & priority) >> 4;
> +	/* Read 32 bit LPM_PRI register */
> +	lpm_pri = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_priority_offset);
>  
> -		lpm_priority[pri0] = mode;
> -		lpm_priority[pri1] = mode + 1;
> -	}
>  
>  	/*
> -	 * Loop though all modes from lowest to highest priority,
> +	 * If lpm_pri value passes verification, then override the default
> +	 * modes here. Otherwise stick with the default.
> +	 */
> +	if (pmc_core_pri_verify(lpm_pri, mode_order))
> +		/* Get list of modes in priority order */
> +		for (mode = 0; mode < LPM_MAX_NUM_MODES; mode++)
> +			pri_order[mode_order[mode]] = mode;
> +	else
> +		dev_warn(&pdev->dev, "Assuming a default substate order for this platform\n");
> +
> +	/*
> +	 * Loop through all modes from lowest to highest priority,
>  	 * and capture all enabled modes in order
>  	 */
>  	i = 0;
>  	for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
> -		int mode = lpm_priority[p];
> +		int mode = pri_order[p];
>  
>  		if (!(BIT(mode) & lpm_en))
>  			continue;
> @@ -1675,7 +1712,7 @@ static int pmc_core_probe(struct platform_device *pdev)
>  	mutex_init(&pmcdev->lock);
>  
>  	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
> -	pmc_core_get_low_power_modes(pmcdev);
> +	pmc_core_get_low_power_modes(pdev);
>  	pmc_core_do_dmi_quirks(pmcdev);
>  
>  	if (pmcdev->map == &tgl_reg_map)
> diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
> index e8dae9c6c45f..b9bf3d3d6f7a 100644
> --- a/drivers/platform/x86/intel_pmc_core.h
> +++ b/drivers/platform/x86/intel_pmc_core.h
> @@ -188,6 +188,8 @@ enum ppfear_regs {
>  #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
>  
>  #define LPM_MAX_NUM_MODES			8
> +#define LPM_DEFAULT_PRI				{ 7, 6, 2, 5, 4, 1, 3, 0 }
> +
>  #define GET_X2_COUNTER(v)			((v) >> 1)
>  #define LPM_STS_LATCH_MODE			BIT(31)
>  
> 
> base-commit: e4ec7a49ef8bb4edc85a0eee005d59fa65c94a0e
>

Randy Dunlap Aug. 19, 2021, 1:22 a.m. UTC | #3

On 8/13/21 6:47 PM, David E. Box wrote:
> Substate priority levels are encoded in 4 bits in the LPM_PRI register.
> This value was 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.  In addition to the overflow, bad values could lead
> to incorrect state reporting.  So rework the priority code to prevent the
> overflow and perform some validation of the register. Use the priority
> register values if they give an ordering of unique numbers between 0 and
> the maximum number of states.  Otherwise, use a default ordering instead.
> 
> Reported-by: Evgeny Novikov <[e-mail]>
> Signed-off-by: David E. Box <[e-mail]>
> ---
> v3:	Modifying Andy's suggestion, just place the entire verification
> 	in a separate function. If it fails, then keep the default
> 	ordering. If it passes, overwrite with the verified ordering.
> 
> 	Fix error in default order array.
> 
> 	Also fix spelling noted by Andy drop the size comment since
> 	the array size is set when declared.
> 
> v2:	Remove lpm_priority size increase. Instead, remove that array and
> 	create 2 new local arrays, one to save priority levels in mode order,
> 	and one to save modes in priority order. Use the mode_order list to
> 	validate that no priority level is above the maximum and to validate
> 	that they are all unique values. Then we can safely create a
> 	priority_order list that will be the basis of how we report substate
> 	information.
> 
>   drivers/platform/x86/intel_pmc_core.c | 65 +++++++++++++++++++++------
>   drivers/platform/x86/intel_pmc_core.h |  2 +
>   2 files changed, 53 insertions(+), 14 deletions(-)

Hi,
I was seeing this:

[    2.027295] ================================================================================
[    2.028593] UBSAN: shift-out-of-bounds in ../drivers/platform/x86/intel_pmc_core.c:1484:9
[    2.029683] shift exponent 255 is too large for 64-bit type 'long unsigned int'
[    2.030775] CPU: 11 PID: 312 Comm: systemd-udevd Tainted: G     U  W         5.14.0-rc6 #3 7cd0fa64f79977022e75f1a75abe17c80d128fc2
[    2.032485] Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./H470M-STX, BIOS P2.10 03/16/2021
[    2.034325] Call Trace:
[    2.040611]  dump_stack_lvl+0x38/0x49
[    2.042513]  dump_stack+0x10/0x12
[    2.044438]  ubsan_epilogue+0x9/0x80
[    2.048462]  __ubsan_handle_shift_out_of_bounds+0xfa/0x140
[    2.050430]  ? __ioremap_caller.constprop.18+0x1e9/0x380
[    2.054850]  pmc_core_probe+0x5cc/0x700 [intel_pmc_core 0d273a9f7ee2dddcef3fc0b98322787b4774a615]
[    2.055856] snd_hda_intel 0000:00:1f.3: azx_get_response timeout, switching to polling mode: last cmd=0x200f0000
[    2.056248]  ? pmc_core_probe+0x5cc/0x700 [intel_pmc_core 0d273a9f7ee2dddcef3fc0b98322787b4774a615]
[    2.059664]  ? __cond_resched+0x19/0x40
[    2.065564]  ? acpi_device_wakeup_disable+0x50/0x80
[    2.067391]  platform_probe+0x49/0x100
[    2.068684]  ? platform_probe+0x49/0x100
[    2.069942]  ? driver_sysfs_add+0x7a/0x100
[    2.071181]  really_probe+0x1f4/0x4c0
[    2.072413]  __driver_probe_device+0x11d/0x1c0
[    2.073642]  driver_probe_device+0x24/0xc0
[    2.074857]  __driver_attach+0xae/0x180
[    2.076055]  ? __device_attach_driver+0x180/0x180
[    2.077247]  ? __device_attach_driver+0x180/0x180
[    2.078454]  bus_for_each_dev+0x72/0xc0
[    2.079646]  driver_attach+0x1e/0x40
[    2.080824]  bus_add_driver+0x156/0x240
[    2.082011]  ? 0xffffffffc0119000
[    2.083184]  driver_register+0x60/0x100
[    2.084331]  ? 0xffffffffc0119000
[    2.085474]  __platform_driver_register+0x1e/0x40
[    2.086612]  pmc_core_driver_init+0x1c/0x1000 [intel_pmc_core 0d273a9f7ee2dddcef3fc0b98322787b4774a615]
[    2.087776]  do_one_initcall+0x43/0x200
[    2.088927]  ? kmem_cache_alloc_trace+0x4e/0x500
[    2.090078]  ? __vunmap+0x1c9/0x240
[    2.091223]  do_init_module+0x5f/0x235
[    2.092350]  load_module+0x29d0/0x2e80
[    2.093476]  ? kernel_read_file+0x2d2/0x300
[    2.094589]  __do_sys_finit_module+0xbe/0x140
[    2.095702]  ? __do_sys_finit_module+0xbe/0x140
[    2.096789]  __x64_sys_finit_module+0x1a/0x40
[    2.097880]  do_syscall_64+0x58/0x80
[    2.098971]  ? syscall_exit_to_user_mode+0x16/0x40
[    2.100064]  ? do_syscall_64+0x67/0x80
[    2.101140]  ? exit_to_user_mode_prepare+0x138/0x1c0
[    2.102213]  ? syscall_exit_to_user_mode+0x16/0x40
[    2.103278]  ? do_syscall_64+0x67/0x80
[    2.104343]  ? exc_page_fault+0x6d/0x140
[    2.105391]  ? asm_exc_page_fault+0x8/0x30
[    2.106429]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[    2.107470] RIP: 0033:0x7f1638f19569
[    2.108506] Code: 2d 00 b8 ca 00 00 00 0f 05 eb a5 66 0f 1f 44 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d f7 38 2d 00 f7 d8 64 89 01 48
[    2.109585] RSP: 002b:00007fff1d6c7758 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[    2.110677] RAX: ffffffffffffffda RBX: 000055d99d2f0780 RCX: 00007f1638f19569
[    2.111757] RDX: 0000000000000000 RSI: 00007f163987ff9d RDI: 0000000000000006
[    2.112841] RBP: 00007f163987ff9d R08: 0000000000000000 R09: 000055d99d0c1940
[    2.113916] R10: 0000000000000006 R11: 0000000000000246 R12: 0000000000020000
[    2.115347] R13: 000055d99d0c1d20 R14: 0000000000000000 R15: 000055d99d0c8610
[    2.116470] ================================================================================

and couldn't tell if this patch was supposed to fix that, so I tested it and I no longer
see the UBSAN report.

So Thanks and

Tested-by: Randy Dunlap <[e-mail]>

Patch

diff --git a/drivers/platform/x86/intel_pmc_core.c b/drivers/platform/x86/intel_pmc_core.c
index b0e486a6bdfb..ae410a358ffe 100644
--- a/drivers/platform/x86/intel_pmc_core.c
+++ b/drivers/platform/x86/intel_pmc_core.c
@@ -1449,9 +1449,42 @@  static int pmc_core_pkgc_show(struct seq_file *s, void *unused)
 }
 DEFINE_SHOW_ATTRIBUTE(pmc_core_pkgc);
 
-static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
+static bool pmc_core_pri_verify(u32 lpm_pri, u8 *mode_order)
 {
-	u8 lpm_priority[LPM_MAX_NUM_MODES];
+	int i, j;
+
+	if (!lpm_pri)
+		return false;
+	/*
+	 * Each byte contains the priority level for 2 modes (7:4 and 3:0).
+	 * In a 32 bit register this allows for describing 8 modes. Store the
+	 * levels and look for values out of range.
+	 */
+	for (i = 0; i < 8; i++) {
+		int level = lpm_pri & GENMASK(3, 0);
+
+		if (level >= LPM_MAX_NUM_MODES)
+			return false;
+
+		mode_order[i] = level;
+		lpm_pri >>= 4;
+	}
+
+	/* Check that we have unique values */
+	for (i = 0; i < LPM_MAX_NUM_MODES - 1; i++)
+		for (j = i + 1; j < LPM_MAX_NUM_MODES; j++)
+			if (mode_order[i] == mode_order[j])
+				return false;
+
+	return true;
+}
+
+static void pmc_core_get_low_power_modes(struct platform_device *pdev)
+{
+	struct pmc_dev *pmcdev = platform_get_drvdata(pdev);
+	u8 pri_order[LPM_MAX_NUM_MODES] = LPM_DEFAULT_PRI;
+	u8 mode_order[LPM_MAX_NUM_MODES];
+	u32 lpm_pri;
 	u32 lpm_en;
 	int mode, i, p;
 
@@ -1462,24 +1495,28 @@  static void pmc_core_get_low_power_modes(struct pmc_dev *pmcdev)
 	lpm_en = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_en_offset);
 	pmcdev->num_lpm_modes = hweight32(lpm_en);
 
-	/* Each byte contains information for 2 modes (7:4 and 3:0) */
-	for (mode = 0; mode < LPM_MAX_NUM_MODES; mode += 2) {
-		u8 priority = pmc_core_reg_read_byte(pmcdev,
-				pmcdev->map->lpm_priority_offset + (mode / 2));
-		int pri0 = GENMASK(3, 0) & priority;
-		int pri1 = (GENMASK(7, 4) & priority) >> 4;
+	/* Read 32 bit LPM_PRI register */
+	lpm_pri = pmc_core_reg_read(pmcdev, pmcdev->map->lpm_priority_offset);
 
-		lpm_priority[pri0] = mode;
-		lpm_priority[pri1] = mode + 1;
-	}
 
 	/*
-	 * Loop though all modes from lowest to highest priority,
+	 * If lpm_pri value passes verification, then override the default
+	 * modes here. Otherwise stick with the default.
+	 */
+	if (pmc_core_pri_verify(lpm_pri, mode_order))
+		/* Get list of modes in priority order */
+		for (mode = 0; mode < LPM_MAX_NUM_MODES; mode++)
+			pri_order[mode_order[mode]] = mode;
+	else
+		dev_warn(&pdev->dev, "Assuming a default substate order for this platform\n");
+
+	/*
+	 * Loop through all modes from lowest to highest priority,
 	 * and capture all enabled modes in order
 	 */
 	i = 0;
 	for (p = LPM_MAX_NUM_MODES - 1; p >= 0; p--) {
-		int mode = lpm_priority[p];
+		int mode = pri_order[p];
 
 		if (!(BIT(mode) & lpm_en))
 			continue;
@@ -1675,7 +1712,7 @@  static int pmc_core_probe(struct platform_device *pdev)
 	mutex_init(&pmcdev->lock);
 
 	pmcdev->pmc_xram_read_bit = pmc_core_check_read_lock_bit(pmcdev);
-	pmc_core_get_low_power_modes(pmcdev);
+	pmc_core_get_low_power_modes(pdev);
 	pmc_core_do_dmi_quirks(pmcdev);
 
 	if (pmcdev->map == &tgl_reg_map)
diff --git a/drivers/platform/x86/intel_pmc_core.h b/drivers/platform/x86/intel_pmc_core.h
index e8dae9c6c45f..b9bf3d3d6f7a 100644
--- a/drivers/platform/x86/intel_pmc_core.h
+++ b/drivers/platform/x86/intel_pmc_core.h
@@ -188,6 +188,8 @@  enum ppfear_regs {
 #define ICL_PMC_SLP_S0_RES_COUNTER_STEP		0x64
 
 #define LPM_MAX_NUM_MODES			8
+#define LPM_DEFAULT_PRI				{ 7, 6, 2, 5, 4, 1, 3, 0 }
+
 #define GET_X2_COUNTER(v)			((v) >> 1)
 #define LPM_STS_LATCH_MODE			BIT(31)