linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	PaX Team <pageexec@freemail.hu>, Jann Horn <jannh@google.com>,
	Eric Biggers <ebiggers3@gmail.com>,
	Christoph Hellwig <hch@infradead.org>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Elena Reshetova <elena.reshetova@intel.com>,
	Hans Liljestrand <ishkamiel@gmail.com>,
	David Windsor <dwindsor@gmail.com>,
	"x86@kernel.org" <x86@kernel.org>, Ingo Molnar <mingo@kernel.org>,
	Arnd Bergmann <arnd@arndb.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	"David S. Miller" <davem@davemloft.net>,
	Rik van Riel <riel@redhat.com>,
	linux-arch <linux-arch@vger.kernel.org>,
	"kernel-hardening@lists.openwall.com" 
	<kernel-hardening@lists.openwall.com>
Subject: Re: [PATCH v2 2/2] x86, refcount: Implement fast refcount overflow protection
Date: Mon, 1 May 2017 17:33:50 -0500	[thread overview]
Message-ID: <20170501223350.6lxjyvkwz4fwjyqd@treble> (raw)
In-Reply-To: <CAGXu5j+OKnHB2POjyBMGc8RXWbgDzi3xiZFBa5Jz0-jNJeOpqg@mail.gmail.com>

On Mon, May 01, 2017 at 10:28:53AM -0700, Kees Cook wrote:
> On Mon, May 1, 2017 at 8:54 AM, Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > On Thu, Apr 27, 2017 at 01:22:05PM -0700, Kees Cook wrote:
> >> +#define __REFCOUNT_EXCEPTION(size)                     \
> >> +       ".if "__stringify(size)" == 4\n\t"              \
> >> +       ".pushsection .text.refcount_overflow\n"        \
> >> +       ".elseif "__stringify(size)" == -4\n\t"         \
> >> +       ".pushsection .text.refcount_underflow\n"       \
> >> +       ".else\n"                                       \
> >> +       ".error \"invalid size\"\n"                     \
> >> +       ".endif\n"                                      \
> >> +       "111:\tlea %[counter],%%"_ASM_CX"\n\t"          \
> >> +       "int $"__stringify(X86_REFCOUNT_VECTOR)"\n"     \
> >> +       "222:\n\t"                                      \
> >> +       ".popsection\n"                                 \
> >> +       "333:\n"                                        \
> >> +       _ASM_EXTABLE(222b, 333b)
> >> +
> >> +#define __REFCOUNT_CHECK(size)                         \
> >> +       "js 111f\n"                                     \
> >> +       __REFCOUNT_EXCEPTION(size)
> >> +
> >> +#define __REFCOUNT_ERROR(size)                         \
> >> +       "jmp 111f\n"                                    \
> >> +       __REFCOUNT_EXCEPTION(size)
> >>
> >> I assume it doesn't like seeing an exception split across .text and
> >> .text.refcount_overflow, but I haven't been able to figure out how
> >> that distinction would be made by the checker. :P
> >
> > This code uses the exception table a little differently than normal.
> > Usually it's used for catching page faults, where the exception table
> > points to the faulting instruction.
> >
> > But instead of a page fault, here it's doing a software interrupt.  So
> > the __ex_table entry doesn't point to the 'int 0x81' instruction, it
> > points to the instruction immediately after it.  In this case there
> > isn't actually an instruction there, which is why objtool is
> > complaining.
> 
> What would it take to adjust objtool for this case?

I still need to think about it some more. I doubt it would be
straightforward.  But I am ok with making such a change, if it makes
sense to do so.

> > Is it superfluous to use the exception table here, when a simple 'jmp
> > 333f' could be used instead after the 'int'?
> 
> I thought the exception tables were needed to have the trap handler
> notice it correctly, and do the right thing as far as continuing
> execution. (This is currently written as a survivable condition: the
> kernel can keep running even though it will kill the userspace
> process.)

Nothing needs to be done to make it continue execution, because regs->ip
will already have the address immediately after the 'int' instruction,
which is where the iret will return.  So fixup_exception() isn't needed.

Instead, along with the aforementioned 'jmp 333f',  you could move the
refcount_error_report() call to outside the fixup_exception() clause:

if (!user_mode(regs)) {
	if (fixup_exception(regs, trapnr))
		return 0;

	if (fixup_bug(regs, trapnr))
		return 0;

	if (IS_ENABLED(CONFIG_FAST_REFCOUNT) && trapnr == X86_REFCOUNT_VECTOR) {
		refcount_error_report(regs, str);
		return 0;
	}

(For better readability the refcount parts could be moved to a
fixup_refcount() function.)

Or, another way to handle it would be to use a real exception.  One idea
would be to share UD0 with WARN somehow, and handle it with fixup_bug().
At least then, IMO, the error handling code would be closer to where it
belongs, with WARN and BUG.

But stepping back a bit, why is the interrupt/exception even needed?  Is
there a reason why it can't do the stack dump from the context of the
original overflow?  i.e., instead of 'int 0x81', just call the error
handler directly.  Which could then WARN, send the self-signal, update
the refcount to INT_MAX, etc.

> > Also it looks like the handler sends a SIGKILL to the current task.  I
> > wonder if something like BUG_ON() could be used instead of implementing
> > a custom error interrupt.
> 
> It's a rate limited report, but it must always kill. BUG doesn't fit
> this usage case (I've got similar problems with other areas; my
> intention is go create something that is configurable WARN vs Oops,
> respects panic_on_oops, etc, but this doesn't exist yet).

Yeah, it would be good to make BUG and WARN flexible enough such that we
don't need these custom errors, so we can get more consistent error
handling behavior.

-- 
Josh

  reply	other threads:[~2017-05-01 22:33 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-25 22:56 [PATCH v2 0/2] x86, refcount: Implement fast refcount overflow Kees Cook
2017-04-25 22:56 ` [PATCH v2 1/2] x86, asm: Add suffix macro for GEN_*_RMWcc() Kees Cook
2017-04-25 22:56 ` [PATCH v2 2/2] x86, refcount: Implement fast refcount overflow protection Kees Cook
2017-04-26  0:25   ` Jann Horn
2017-04-26  3:52     ` Kees Cook
2017-04-27  1:31   ` kbuild test robot
2017-04-27 20:22     ` Kees Cook
2017-05-01 15:54       ` Josh Poimboeuf
2017-05-01 17:28         ` Kees Cook
2017-05-01 22:33           ` Josh Poimboeuf [this message]
2017-05-01 16:30   ` Josh Poimboeuf
2017-05-01 17:36     ` Kees Cook
2017-05-01 22:45       ` Josh Poimboeuf
2017-04-26  2:01 ` [PATCH v2 0/2] x86, refcount: Implement fast refcount overflow PaX Team
2017-04-26  3:59   ` Kees Cook

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=20170501223350.6lxjyvkwz4fwjyqd@treble \
    --to=jpoimboe@redhat.com \
    --cc=James.Bottomley@hansenpartnership.com \
    --cc=arnd@arndb.de \
    --cc=axboe@kernel.dk \
    --cc=davem@davemloft.net \
    --cc=dwindsor@gmail.com \
    --cc=ebiggers3@gmail.com \
    --cc=elena.reshetova@intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hch@infradead.org \
    --cc=ishkamiel@gmail.com \
    --cc=jannh@google.com \
    --cc=keescook@chromium.org \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=pageexec@freemail.hu \
    --cc=peterz@infradead.org \
    --cc=riel@redhat.com \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).