Re: [regression] x86/signal/64: Fix SS handling for signals delivered to 64-bit programs breaks dosemu

[Andy Lutomirski]

On Wed, Sep 2, 2015 at 7:21 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
> On Wed, Sep 2, 2015 at 2:17 AM, Stas Sergeev <stsp@xxxxxxx> wrote:
>> 02.09.2015 08:12, Andy Lutomirski ÐÐÑÐÑ:
>>
>>> On Wed, Aug 19, 2015 at 9:30 AM, Stas Sergeev <stsp@xxxxxxx> wrote:
>>>>
>>>> 19.08.2015 18:46, Andy Lutomirski ÐÐÑÐÑ:
>>>>>
>>>>> On Wed, Aug 19, 2015 at 2:35 AM, Stas Sergeev <stsp@xxxxxxx> wrote:
>>>>>>>
>>>>>>> Incidentally, I tried implementing the sigaction flag approach.  I
>>>>>>> think it's no good.  When we return from a signal, there's no concept
>>>>>>> of sigaction -- it's just sigreturn.  Sigreturn can't look up the
>>>>>>> sigaction flags -- what if the signal handler calls sigaction itself.
>>>>>>
>>>>>> How about the SA_hyz flag that does the following:
>>>>>> - Saves SS into sigcontext
>>>>>> - Forces SS to USER_DS on signal delivery
>>>>>> - Sets the uc_flags flag for sigreturn() to take care of the rest.
>>>>>> You'll have both the control on every bit of action, and a simple
>>>>>> detection logic: if SA_hyz didn't set the uc flag - it didn't work.
>>>>>> You can even employ your lar heuristic here for the case when the
>>>>>> aforementioned SA_hyz is not set. But please, please not when it is
>>>>>> set! In fact, I wonder if you had in mind exactly that: using the
>>>>>> lar heuristic only if the SA_hyz is not set. If so - I misunderstood.
>>>>>> Just please don't add it when it is set.
>>>>>
>>>>> Hmm, interesting.  Maybe that would work for everything.  How's this
>>>>> to make it concrete?
>>>>>
>>>>> Add a sigaction flag SA_RESTORE_SS.
>>>>>
>>>>> On signal delivery, always save SS into sigcontext->ss. if
>>>>> SA_RESTORE_SS is set, then unconditionally switch HW SS to __USER_DS
>>>>> and set UC_RESTORE_SS.  If SA_RESTORE_SS is clear, then leave HW SS
>>>>> alone (i.e. preserve the old behavior).
>>>>
>>>> Either that, or employ the lar heuristic for the "not set" case
>>>> (I think its not needed).
>>>>
>>>>> On signal return, if UC_RESTORE_SS is set, then restore
>>>>> sigcontext->ss.  If not, then set SS to __USER_DS (as old kernels
>>>>> did).
>>>>>
>>>>> This should change nothing at all (except the initial value of
>>>>> sigcontext->ss / __pad0) on old kernels.
>>>>
>>>> Agreed.
>>>>
>>> Let me throw out one more possibility, just for completeness:
>>>
>>> We don't add any SA_xyz flags.  On signal delivery, we use the LAR
>>> heuristic.  We always fill in sigcontext->ss, and we set a new
>>> UC_SIGCONTEXT_SS flag to indicate that we support the new behavior.
>>>
>>> On sigreturn, we honor the sigcontext's ss, *unless* CS is 64 bit and
>>> SS is invalid.  In the latter case, we replace the saved ss with
>>> __USER_DS.
>>
>> But this is not a new proposal, see here:
>> https://lkml.org/lkml/2015/8/13/436
>> The very last sentence says exactly the same.
>> I thought this is in the past. :)
>>
>
> True, but I still want to make sure I understand the alternatives
> well, and we never really considered this approach.
>
>>> This should work for old DOSEMU.  It's a bit gross, but it has the
>>> nice benefit that everyone (even things that aren't DOSEMU) gain the
>>> ability to catch signals thrown from bogus SS contexts, which probably
>>> improves debugability.  It's also nice to not have the SA flag.
>>
>> Pros:
>> - No new SA flag
>> - May improve debugability in some unknown scenario where people
>> do not want to just use the new flag to get their things improved
>>
>> Cons:
>> - Does not allow to cleanly use siglongjmp(), as then there is a risk
>> to jump to 64bit code with bad SS
>
> What's the issue here?  I don't understand.
>
> On musl, (sig)longjmp just restores rsp, rbx, rbp, and r12-r15, so it
> won't be affected.  AFAIK all implementations of siglongjmp are likely
> to call sigprocmask or similar, and that will clobber SS.  I'm not
> aware of an implementation of siglongjmp that uses sigreturn.
>
>> - Async signals can silently "validate" SS behind your back
>
> True, and that's unfortunate.  But async signals without SA_SAVE_SS
> set with the other approach have exactly the same problem.  At least
> with the approach it won't happen in 32-bit or 16-bit code, as we'd
> only do the heuristic SS fix on sigreturn when returning to 64-bit
> code.
>
>> - No way to extend that solution to later fixing the TLS problem
>
> True.
>
>> - Many ugly checks in the code, that are not always even obvious
>> (eg you wanted to try verw instead, and there was a gotcha with
>> NP bit)
>
> Also true.  OTOH, there'll be a unit test.
>
>>
>> Is the new SA flag such a big deal here to even bother?
>
> Not really, but given that the new behavior seems clearly better
> behaved than the old, it would be nice to be able to have the good
> behavior, or at least most of it, be the default.
>

In fact, I think we can do even better.

On signal delivery:
 - Always set UC_SAVED_SS (for feature detection)
 - Always save SS into sigcontext
 - Use the LAR heuristic to fix SS
 - If the old CS was 64-bit, set UC_STRICT_RESTORE_SS

On sigreturn:
 - Ignore UC_SAVED_SS.
 - Restore SS.
 - If the restored SS is bad, the new CS is 64-bit, and
UC_STRICT_RESTORE_SS is *not* set, then replace SS with __USER_DS.

In other words, we honor the sigcontext SS *unless* the original
signal context was not 64-bit (or UC_STRICT_RESTORE_SS was otherwise
cleared), the new context is 64-bit, and the new SS is bad.

If I understand correctly, old dosemu will keep working: old dosemu
never sigreturns back to anything other than 64-bit mode because it
can't.  (Unless there's a case where it does flat 32-bit mode using
__USER_DS -- is there?)

CRIU should be fine, since modern CRIU already saves and restore SS
and since older CRIU presumably doesn't set UC_STRICT_RESTORE_SS.

sigcontext_64 will work unmodified because it'll pick up
UC_STRICT_RESTORE_SS automatically as it does its weird sigreturns
directly from raise().

New DOSEMU can detect that the old workarounds aren't needed when it
sees UC_SAVED_SS and, if it wants stricter behavior, it can manually
mask off UC_STRICT_RESTORE_SS.

Old Wine won't be affected because I don't think Wine ever does 32-bit
emulation from a 64-bit binary in the first place.

Faults due to bad SS will never be silently fixed up because the
silent fixup only happens if the signal handler explicitly changes CS,
and only things that are aware of segmentation will do that.  (IOW
some random SIGALRM handler isn't going to touch sigcontext->cs.)

The new behavior is arguably not even insane.  People who reset CS to
some 64-bit value probably don't care what's in SS on return since SS
has no effect in 64-bit mode except possibly causing IRET to fail.
The only thing that seems to care is the DOSEMU IRET trampoline, and
that reprograms SS anyway (unless I'm still missing something).

Thoughts?

--Andy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/