CHROMIUM: arch/arm: move secure_computing into trace; respec... [chromiumos/third_party/kernel : chromeos-3.2]

7 views
Skip to first unread message

Will Drewry (Code Review)

unread,
Apr 25, 2012, 3:35:06 PM4/25/12
to Olof Johansson, Kees Cook, Mandeep Singh Baines
Hello Olof Johansson, Kees Cook,

I'd like you to do a code review. Please visit

https://gerrit.chromium.org/gerrit/21107

to review the following change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................

CHROMIUM: arch/arm: move secure_computing into trace; respect return code

(I will post this upstream after the 3.5 merge window)

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
patsh in entry-common.S. In order to add support for
CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too
badly, seccomp was moved into the syscall_trace() handler.

Additionally, the return value for secure_computing() is now checked
and a -1 value will result in the system call being skipped.

Change-Id: I0b241909a1a0a13774cfb113c654420322dcb825
Signed-off-by: Will Drewry <w...@chromium.org>

BUG=chromium-os:27878
TEST=compiles for arm. Need to test on a live machine.
---
M arch/arm/kernel/entry-common.S
M arch/arm/kernel/ptrace.c
2 files changed, 4 insertions(+), 6 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/07/21107/1
--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Id1bd39d603f410d94874761ed4d9673d7d28ff62
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Kees Cook <kees...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>

Kees Cook (Code Review)

unread,
Apr 25, 2012, 4:14:27 PM4/25/12
to Will Drewry, Olof Johansson
Kees Cook has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: I would prefer that you didn't submit this

(2 inline comments)

....................................................
File arch/arm/kernel/entry-common.S
Line 441: tst r10, #_TIF_SYSCALL_TRACE @ are we tracing syscalls?
I think you need to merge these r10 tests, otherwise we end up calling __sys_trace twice in a row.

....................................................
File arch/arm/kernel/ptrace.c
Line 912: if (why == 0 && secure_computing(scno) == -1)
should this be < 0 rather than specifically -1?

--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Will Drewry (Code Review)

unread,
Apr 25, 2012, 6:07:42 PM4/25/12
to Olof Johansson, Kees Cook
Will Drewry has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: (2 inline comments)

thanks for the comments! Any thoughts?

....................................................
File arch/arm/kernel/entry-common.S
Line 441: tst r10, #_TIF_SYSCALL_TRACE @ are we tracing syscalls?
Why? If TIF_SECCOMP matches, it jumps to __sys_trace and if it doesn't then the next one jumps to __sys_trace. I don't think it ever returns to the callsite, so I don't think it dupes.

Unfortunately, it's non-trivial to merge these tests because there isn't enough space in the argument field for tst to have _TIF_SYSCALL_TRACE|_TIF_SECCOMP. I'd have to ld it somewhere and test that :(

Am I way off with the branch behavior?

....................................................
File arch/arm/kernel/ptrace.c
Line 912: if (why == 0 && secure_computing(scno) == -1)
Maybe -- I specify -1 in the arch documentation, but perhaps < 0 would be a better move?

--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1bd39d603f410d94874761ed4d9673d7d28ff62
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Kees Cook <kees...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>

Kees Cook (Code Review)

unread,
Apr 25, 2012, 6:16:16 PM4/25/12
to Will Drewry, Olof Johansson
Kees Cook has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Looks good to me, approved

(2 inline comments)

....................................................
File arch/arm/kernel/entry-common.S
Line 441: tst r10, #_TIF_SYSCALL_TRACE @ are we tracing syscalls?
Ooops, sorry, I'm confused about "BL" that does the branch-and-link, which is actually used in __sys_trace. Nevermind!

....................................................
File arch/arm/kernel/ptrace.c
Line 912: if (why == 0 && secure_computing(scno) == -1)
I love being overly cautious for these things, so this is really just a nit.

--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1bd39d603f410d94874761ed4d9673d7d28ff62
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Kees Cook <kees...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>

Will Drewry (Code Review)

unread,
Apr 26, 2012, 3:56:56 PM4/26/12
to Olof Johansson, Mandeep Singh Baines
Will Drewry has uploaded a new change for review.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................

CHROMIUM: arch/arm: move secure_computing into trace; respect return code

(I will post this upstream after the 3.5 merge window)

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
patsh in entry-common.S. In order to add support for
CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too
badly, seccomp was moved into the syscall_trace() handler.

Additionally, the return value for secure_computing() is now checked
and a -1 value will result in the system call being skipped.

Change-Id: I0b241909a1a0a13774cfb113c654420322dcb825
Signed-off-by: Will Drewry <w...@chromium.org>

BUG=chromium-os:27878
TEST=compiles for arm. Need to test on a live machine.
---
M arch/arm/kernel/entry-common.S
M arch/arm/kernel/ptrace.c
2 files changed, 6 insertions(+), 8 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/42/21242/1
--
To view, visit https://gerrit.chromium.org/gerrit/21242
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I2d97dc10a7003ba85b643b51be5aa0e9eb0fd92c

