public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Stas Sergeev <stsp@list.ru>
To: Andy Lutomirski <luto@kernel.org>,
	x86@kernel.org, linux-kernel@vger.kernel.org
Cc: Brian Gerst <brgerst@gmail.com>,
	Denys Vlasenko <dvlasenk@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Borislav Petkov <bp@alien8.de>,
	Cyrill Gorcunov <gorcunov@gmail.com>,
	Pavel Emelyanov <xemul@parallels.com>
Subject: Re: [RFC 3/4] x86/signal/64: Re-add support for SS in the 64-bit signal context
Date: Tue, 13 Oct 2015 17:59:31 +0300	[thread overview]
Message-ID: <561D1C53.8080302@list.ru> (raw)
In-Reply-To: <6e14e2f7ce1c38fc1996bd4d5d3d9dc70b8bd94d.1444697927.git.luto@kernel.org>

13.10.2015 04:04, Andy Lutomirski пишет:
> + * UC_SIGCONTEXT_SS will be set when delivering 64-bit or x32 signals on
> + * kernels that save SS in the sigcontext.  All kernels that set
> + * UC_SIGCONTEXT_SS will correctly restore at least the low 32 bits of esp
> + * regardless of SS (i.e. they implement espfix).
Is this comment relevant? I think neither signal delivery
nor sigreturn were affected by esp corruption, or were they?
I guess you suggest to use that flag as the detection
for espfix, but I don't think this is relevant: you may
need to know about espfix also outside of a signal handler.
In fact, I don't think espfix needs any run-time detection,
because then the stack fault will simply not happen, and that's all.
I think it is a matter of compile-time detection only.

> + *
> + * Kernels that set UC_SIGCONTEXT_SS will also set UC_STRICT_RESTORE_SS
> + * when delivering a signal that came from 64-bit code.
> + *
> + * Sigreturn modifies its behavior depending on the UC_STRICT_RESTORE_SS
> + * flag.  If UC_STRICT_RESTORE_SS is set, then the SS value in the
> + * signal context is restored verbatim.  If UC_STRICT_RESTORE_SS is not
> + * set, the CS value in the signal context refers to a 64-bit code
> + * segment, and the signal context's SS value is invalid, it will be
> + * replaced by an flat 32-bit selector.
> +
> + * This behavior serves two purposes.  It ensures that older programs
> + * that are unaware of the signal context's SS slot and either construct
> + * a signal context from scratch or that catch signals from segmented
> + * contexts and change CS to a 64-bit selector won't crash due to a bad
> + * SS value.  It also ensures that signal handlers that do not modify
> + * the signal context at all return back to the exact CS and SS state
> + * that they came from.
Do you need a second flag for this?
IIRC non-restoring was needed because:
1. dosemu saves SS to different place
2. If you save it yourself, dosemu can invalidate it, but not replace
with the right one because of 1.
IMHO to solve this, you need _either_ the second flag or
the heuristic, but not both.

With new flag:
Just don't set it by default, and the new progs will set it themselves.
Old progs are unaffected.
When it is set, SS should always be restored.
I prefer this approach.

With heuristic:
Save SS yourself on delivery, and, if it happens invalid on sigreturn -
replace it with better one.
Old progs are unaffected because they use iret anyway, and that iret
happens _after_ sigreturn.
New progs will never leave invalid SS in the right sigcontext slot.

So why have you choose to have both the new flag UC_STRICT_RESTORE_SS
and the heuristic?

> This is a bit risky, and another option would be to do nothing at
> all.
Andy, could you please stop pretending there are no other solutions?
You do not have to like them. You do not have to implement them.
But your continuous re-assertions that they do not exist, make me
feel a bit uncomfortable after I spelled them many times.

> Stas, what do you think?  Could you test this?
I think I'll get to testing this only at a week-end.
In a mean time, the question about a safety of leaving LDT SS
in 64bit mode still makes me wonder. Perhaps, instead of re-iterating
this here, you can describe this all in the patch comments? Namely:
- How will LDT SS interact with nested signals
- with syscalls
- with siglongjmp()
- with another thread. Do we have a per-thread or a per-process LDT
these days? If LDT is per-process, my question is what will happen
if another thread invalidates an LDT entry while we are in 64bit mode.
If LDT is per-thread, there is no such question.

> If SS starts out invalid (this can happen if the signal was caused
> by an IRET fault or was delivered on the way out of set_thread_area
> or modify_ldt), then IRET to the signal handler can fail, eventually
> killing the task.
Is this signal-pecific? I.e. the return from IRQs happens via iret too.
So if we are running with invalid SS in 64bit mode, can the iret from
IRQ also cause the problem?


On an off-topic: there was recently a patch from you that
disables vm86() by mmap_min_addr. I've found that dosemu, when
started as root, could override mmap_min_addr. I guess this will
no longer work, right? Not a big regression, just something to
know and document.

  reply	other threads:[~2015-10-13 16:01 UTC|newest]

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-10-13  1:04 [RFC 0/4] x86: sigcontext SS fixes, take 2 Andy Lutomirski
2015-10-13  1:04 ` [RFC 1/4] x86/signal/64: Add a comment about sigcontext->fs and gs Andy Lutomirski
2015-10-13  1:04 ` [RFC 2/4] x86/signal/64: Fix SS if needed when delivering a 64-bit signal Andy Lutomirski
2015-10-13  1:04 ` [RFC 3/4] x86/signal/64: Re-add support for SS in the 64-bit signal context Andy Lutomirski
2015-10-13 14:59   ` Stas Sergeev [this message]
2015-10-14 15:01     ` Ingo Molnar
2015-10-14 15:09       ` Stas Sergeev
2015-10-14 16:40     ` Andy Lutomirski
2015-10-14 17:40       ` Stas Sergeev
2015-10-14 18:06         ` Andy Lutomirski
2015-10-14 18:34           ` Stas Sergeev
2015-10-14 18:52             ` Andy Lutomirski
2015-10-14 21:37               ` Stas Sergeev
2015-10-14 21:41                 ` Andy Lutomirski
2015-10-18 13:36                   ` Stas Sergeev
2015-10-18 16:12                     ` Andy Lutomirski
2015-10-18 16:29                       ` Stas Sergeev
2015-10-18 16:36                         ` Andy Lutomirski
2015-10-18 16:43                           ` Stas Sergeev
2015-10-18 17:06                             ` Andy Lutomirski
2015-10-14 16:40   ` Cyrill Gorcunov
2015-10-14 16:42     ` Andy Lutomirski
2015-10-14 16:57       ` Cyrill Gorcunov
2015-10-14 16:57     ` Stas Sergeev
2015-10-14 17:01       ` Cyrill Gorcunov
2015-10-13  1:04 ` [RFC 4/4] selftests/x86: Add tests for UC_SIGCONTEXT_SS and UC_STRICT_RESTORE_SS Andy Lutomirski

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=561D1C53.8080302@list.ru \
    --to=stsp@list.ru \
    --cc=bp@alien8.de \
    --cc=brgerst@gmail.com \
    --cc=dvlasenk@redhat.com \
    --cc=gorcunov@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=torvalds@linux-foundation.org \
    --cc=x86@kernel.org \
    --cc=xemul@parallels.com \
    /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