The LBR 'from' adddress is under full userspace control; ensure we
validate it before reading from it.
Note: is_module_text_address() can potentially be quite expensive;
for those running into that either stop using modules as
all sane people do or optimize it using an RCU backed rb-tree.
Reported-by: Andi Kleen <[email protected]>
Cc: [email protected]
Signed-off-by: Peter Zijlstra <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
---
arch/x86/kernel/cpu/perf_event_intel_lbr.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -442,8 +442,18 @@ static int branch_type(unsigned long fro
return X86_BR_NONE;
addr = buf;
- } else
- addr = (void *)from;
+ } else {
+ /*
+ * The LBR logs any address in the IP, even if the IP just
+ * faulted. This means userspace can control the from address.
+ * Ensure we don't blindy read any address by validating it is
+ * a known text address.
+ */
+ if (kernel_text_address(from))
+ addr = (void *)from;
+ else
+ return X86_BR_NONE;
+ }
/*
* decoder needs to know the ABI especially
> + } else {
> + /*
> + * The LBR logs any address in the IP, even if the IP just
> + * faulted. This means userspace can control the from address.
> + * Ensure we don't blindy read any address by validating it is
> + * a known text address.
> + */
> + if (kernel_text_address(from))
Sorry doing it this way is just incredible expensive and dumb.
After all the tool is called "perf" and not "non perf"
-Andi
* Andi Kleen <[email protected]> wrote:
> > + } else {
> > + /*
> > + * The LBR logs any address in the IP, even if the IP just
> > + * faulted. This means userspace can control the from address.
> > + * Ensure we don't blindy read any address by validating it is
> > + * a known text address.
> > + */
> > + if (kernel_text_address(from))
>
> Sorry doing it this way is just incredible expensive and dumb.
If anyone using this feature notices the __module_address() overhead then
a 'module addresses RCU rbtree' could be added, which should solve the
overhead impact.
In any case Peter's patch fixes the bug without regressing the feature as
it is implemented today. Do you have a better solution that does not break
the ABI? The solution you proposed before regresses existing
functionality.
Thanks,
Ingo
Commit-ID: 6e15eb3ba6c0249c9e8c783517d131b47db995ca
Gitweb: http://git.kernel.org/tip/6e15eb3ba6c0249c9e8c783517d131b47db995ca
Author: Peter Zijlstra <[email protected]>
AuthorDate: Fri, 3 May 2013 14:11:24 +0200
Committer: Ingo Molnar <[email protected]>
CommitDate: Sat, 4 May 2013 08:37:47 +0200
perf/x86/intel/lbr: Fix LBR filter
The LBR 'from' adddress is under full userspace control; ensure
we validate it before reading from it.
Note: is_module_text_address() can potentially be quite
expensive; for those running into that with high overhead
in modules optimize it using an RCU backed rb-tree.
Reported-by: Andi Kleen <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: <[email protected]>
Cc: [email protected]
Link: http://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
Link: http://lkml.kernel.org/n/[email protected]
---
arch/x86/kernel/cpu/perf_event_intel_lbr.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/perf_event_intel_lbr.c b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
index da02e9c..de341d4 100644
--- a/arch/x86/kernel/cpu/perf_event_intel_lbr.c
+++ b/arch/x86/kernel/cpu/perf_event_intel_lbr.c
@@ -442,8 +442,18 @@ static int branch_type(unsigned long from, unsigned long to)
return X86_BR_NONE;
addr = buf;
- } else
- addr = (void *)from;
+ } else {
+ /*
+ * The LBR logs any address in the IP, even if the IP just
+ * faulted. This means userspace can control the from address.
+ * Ensure we don't blindy read any address by validating it is
+ * a known text address.
+ */
+ if (kernel_text_address(from))
+ addr = (void *)from;
+ else
+ return X86_BR_NONE;
+ }
/*
* decoder needs to know the ABI especially