On older 32bit kernels (e.g. Ubuntu Hardy), the seccomp sandbox fails to hand...

On older 32bit kernels (e.g. Ubuntu Hardy), the seccomp sandbox fails to handle
signals correctly. This is primarily a result of the kernel not supporting
non-executable data segments. But it also runs into problems because the
format of the signal frame is subtly different and does not appear to always
include a "magic restorer function".

This changelist removes all dependencies on NX support from the 32bit version
of the code. And it eliminates the code that patches the restorer function.

Both of these features were originally added to make it easier for gdb to
debug code that runs inside of a signal handler. But given the observed problems
with this approach, it does not seem worth the effort.

64bit code seems unaffected by all of these problems -- presumably because
that architecture is a lot more recent. So, we'll not make any changes to it.

BUG=http://code.google.com/p/seccompsandbox/issues/detail?id=5
TEST=make test

Committed: http://code.google.com/p/seccompsandbox/source/detail?r=147

[Markus (顧孟勤), 2010-11-09 03:26:20]

This bug was causing problems with running unittests on the buildbots.

In particular, the code that patched the signature of the restorer function was
corrupting the FPU state.

And we couldn't debug this problem easily, because the missing NX support then
broke signal handling.

Evan helped with finding a reproducible test scenario in a Hardy VM, and after
applying this change the unittest (e.g. Chrome's browser_tests) succeeds.

[Mark Seaborn, 2010-11-09 09:40:31]

LGTM.  I didn't understand this code very well before, but this makes it a bit
simpler.

One suggestion: if I commit the asm-file conversion
(http://codereview.chromium.org/4325001/show), you could add textual labels,
which would make this easier to understand.  Single-char numeric labels are very
hard to navigate (especially with two architectures in one file!).  But you are
welcome to commit this change before that conversion.

http://codereview.chromium.org/4688002/diff/1/2
File sandbox.cc (right):

http://codereview.chromium.org/4688002/diff/1/2#newcode557
sandbox.cc:557: "movl $27f, 0(%%esp)\n"      // trigger a SEGV on return (if NX
enabled)
I don't understand the "(if NX enabled)" comment.  Surely label 27 is
executable?

http://codereview.chromium.org/4688002/diff/1/2#newcode582
sandbox.cc:582: "movl $6b, 0(%%esp)\n"      // set appropriate restorer function
Indentation is 1 space off

[Markus (顧孟勤), 2010-11-09 16:45:38]

On Tue, Nov 9, 2010 at 01:40, <mseaborn@chromium.org> wrote:

> LGTM.  I didn't understand this code very well before, but this makes it a
> bit
> simpler.
>

Good. I am a little sad we are loosing this feature, as it conceivably would
have made things a little easier when debugging with gdb. But it really
seems too fragile. And I am glad you agree the code is more readable. No
matter what, assembly is always difficult to read.


> One suggestion: if I commit the asm-file conversion
> (http://codereview.chromium.org/4325001/show), you could add textual
> labels,
> which would make this easier to understand.  Single-char numeric labels are
> very
> hard to navigate (especially with two architectures in one file!).  But you
> are
> welcome to commit this change before that conversion.
>

Yes, the conversion to an external assembly file is going to help with
making this code easier to read. And symbolic labels are a good idea in that
case.

But we should make sure that we pick a naming convention that doesn't
pollute the global namespace. I think, this probably means all labels need
to start with ".L". But I haven't actually verified whether that works.


> http://codereview.chromium.org/4688002/diff/1/2
> File sandbox.cc (right):
>
> http://codereview.chromium.org/4688002/diff/1/2#newcode557
> sandbox.cc:557: "movl $27f, 0(%%esp)\n"      // trigger a SEGV on return
> (if NX enabled)
> I don't understand the "(if NX enabled)" comment.  Surely label 27 is
> executable?
>

Great catch. Thank you for finding that. Yes, that was a stale comment. I
fixed it now.

http://codereview.chromium.org/4688002/diff/1/2#newcode582
> sandbox.cc:582: "movl $6b, 0(%%esp)\n"      // set appropriate restorer
> function
> Indentation is 1 space off


Done


>
>
> http://codereview.chromium.org/4688002/show
>

[Mark Seaborn, 2010-11-09 20:01:23]

On 9 November 2010 16:45, Markus Gutschke <markus@chromium.org> wrote:

> On Tue, Nov 9, 2010 at 01:40, <mseaborn@chromium.org> wrote:
>
>> One suggestion: if I commit the asm-file conversion
>
> (http://codereview.chromium.org/4325001/show), you could add textual
>> labels,
>> which would make this easier to understand.  Single-char numeric labels
>> are very
>> hard to navigate (especially with two architectures in one file!).  But
>> you are
>> welcome to commit this change before that conversion.
>>
>
> Yes, the conversion to an external assembly file is going to help with
> making this code easier to read. And symbolic labels are a good idea in that
> case.
>
> But we should make sure that we pick a naming convention that doesn't
> pollute the global namespace. I think, this probably means all labels need
> to start with ".L". But I haven't actually verified whether that works.
>

As long as you don't declare a symbol with ".global", you're not polluting
the global namespace, just using the compilation unit's local namespace.  I
did this with "fatal_error" in trusted_thread_*.S.  So I don't think we need
a prefix for that.

Cheers,
Mark

[Markus (顧孟勤), 2010-11-09 20:04:42]

I am not quite sure about that. Gdb definitely seems to see that label
globally. It also gets confused and thinks it starts a new function. So, if
you try to disassemble all of playground$runTrustedThread you can't. It will
stop as soon as it encounters the fatal_error label.

I haven't checked whether gdb is the only thing affected by this, or whether
the label is confusing other programs (e.g. linker, dynamic loader, ...).


Markus

On Tue, Nov 9, 2010 at 12:01, Mark Seaborn <mseaborn@chromium.org> wrote:

> On 9 November 2010 16:45, Markus Gutschke <markus@chromium.org> wrote:
>
>> On Tue, Nov 9, 2010 at 01:40, <mseaborn@chromium.org> wrote:
>>
>>> One suggestion: if I commit the asm-file conversion
>>
>> (http://codereview.chromium.org/4325001/show), you could add textual
>>> labels,
>>> which would make this easier to understand.  Single-char numeric labels
>>> are very
>>> hard to navigate (especially with two architectures in one file!).  But
>>> you are
>>> welcome to commit this change before that conversion.
>>>
>>
>> Yes, the conversion to an external assembly file is going to help with
>> making this code easier to read. And symbolic labels are a good idea in that
>> case.
>>
>> But we should make sure that we pick a naming convention that doesn't
>> pollute the global namespace. I think, this probably means all labels need
>> to start with ".L". But I haven't actually verified whether that works.
>>
>
> As long as you don't declare a symbol with ".global", you're not polluting
> the global namespace, just using the compilation unit's local namespace.  I
> did this with "fatal_error" in trusted_thread_*.S.  So I don't think we need
> a prefix for that.
>
> Cheers,
> Mark
>
>

[Mark Seaborn, 2010-11-09 20:55:02]

On 9 November 2010 20:04, Markus Gutschke <markus@chromium.org> wrote:

> I am not quite sure about that. Gdb definitely seems to see that label
> globally. It also gets confused and thinks it starts a new function. So, if
> you try to disassemble all of playground$runTrustedThread you can't. It will
> stop as soon as it encounters the fatal_error label.
>
> I haven't checked whether gdb is the only thing affected by this, or
> whether the label is confusing other programs (e.g. linker, dynamic loader,
> ...).
>

Hmm, I see what you mean.  I was only considering the linker -- which I
think should be fine, because static functions are declared this way.
Though adding a ".L" prefix wouldn't hurt for more generically-named labels.

It looks like gdb treats runTrustedThread as one function (and not finishing
at fatal_error), if this is added to the end:
        .size playground$runTrustedThread, . - playground$runTrustedThread

Cheers,
Mark

On Tue, Nov 9, 2010 at 12:01, Mark Seaborn <mseaborn@chromium.org> wrote:
>
>> On 9 November 2010 16:45, Markus Gutschke <markus@chromium.org> wrote:
>>
>>>  On Tue, Nov 9, 2010 at 01:40, <mseaborn@chromium.org> wrote:
>>>
>>>> One suggestion: if I commit the asm-file conversion
>>>
>>> (http://codereview.chromium.org/4325001/show), you could add textual
>>>> labels,
>>>> which would make this easier to understand.  Single-char numeric labels
>>>> are very
>>>> hard to navigate (especially with two architectures in one file!).  But
>>>> you are
>>>> welcome to commit this change before that conversion.
>>>>
>>>
>>> Yes, the conversion to an external assembly file is going to help with
>>> making this code easier to read. And symbolic labels are a good idea in that
>>> case.
>>>
>>> But we should make sure that we pick a naming convention that doesn't
>>> pollute the global namespace. I think, this probably means all labels need
>>> to start with ".L". But I haven't actually verified whether that works.
>>>
>>
>> As long as you don't declare a symbol with ".global", you're not polluting
>> the global namespace, just using the compilation unit's local namespace.  I
>> did this with "fatal_error" in trusted_thread_*.S.  So I don't think we need
>> a prefix for that.
>>
>> Cheers,
>> Mark
>>
>>
>