From: Kan Liang <[email protected]>
An oops will be triggered, if perf tries to access an invalid address
which exceeds the mapped area.
Check the address before the actual access to MMIO sapce of an uncore
unit.
Suggested-by: David Laight <[email protected]>
Signed-off-by: Kan Liang <[email protected]>
---
arch/x86/events/intel/uncore.c | 3 +++
arch/x86/events/intel/uncore.h | 12 ++++++++++++
arch/x86/events/intel/uncore_snbep.c | 6 ++++++
3 files changed, 21 insertions(+)
diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
index cf76d66..284f8e7 100644
--- a/arch/x86/events/intel/uncore.c
+++ b/arch/x86/events/intel/uncore.c
@@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
if (!box->io_addr)
return 0;
+ if (!is_valid_mmio_offset(box, event->hw.event_base))
+ return 0;
+
return readq(box->io_addr + event->hw.event_base);
}
diff --git a/arch/x86/events/intel/uncore.h b/arch/x86/events/intel/uncore.h
index c2e5725..672e9e1 100644
--- a/arch/x86/events/intel/uncore.h
+++ b/arch/x86/events/intel/uncore.h
@@ -197,6 +197,18 @@ static inline bool uncore_pmc_freerunning(int idx)
return idx == UNCORE_PMC_IDX_FREERUNNING;
}
+static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
+ unsigned long offset)
+{
+ if (box->pmu->type->mmio_map_size > offset)
+ return true;
+
+ pr_warn_once("perf uncore: Access invalid address of %s.\n",
+ box->pmu->type->name);
+
+ return false;
+}
+
static inline
unsigned int uncore_mmio_box_ctl(struct intel_uncore_box *box)
{
diff --git a/arch/x86/events/intel/uncore_snbep.c b/arch/x86/events/intel/uncore_snbep.c
index 801b662..9c7a641 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -4487,6 +4487,9 @@ static void snr_uncore_mmio_enable_event(struct intel_uncore_box *box,
if (!box->io_addr)
return;
+ if (!is_valid_mmio_offset(box, hwc->config_base))
+ return;
+
writel(hwc->config | SNBEP_PMON_CTL_EN,
box->io_addr + hwc->config_base);
}
@@ -4499,6 +4502,9 @@ static void snr_uncore_mmio_disable_event(struct intel_uncore_box *box,
if (!box->io_addr)
return;
+ if (!is_valid_mmio_offset(box, hwc->config_base))
+ return;
+
writel(hwc->config, box->io_addr + hwc->config_base);
}
--
2.7.4
On Thu, May 28, 2020 at 06:15:27AM -0700, [email protected] wrote:
> From: Kan Liang <[email protected]>
>
> An oops will be triggered, if perf tries to access an invalid address
> which exceeds the mapped area.
>
> Check the address before the actual access to MMIO sapce of an uncore
> unit.
Ah ok the range check is here
>
> Suggested-by: David Laight <[email protected]>
> Signed-off-by: Kan Liang <[email protected]>
> ---
> arch/x86/events/intel/uncore.c | 3 +++
> arch/x86/events/intel/uncore.h | 12 ++++++++++++
> arch/x86/events/intel/uncore_snbep.c | 6 ++++++
> 3 files changed, 21 insertions(+)
>
> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
> index cf76d66..284f8e7 100644
> --- a/arch/x86/events/intel/uncore.c
> +++ b/arch/x86/events/intel/uncore.c
> @@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
> if (!box->io_addr)
> return 0;
>
> + if (!is_valid_mmio_offset(box, event->hw.event_base))
> + return 0;
Is this function used somewhere else? Otherwise it should be added
together with its users.
-Andi
From: [email protected]
> Sent: 28 May 2020 14:15
...
> +static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
> + unsigned long offset)
You need a better name, needs to start 'uncore_' and 'mmio'
probably isn't right either.
> +{
> + if (box->pmu->type->mmio_map_size > offset)
> + return true;
Swap over.
Conditionals always read best if 'variable op constant'.
So you want:
if (offset < box->pmu->type->mmio_map_size)
return true;
> +
> + pr_warn_once("perf uncore: Access invalid address of %s.\n",
> + box->pmu->type->name);
Pretty hard to debug without the invalid offset.
I've no idea what the code is supposed to be doing though.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On 5/28/2020 9:30 AM, Andi Kleen wrote:
> On Thu, May 28, 2020 at 06:15:27AM -0700, [email protected] wrote:
>> From: Kan Liang <[email protected]>
>>
>> An oops will be triggered, if perf tries to access an invalid address
>> which exceeds the mapped area.
>>
>> Check the address before the actual access to MMIO sapce of an uncore
>> unit.
>
> Ah ok the range check is here
>
>>
>> Suggested-by: David Laight <[email protected]>
>> Signed-off-by: Kan Liang <[email protected]>
>> ---
>> arch/x86/events/intel/uncore.c | 3 +++
>> arch/x86/events/intel/uncore.h | 12 ++++++++++++
>> arch/x86/events/intel/uncore_snbep.c | 6 ++++++
>> 3 files changed, 21 insertions(+)
>>
>> diff --git a/arch/x86/events/intel/uncore.c b/arch/x86/events/intel/uncore.c
>> index cf76d66..284f8e7 100644
>> --- a/arch/x86/events/intel/uncore.c
>> +++ b/arch/x86/events/intel/uncore.c
>> @@ -132,6 +132,9 @@ u64 uncore_mmio_read_counter(struct intel_uncore_box *box,
>> if (!box->io_addr)
>> return 0;
>>
>> + if (!is_valid_mmio_offset(box, event->hw.event_base))
>> + return 0;
>
> Is this function used somewhere else? Otherwise it should be added
> together with its users.
>
Yes, it's generic function. Current MMIO uncore units invoke it to read
counter.
Thanks,
Kan
On 5/28/2020 9:33 AM, David Laight wrote:
> From: [email protected]
>> Sent: 28 May 2020 14:15
> ...
>> +static inline bool is_valid_mmio_offset(struct intel_uncore_box *box,
>> + unsigned long offset)
>
> You need a better name, needs to start 'uncore_' and 'mmio'
> probably isn't right either.
>
Sure
>> +{
>> + if (box->pmu->type->mmio_map_size > offset)
>> + return true;
>
> Swap over.
> Conditionals always read best if 'variable op constant'.
> So you want:
> if (offset < box->pmu->type->mmio_map_size)
> return true;
>
Sure
>> +
>> + pr_warn_once("perf uncore: Access invalid address of %s.\n",
>> + box->pmu->type->name);
>
> Pretty hard to debug without the invalid offset.
>
I will dump the box->io_addr and offset for debugging.
Thanks,
Kan
> I've no idea what the code is supposed to be doing though.
>
> David
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
> > > +
> > > + pr_warn_once("perf uncore: Access invalid address of %s.\n",
> > > + box->pmu->type->name);
> >
> > Pretty hard to debug without the invalid offset.
> >
>
> I will dump the box->io_addr and offset for debugging.
Please don't overengineer.
-Andi
On 5/28/2020 10:02 AM, Andi Kleen wrote:
>>>> +
>>>> + pr_warn_once("perf uncore: Access invalid address of %s.\n",
>>>> + box->pmu->type->name);
>>>
>>> Pretty hard to debug without the invalid offset.
>>>
>>
>> I will dump the box->io_addr and offset for debugging.
>
> Please don't overengineer.
>
OK. Will only dump the invalid offset.
Thanks,
Kan