Re: UBSAN: Undefined behaviour in arch/x86/events/intel/p6.c:116:29

[Peter Zijlstra]

On Tue, Nov 26, 2019 at 07:55:08PM +0200, Meelis Roos wrote:
> While testing 5.4 on a Dell D600 (32-bit), I noticed the old UBSAN warnings from p6 perf events.
> I remember having seen these warnings on other p6 era computers too.
> 
> [    2.795167] ================================================================================
> [    2.795206] UBSAN: Undefined behaviour in arch/x86/events/intel/p6.c:116:29
> [    2.795235] index 8 is out of range for type 'u64 [8]'
> [    2.795265] CPU: 0 PID: 1 Comm: swapper Not tainted 5.4.0-03419-g386403a115f9-dirty #18
> [    2.795266] Hardware name: Dell Computer Corporation Latitude D600                   /0X2034, BIOS A16 06/29/2005
> [    2.795268] Call Trace:
> [    2.795283]  dump_stack+0x16/0x19
> [    2.795290]  ubsan_epilogue+0xb/0x29
> [    2.795293]  __ubsan_handle_out_of_bounds.cold+0x43/0x48
> [    2.795299]  ? sysfs_add_file_mode_ns+0xad/0x180
> [    2.795304]  p6_pmu_event_map+0x3b/0x50
> [    2.795306]  is_visible+0x25/0x30
> [    2.795308]  ? collect_events+0x150/0x150
> [    2.795310]  internal_create_group+0xd8/0x3e0
> [    2.795312]  ? collect_events+0x150/0x150
> [    2.795314]  internal_create_groups.part.0+0x34/0x80
> [    2.795317]  sysfs_create_groups+0x10/0x20
> [    2.795321]  device_add+0x536/0x5a0
> [    2.795326]  ? kvasprintf_const+0x59/0x90
> [    2.795331]  ? kfree_const+0xf/0x30
> [    2.795334]  ? kobject_set_name_vargs+0x6a/0xa0
> [    2.795338]  pmu_dev_alloc+0x8e/0xe0
> [    2.795344]  perf_event_sysfs_init+0x40/0x78
> [    2.795346]  ? stack_map_init+0x17/0x17
> [    2.795347]  do_one_initcall+0x7a/0x1b3
> [    2.795351]  ? do_early_param+0x75/0x75
> [    2.795354]  kernel_init_freeable+0x1ae/0x230
> [    2.795357]  ? rest_init+0x6d/0x6d
> [    2.795359]  kernel_init+0x9/0xf3
> [    2.795361]  ? rest_init+0x6d/0x6d
> [    2.795363]  ret_from_fork+0x2e/0x38
> [    2.795364] ================================================================================

Does something like so fix it?

diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 9a89d98c55bd..f0ab61cd2f68 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1642,9 +1642,12 @@ static struct attribute_group x86_pmu_format_group __ro_after_init = {
 
 ssize_t events_sysfs_show(struct device *dev, struct device_attribute *attr, char *page)
 {
-	struct perf_pmu_events_attr *pmu_attr = \
+	struct perf_pmu_events_attr *pmu_attr =
 		container_of(attr, struct perf_pmu_events_attr, attr);
-	u64 config = x86_pmu.event_map(pmu_attr->id);
+	u64 config = 0;
+
+	if (pmu_attr->id < x86_pmu.max_events)
+		config = x86_pmu.event_map(pmu_attr->id);
 
 	/* string trumps id */
 	if (pmu_attr->event_str)