Will Drewry (Code Review)

unread,
Apr 26, 2012, 4:01:40 PM4/26/12
to
Will Drewry has abandoned this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Abandoned

--
To view, visit https://gerrit.chromium.org/gerrit/21242
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon

Will Drewry (Code Review)

unread,
Apr 26, 2012, 4:10:10 PM4/26/12
to
Will Drewry has abandoned this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Restored

unb0rking

--
To view, visit https://gerrit.chromium.org/gerrit/21242
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon

Will Drewry (Code Review)

unread,
Apr 27, 2012, 3:56:02 PM4/27/12
to Olof Johansson, Mandeep Singh Baines
Will Drewry has uploaded a new change for review.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................

CHROMIUM: arch/arm: move secure_computing into trace; respect return code

(I will post this upstream after the 3.5 merge window)

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
patsh in entry-common.S. In order to add support for
CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too
badly, seccomp was moved into the syscall_trace() handler.

Additionally, the return value for secure_computing() is now checked
and a -1 value will result in the system call being skipped.

Change-Id: I0b241909a1a0a13774cfb113c654420322dcb825
Signed-off-by: Will Drewry <w...@chromium.org>

BUG=chromium-os:27878
TEST=compiles for arm. Need to test on a live machine.
---
M arch/arm/kernel/entry-common.S
M arch/arm/kernel/ptrace.c
2 files changed, 6 insertions(+), 8 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/kernel refs/changes/76/21376/1
--
To view, visit https://gerrit.chromium.org/gerrit/21376
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I61470515539427a3f61667710715af8ee9d754da

Will Drewry (Code Review)

unread,
Apr 27, 2012, 4:00:13 PM4/27/12
to
Will Drewry has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Verified; Looks good to me, approved

(Note, all items in the series have been reviewed by keescook@ too and tested by jorgelo@ as well. I confused gerrit, so I'll include this note on all pushes).

--
To view, visit https://gerrit.chromium.org/gerrit/21376
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I61470515539427a3f61667710715af8ee9d754da
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>

Will Drewry (Code Review)

unread,
Apr 27, 2012, 4:00:14 PM4/27/12
to
Will Drewry has submitted this change and it was merged.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


CHROMIUM: arch/arm: move secure_computing into trace; respect return code

(I will post this upstream after the 3.5 merge window)

There is very little difference in the TIF_SECCOMP and TIF_SYSCALL_TRACE
patsh in entry-common.S. In order to add support for
CONFIG_HAVE_ARCH_SECCOMP_FILTER without mangling the assembly too
badly, seccomp was moved into the syscall_trace() handler.

Additionally, the return value for secure_computing() is now checked
and a -1 value will result in the system call being skipped.

Change-Id: I0b241909a1a0a13774cfb113c654420322dcb825
Signed-off-by: Will Drewry <w...@chromium.org>

BUG=chromium-os:27878
TEST=compiles for arm. Need to test on a live machine.
---
M arch/arm/kernel/entry-common.S
M arch/arm/kernel/ptrace.c
2 files changed, 6 insertions(+), 8 deletions(-)

Approvals:
Will Drewry: Verified; Looks good to me, approved


--
To view, visit https://gerrit.chromium.org/gerrit/21376
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I61470515539427a3f61667710715af8ee9d754da
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>

Will Drewry (Code Review)

unread,
Apr 27, 2012, 11:38:54 PM4/27/12
to
Will Drewry has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................




--
To view, visit https://gerrit.chromium.org/gerrit/21242
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment

Will Drewry (Code Review)

unread,
Apr 27, 2012, 11:38:54 PM4/27/12
to
Will Drewry has abandoned this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Abandoned

--
To view, visit https://gerrit.chromium.org/gerrit/21242
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon

Will Drewry (Code Review)

unread,
Apr 27, 2012, 11:39:05 PM4/27/12
to Olof Johansson, Kees Cook
Will Drewry has posted comments on this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................




--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Id1bd39d603f410d94874761ed4d9673d7d28ff62
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Kees Cook <kees...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>

Will Drewry (Code Review)

unread,
Apr 27, 2012, 11:39:05 PM4/27/12
to Olof Johansson, Kees Cook
Will Drewry has abandoned this change.

Change subject: CHROMIUM: arch/arm: move secure_computing into trace; respect return code
......................................................................


Patch Set 1: Abandoned

--
To view, visit https://gerrit.chromium.org/gerrit/21107
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: abandon
Gerrit-Change-Id: Id1bd39d603f410d94874761ed4d9673d7d28ff62
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/kernel
Gerrit-Branch: chromeos-3.2
Gerrit-Owner: Will Drewry <w...@chromium.org>
Gerrit-Reviewer: Kees Cook <kees...@chromium.org>
Gerrit-Reviewer: Olof Johansson <ol...@chromium.org>
Gerrit-Reviewer: Will Drewry <w...@chromium.org>
Reply all
Reply to author
Forward
0 new messages