LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Randy Dunlap <rdunlap@infradead.org>
To: "David E. Box" <david.e.box@linux.intel.com>,
	irenic.rajneesh@gmail.com, novikov@ispras.ru,
	gayatri.kammela@intel.com, hdegoede@redhat.com,
	mgross@linux.intel.com, andy.shevchenko@gmail.com
Cc: platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow
Date: Wed, 18 Aug 2021 18:22:08 -0700	[thread overview]
Message-ID: <83282625-9a6f-0d05-90c9-22b67107537f@infradead.org> (raw)
In-Reply-To: <20210814014728.520856-1-david.e.box@linux.intel.com>

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


      parent reply	other threads:[~2021-08-19  1:22 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-14  1:47 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 [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=83282625-9a6f-0d05-90c9-22b67107537f@infradead.org \
    --to=rdunlap@infradead.org \
    --cc=andy.shevchenko@gmail.com \
    --cc=david.e.box@linux.intel.com \
    --cc=gayatri.kammela@intel.com \
    --cc=hdegoede@redhat.com \
    --cc=irenic.rajneesh@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgross@linux.intel.com \
    --cc=novikov@ispras.ru \
    --cc=platform-driver-x86@vger.kernel.org \
    --subject='Re: [PATCH v3] platform/x86: intel_pmc_core: Prevent possibile overflow' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).