LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Lai Jiangshan <jiangshanlai+lkml@gmail.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Andy Lutomirski <luto@amacapital.net>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Lai Jiangshan <laijs@linux.alibaba.com>,
	Sean Christopherson <sean.j.christopherson@intel.com>,
	Daniel Thompson <daniel.thompson@linaro.org>,
	a.darwish@linutronix.de, Steven Rostedt <rostedt@goodmis.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Subject: Re: [PATCH 02/14] x86/hw_breakpoint: Prevent data breakpoints on direct GDT
Date: Sat, 30 May 2020 23:15:17 +0800
Message-ID: <CAJhGHyC=ThjZEDsC48NiBWq8on089VstP57ocG5OTp3LYahykg@mail.gmail.com> (raw)
In-Reply-To: <582d9136-8f8b-fa07-862e-9ea5d440c09f@citrix.com>

On Sat, May 30, 2020 at 8:48 PM Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 29/05/2020 22:27, Peter Zijlstra wrote:
> > From: Lai Jiangshan <laijs@linux.alibaba.com>
> >
> > 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 <laijs@linux.alibaba.com>
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> > Link: https://lkml.kernel.org/r/20200526014221.2119-3-laijs@linux.alibaba.com
> > ---
> >  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

  reply index

Thread overview: 35+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-29 21:27 [PATCH 00/14] x86/entry: disallow #DB more and x86/entry lockdep/nmi Peter Zijlstra
2020-05-29 21:27 ` [PATCH 01/14] x86/hw_breakpoint: Add within_area() to check data breakpoints Peter Zijlstra
2020-05-29 21:27 ` [PATCH 02/14] x86/hw_breakpoint: Prevent data breakpoints on direct GDT Peter Zijlstra
2020-05-30 12:45   ` Andrew Cooper
2020-05-30 15:15     ` Lai Jiangshan [this message]
2020-05-29 21:27 ` [PATCH 03/14] x86/hw_breakpoint: Prevent data breakpoints on per_cpu cpu_tss_rw Peter Zijlstra
2020-05-29 21:27 ` [PATCH 04/14] x86/hw_breakpoint: Prevent data breakpoints on user_pcid_flush_mask Peter Zijlstra
2020-05-29 21:27 ` [PATCH 05/14] x86/entry: Introduce local_db_{save,restore}() Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 06/14] x86/entry, nmi: Disable #DB Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 07/14] x86/entry, mce: Disallow #DB during #MC Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 08/14] x86/entry: Optimize local_db_save() for virt Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-06-03  1:17   ` [PATCH 08/14] " Sean Christopherson
2020-05-29 21:27 ` [PATCH 09/14] x86/entry: Remove debug IDT frobbing Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 10/14] x86/entry: Remove DBn stacks Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 11/14] x86/entry: Clarify irq_{enter,exit}_rcu() Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-06-02 14:42   ` [PATCH 11/14] " Qian Cai
2020-06-02 15:05     ` Peter Zijlstra
2020-06-02 18:47       ` Qian Cai
2020-06-03 17:50       ` [tip: x86/entry] x86/entry: Use __irq_exit_rcu() in irq_exit() tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 12/14] x86/entry: Rename trace_hardirqs_off_prepare() Peter Zijlstra
2020-05-30  9:57   ` [tip: x86/entry] " tip-bot2 for Peter Zijlstra
2020-05-29 21:27 ` [PATCH 13/14] lockdep: Prepare for NMI IRQ state tracking Peter Zijlstra
2020-05-29 22:14   ` Steven Rostedt
2020-05-29 22:25     ` Peter Zijlstra
2020-05-29 22:28       ` Steven Rostedt
2020-05-29 22:33       ` Peter Zijlstra
2020-06-02 20:00       ` Peter Zijlstra
2020-05-29 21:27 ` [PATCH 14/14] x86/entry: Fix NMI vs " Peter Zijlstra

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAJhGHyC=ThjZEDsC48NiBWq8on089VstP57ocG5OTp3LYahykg@mail.gmail.com' \
    --to=jiangshanlai+lkml@gmail.com \
    --cc=a.darwish@linutronix.de \
    --cc=andrew.cooper3@citrix.com \
    --cc=bigeasy@linutronix.de \
    --cc=daniel.thompson@linaro.org \
    --cc=laijs@linux.alibaba.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@amacapital.net \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=sean.j.christopherson@intel.com \
    --cc=tglx@linutronix.de \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git
	git clone --mirror https://lore.kernel.org/lkml/9 lkml/git/9.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git