Re: syscall_get_error() && TS_ checks

[Andy Lutomirski]

On Thu, Mar 30, 2017 at 11:35 AM, Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Mar 30, 2017 at 11:23 AM, Andy Lutomirski <luto@xxxxxxxxxxxxxx> wrote:
>> On Thu, Mar 30, 2017 at 10:46 AM, Linus Torvalds
>> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>>>
>>> We *really* shouldn't sign-extend that value if the debugger ends up
>>> updating the pointer (or maybe the debugger just reloads previous
>>> values, not really "updating" anything - I think that's what gdb does
>>> when you do a call within the context of the debugged program from
>>> within gdb, for example)
>>
>> Can you think of a case where this would actually matter?
>
> I'd actually be willing to have people try, but then you'd have to
> sign-extend *all* registers. No way in hell will I accept a patch that
> randomly sign-extends just %eax.
>
> And then actually run such a kernel on a 32-bit distro, and verifying
> that things like gdb and strace really work. But it needs real
> testing, not some kind of handwaving. It's a *big* change.

I'll offer the following handwave: if there are problems, I'd expect
to see them in mixed-bitness uses, not 32-bit distros.  But the 32-bit
case is worth testing, too.

>
>> --- issue 1 ---
>>
>> get_nr_restart_syscall() does:
>>
>>        if (current->thread.status & (TS_COMPAT|TS_I386_REGS_POKED))
>>                return __NR_ia32_restart_syscall;
>>
>> This is very, very buggy
>
> Quite frankly, a bug somewhere else is not an excuse for then making
> other code buggier. I don't see what "issue 1" has to do with anything
> what-so-ever,.
>
>> --- issue 2 ---
>>
>> syscall_get_error().  This is available on all arches, but it appears
>> to be used *only* on x86.
>
> So I think that one is a red herring. We can trivially fix that by
> just (a) removing everybody elses syscall_get_error(), and just
> inlining the x86 case into the x86 signal handling. Boom, gone.
>
> And in the signal handling path, the sign-extension and test of %eax
> is reivially ok. Not because it's ok in general, but because we've
> verified using %orig_eax that we're in a system call return path. We
> could happily delete the whole TS_I386_REGS_POKED thing, I think.

Then how do we know whether to sign extend?  TS_COMPAT does *not* work
because it isn't set on signal delivery.

>
> So don't even _try_ to equate this code with the general sign
> extension code by ptrace. That is a totally different animal
> altogether.

Of course it's different.  But doing sign extension in general would
avoid the need for this special case, and it seems that the special
case isn't actually correct in cases that people try to use.  Oleg,
can you clarify what's broken?

--Andy