Re: [GIT PULL] Performance events changes for v7.1

[Ravi Bangoria <ravi.bangoria@amd.com>]

>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -698,6 +698,8 @@
>>  #define MSR_AMD64_IBSBRTARGET		0xc001103b
>>  #define MSR_AMD64_ICIBSEXTDCTL		0xc001103c
>>  #define MSR_AMD64_IBSOPDATA4		0xc001103d
>> +#define MSR_AMD64_IBSOPCTL2		0xc001103e
>> +#define MSR_AMD64_IBSFETCHCTL2		0xc001103f
>>  #define MSR_AMD64_IBS_REG_COUNT_MAX	8 /* includes MSR_AMD64_IBSBRTARGET */
> 
> This isn't a regression from the current patch, but looking at the adjacent
> MSR handling code, is there a potential stack buffer overflow in the IBS
> NMI handler on CPUs supporting IBS_CAPS_OPDATA4?
> 
> In arch/x86/events/amd/ibs.c, struct perf_ibs_data is allocated on the
> stack in perf_ibs_handle_irq(). Its regs array is sized using
> MSR_AMD64_IBS_REG_COUNT_MAX, which is 8.
> 
> When handling a perf_ibs_op event, the initial read loop fetches up to 7
> MSRs, advancing the pointer buf to &ibs_data.regs[7].
> 
> Subsequently, the handler reads additional MSRs depending on CPU caps:
> 
> 	if (event->attr.sample_type & PERF_SAMPLE_RAW) {
> 		if (perf_ibs == &perf_ibs_op) {
> 			if (ibs_caps & IBS_CAPS_BRNTRGT) {
> 				rdmsrq(MSR_AMD64_IBSBRTARGET, *buf++);
> 				br_target_idx = size;
> 				size++;
> 			}
> 			if (ibs_caps & IBS_CAPS_OPDATA4) {
> 				rdmsrq(MSR_AMD64_IBSOPDATA4, *buf++);
> 				size++;
> 			}
> 		}
> 
> If the CPU supports IBS_CAPS_BRNTRGT, MSR_AMD64_IBSBRTARGET is read into
> regs[7], advancing buf to &regs[8].
> 
> If the CPU also supports IBS_CAPS_OPDATA4, it reads MSR_AMD64_IBSOPDATA4 into
> the next pointer position, which translates to regs[8]. This writes one
> element past the end of the 8-element array, overwriting adjacent local
> variables on the stack.
> 
> Furthermore, ibs_data.size is incremented to 9, so
> perf_sample_save_raw_data() will subsequently read 72 bytes of regs,
> exposing the corrupted stack data to user-space in the raw perf record. Since
> unprivileged users can configure RAW IBS events, this might lead to
> unintended behavior.
> 
> Should MSR_AMD64_IBS_REG_COUNT_MAX be increased to account for
> IBS_CAPS_OPDATA4?

Thanks for the bug report.

While this seems like a genuine overflow issue, IbsOpData4 has been
deprecated from long back. I was unable to find any platform that
supports it. So it's not a real a concern.

Thanks,
Ravi