* Re: [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow
2021-08-14 1:47 [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow David E. Box
@ 2021-08-14 9:50 ` Andy Shevchenko
2021-08-17 18:56 ` Hans de Goede
2021-08-19 1:22 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Andy Shevchenko @ 2021-08-14 9:50 UTC (permalink / raw)
To: David E. Box
Cc: Rajneesh Bhardwaj, Evgeny Novikov, Gayatri Kammela,
Hans de Goede, Mark Gross, Platform Driver,
Linux Kernel Mailing List
On Sat, Aug 14, 2021 at 4:49 AM David E. Box
<david.e.box@linux.intel.com> 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 <andy.shevchenko@gmail.com>
> Reported-by: Evgeny Novikov <novikov@ispras.ru>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 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)
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow
2021-08-14 1:47 [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow David E. Box
2021-08-14 9:50 ` Andy Shevchenko
@ 2021-08-17 18:56 ` Hans de Goede
2021-08-19 1:22 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Hans de Goede @ 2021-08-17 18:56 UTC (permalink / raw)
To: David E. Box, irenic.rajneesh, novikov, gayatri.kammela, mgross,
andy.shevchenko
Cc: platform-driver-x86, linux-kernel
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 <novikov@ispras.ru>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
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
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow
2021-08-14 1:47 [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow David E. Box
2021-08-14 9:50 ` Andy Shevchenko
2021-08-17 18:56 ` Hans de Goede
@ 2021-08-19 1:22 ` Randy Dunlap
2 siblings, 0 replies; 4+ messages in thread
From: Randy Dunlap @ 2021-08-19 1:22 UTC (permalink / raw)
To: David E. Box, irenic.rajneesh, novikov, gayatri.kammela,
hdegoede, mgross, andy.shevchenko
Cc: platform-driver-x86, linux-kernel
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 <novikov@ispras.ru>
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> ---
> 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 <rdunlap@infradead.org>
--
~Randy
^ permalink raw reply [flat|nested] 4+ messages in thread