From: Lai Jiangshan <[email protected]>
A data breakpoint on the GDT is terrifying and should be avoided.
The GDT on CPU entry area is already protected. The direct GDT
should be also protected, although it is seldom used and only
used for short time.
Signed-off-by: Lai Jiangshan <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Link: https://lkml.kernel.org/r/[email protected]
---
arch/x86/kernel/hw_breakpoint.c | 30 ++++++++++++++++++++++--------
1 file changed, 22 insertions(+), 8 deletions(-)
--- a/arch/x86/kernel/hw_breakpoint.c
+++ b/arch/x86/kernel/hw_breakpoint.c
@@ -32,6 +32,7 @@
#include <asm/processor.h>
#include <asm/debugreg.h>
#include <asm/user.h>
+#include <asm/desc.h>
/* Per cpu debug control register value */
DEFINE_PER_CPU(unsigned long, cpu_dr7);
@@ -237,13 +238,26 @@ static inline bool within_area(unsigned
}
/*
- * Checks whether the range from addr to end, inclusive, overlaps the CPU
- * entry area range.
+ * Checks whether the range from addr to end, inclusive, overlaps the fixed
+ * mapped CPU entry area range or other ranges used for CPU entry.
*/
-static inline bool within_cpu_entry_area(unsigned long addr, unsigned long end)
+static inline bool within_cpu_entry(unsigned long addr, unsigned long end)
{
- return within_area(addr, end, CPU_ENTRY_AREA_BASE,
- CPU_ENTRY_AREA_TOTAL_SIZE);
+ int cpu;
+
+ /* CPU entry erea is always used for CPU entry */
+ if (within_area(addr, end, CPU_ENTRY_AREA_BASE,
+ CPU_ENTRY_AREA_TOTAL_SIZE))
+ return true;
+
+ for_each_possible_cpu(cpu) {
+ /* The original rw GDT is being used after load_direct_gdt() */
+ if (within_area(addr, end, (unsigned long)get_cpu_gdt_rw(cpu),
+ GDT_SIZE))
+ return true;
+ }
+
+ return false;
}
static int arch_build_bp_info(struct perf_event *bp,
@@ -257,12 +271,12 @@ static int arch_build_bp_info(struct per
return -EINVAL;
/*
- * Prevent any breakpoint of any type that overlaps the
- * cpu_entry_area. This protects the IST stacks and also
+ * Prevent any breakpoint of any type that overlaps the CPU
+ * entry area and data. This protects the IST stacks and also
* reduces the chance that we ever find out what happens if
* there's a data breakpoint on the GDT, IDT, or TSS.
*/
- if (within_cpu_entry_area(attr->bp_addr, bp_end))
+ if (within_cpu_entry(attr->bp_addr, bp_end))
return -EINVAL;
hw->address = attr->bp_addr;
On 29/05/2020 22:27, Peter Zijlstra wrote:
> From: Lai Jiangshan <[email protected]>
>
> A data breakpoint on the GDT is terrifying and should be avoided.
> The GDT on CPU entry area is already protected. The direct GDT
> should be also protected, although it is seldom used and only
> used for short time.
While I agree with the sentiment...
>
> Signed-off-by: Lai Jiangshan <[email protected]>
> Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> Link: https://lkml.kernel.org/r/[email protected]
> ---
> arch/x86/kernel/hw_breakpoint.c | 30 ++++++++++++++++++++++--------
> 1 file changed, 22 insertions(+), 8 deletions(-)
>
> --- a/arch/x86/kernel/hw_breakpoint.c
> +++ b/arch/x86/kernel/hw_breakpoint.c
> @@ -32,6 +32,7 @@
> #include <asm/processor.h>
> #include <asm/debugreg.h>
> #include <asm/user.h>
> +#include <asm/desc.h>
>
> /* Per cpu debug control register value */
> DEFINE_PER_CPU(unsigned long, cpu_dr7);
> @@ -237,13 +238,26 @@ static inline bool within_area(unsigned
> }
>
> /*
> - * Checks whether the range from addr to end, inclusive, overlaps the CPU
> - * entry area range.
> + * Checks whether the range from addr to end, inclusive, overlaps the fixed
> + * mapped CPU entry area range or other ranges used for CPU entry.
> */
> -static inline bool within_cpu_entry_area(unsigned long addr, unsigned long end)
> +static inline bool within_cpu_entry(unsigned long addr, unsigned long end)
> {
> - return within_area(addr, end, CPU_ENTRY_AREA_BASE,
> - CPU_ENTRY_AREA_TOTAL_SIZE);
> + int cpu;
> +
> + /* CPU entry erea is always used for CPU entry */
> + if (within_area(addr, end, CPU_ENTRY_AREA_BASE,
> + CPU_ENTRY_AREA_TOTAL_SIZE))
> + return true;
> +
> + for_each_possible_cpu(cpu) {
> + /* The original rw GDT is being used after load_direct_gdt() */
> + if (within_area(addr, end, (unsigned long)get_cpu_gdt_rw(cpu),
> + GDT_SIZE))
... why the O(n) loop over the system?
It is only GDTs which might ever be active on this local CPU(/thread)
which are a problem, because the breakpoint registers are similarly local.
Nothing is going to go wrong If I put a breakpoint on someone else's
live GDT, because they wont interact in the "fun" ways we're trying to
avoid.
~Andrew
On Sat, May 30, 2020 at 8:48 PM Andrew Cooper <[email protected]> wrote:
>
> On 29/05/2020 22:27, Peter Zijlstra wrote:
> > From: Lai Jiangshan <[email protected]>
> >
> > A data breakpoint on the GDT is terrifying and should be avoided.
> > The GDT on CPU entry area is already protected. The direct GDT
> > should be also protected, although it is seldom used and only
> > used for short time.
>
> While I agree with the sentiment...
>
> >
> > Signed-off-by: Lai Jiangshan <[email protected]>
> > Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
> > Link: https://lkml.kernel.org/r/[email protected]
> > ---
> > arch/x86/kernel/hw_breakpoint.c | 30 ++++++++++++++++++++++--------
> > 1 file changed, 22 insertions(+), 8 deletions(-)
> >
> > --- a/arch/x86/kernel/hw_breakpoint.c
> > +++ b/arch/x86/kernel/hw_breakpoint.c
> > @@ -32,6 +32,7 @@
> > #include <asm/processor.h>
> > #include <asm/debugreg.h>
> > #include <asm/user.h>
> > +#include <asm/desc.h>
> >
> > /* Per cpu debug control register value */
> > DEFINE_PER_CPU(unsigned long, cpu_dr7);
> > @@ -237,13 +238,26 @@ static inline bool within_area(unsigned
> > }
> >
> > /*
> > - * Checks whether the range from addr to end, inclusive, overlaps the CPU
> > - * entry area range.
> > + * Checks whether the range from addr to end, inclusive, overlaps the fixed
> > + * mapped CPU entry area range or other ranges used for CPU entry.
> > */
> > -static inline bool within_cpu_entry_area(unsigned long addr, unsigned long end)
> > +static inline bool within_cpu_entry(unsigned long addr, unsigned long end)
> > {
> > - return within_area(addr, end, CPU_ENTRY_AREA_BASE,
> > - CPU_ENTRY_AREA_TOTAL_SIZE);
> > + int cpu;
> > +
> > + /* CPU entry erea is always used for CPU entry */
> > + if (within_area(addr, end, CPU_ENTRY_AREA_BASE,
> > + CPU_ENTRY_AREA_TOTAL_SIZE))
> > + return true;
> > +
> > + for_each_possible_cpu(cpu) {
> > + /* The original rw GDT is being used after load_direct_gdt() */
> > + if (within_area(addr, end, (unsigned long)get_cpu_gdt_rw(cpu),
> > + GDT_SIZE))
>
> ... why the O(n) loop over the system?
>
> It is only GDTs which might ever be active on this local CPU(/thread)
> which are a problem, because the breakpoint registers are similarly local.
>
> Nothing is going to go wrong If I put a breakpoint on someone else's
> live GDT, because they wont interact in the "fun" ways we're trying to
> avoid.
Hello
It can help to find the bugs that some cpus may access
to the the wrong GDTs as your suggestion and avoids the
O(nr_cpus) loop.
However, it needs to refactor the hw_breakpoint.c to some
extend:
Some breakpoints are allowed when they are being installed,
but they will be filtered out on some CPUs without causing
any confusion in the general hw_breakpoint.c and perf event
and the handlers need to know they may lost some events in
some cases.
But current code doesn't have such framework yet, we have to
block them directly IMHO.
Thanks
Lai