linux-kernel - Re: [PATCH 1/2] x86/stacktrace: do not fail when regs on stack for ORC

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171130195710.cu2lsa47kwnow2hi@treble>
Date:   Thu, 30 Nov 2017 13:57:10 -0600
From:   Josh Poimboeuf <jpoimboe@...hat.com>
To:     Jiri Slaby <jslaby@...e.cz>
Cc:     mingo@...hat.com, linux-kernel@...r.kernel.org,
        Thomas Gleixner <tglx@...utronix.de>,
        "H. Peter Anvin" <hpa@...or.com>, x86@...nel.org
Subject: Re: [PATCH 1/2] x86/stacktrace: do not fail when regs on stack for
 ORC

On Thu, Nov 30, 2017 at 09:03:24AM +0100, Jiri Slaby wrote:
> save_stack_trace_reliable now returns "non reliable" when there are
> kernel pt_regs on stack. This means an interrupt or exception happened.
> Somewhere down the route. It is a problem for frame pointer unwinder,
> because the frame might now have been set up yet when the irq happened,
> so it might fail to unwind from the interrupted function.
> 
> With ORC, this is not a problem, as ORC has out-of-band data. We can
> find ORC data even for the IP in interrupted function and always unwind
> one level up.
> 
> So introduce `unwind_regs_reliable' which decides if this is an issue
> for the currently selected unwinder at all and change the code
> accordingly.

Thanks.  I'm thinking there a few ways we can simplify things.  (Most of
these are actually issues with the existing code.)

- Currently we check to make sure that there's no frame *after* the user
  space regs.  I think there's no way that could ever happen and the
  check is overkill.

- We should probably remove the STACKTRACE_DUMP_ONCE() warnings.  There
  are some known places where a stack trace will fail, particularly with
  generated code.  I wish we had a DEBUG_WARN_ON() macro which used
  pr_debug(), but oh well.  At least the livepatch code has some helpful
  pr_warn()s, those are probably good enough.

- The unwind->error checks are superfluous.  The only errors we need to
  check for are (a) whether the FP unwinder encountered a kernel irq and
  b) whether we reached the final user regs frame.  So I think
  unwind->error can be removed altogether.

So with those changes in mind, how about something like this (plus
comments)?

	for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);
	     unwind_next_frame(&state)) {

		regs = unwind_get_entry_regs(&state);
		if (regs) {
			if (user_mode(regs))
				goto success;

			if (IS_ENABLED(CONFIG_FRAME_POINTER))
				return -EINVAL;
		}

		addr = unwind_get_return_address(&state);
		if (!addr)
			return -EINVAL;

		if (save_stack_address(trace, addr, false))
			return -EINVAL;
	}

	return -EINVAL;

success:
	if (trace->nr_entries < trace->max_entries)
		trace->entries[trace->nr_entries++] = ULONG_MAX;

	return 0;

After these changes I believe we can enable
CONFIG_HAVE_RELIABLE_STACKTRACE for ORC.

Also, when you post the next version, please cc the live patching
mailing list, since this is directly relevant to livepatch.

Thanks!

-- 
Josh