Disable yama on 3.2 kernels with 32bit userland and 64bit kernel.

Disable two yama tests on 3.2 kernels with 32bit userland and 64bit kernel.

Disabling yama restrictions is broken there.

BUG=391916
R=jorgelo@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281666

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281757

[Nico, 2014-07-07 23:00:29]

Kees: I'm not sure if this is the right fix; I guessed from your commit message
that I linked to on the bug.

[Jorge Lucangeli Obes, 2014-07-08 04:14:28]

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama.cc
File sandbox/linux/services/yama.cc (right):

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.cc:36: if (enable_restrictions &&
Yama::HasLinux32Bug()) {
This doesn't really make sense. The problem is when trying to *disable* Yama
restrictions, not when trying to enable them.

You could argue that we should not enable the restrictions if we know we cannot
disable them, but I'd rather just adapt the tests and not this code. We're
making users less secure here.

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.cc:87: return 0;
As above.

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.cc:129: // On 3.2 kernels, yama doesn't work for
32bit binaries on 64bit kernels.
32-bit, 64-bit

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.cc:135: base::SysInfo::OperatingSystemVersion(),
"3.2", /*case_sensitive=*/false);
'false /* case_sensitive */'

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama.h
File sandbox/linux/services/yama.h (right):

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.h:52: // Linux 3.2 has a bug with 32bit userspace
and 64bit kernels. This function
32-bit, 64-bit

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama_...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama_...
sandbox/linux/services/yama_unittests.cc:75: if (!Yama::HasLinux32Bug()) {
GetStatus should still work as before.

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama_...
sandbox/linux/services/yama_unittests.cc:117: if (!Yama::IsPresent())
This is where we should be if-casing 3.2 kernels.

[Nico, 2014-07-08 04:24:43]

Thanks! All done.

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama.cc
File sandbox/linux/services/yama.cc (right):

https://codereview.chromium.org/371113003/diff/1/sandbox/linux/services/yama....
sandbox/linux/services/yama.cc:135: base::SysInfo::OperatingSystemVersion(),
"3.2", /*case_sensitive=*/false);
On 2014/07/08 04:14:28, jorgelo (OOO till July 11th) wrote:
> 'false /* case_sensitive */'

This is somewhat common style:
https://code.google.com/p/chromium/codesearch#search/&q=%5C/%5C*.*%5Cw=%5C*&s...
(and even detected by clang-format; it omits the spaces in the comment when it
detects this)

[Jorge Lucangeli Obes, 2014-07-08 04:57:14]

lgtm but please fix the nits.

https://codereview.chromium.org/371113003/diff/20001/sandbox/linux/services/y...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/20001/sandbox/linux/services/y...
sandbox/linux/services/yama_unittests.cc:26: // On 3.2 kernels, yama doesn't
work for 32bit binaries on 64bit kernels.
Yama, 32-bit, 64-bit

[Nico, 2014-07-08 05:00:47]

Thanks!

https://codereview.chromium.org/371113003/diff/20001/sandbox/linux/services/y...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/20001/sandbox/linux/services/y...
sandbox/linux/services/yama_unittests.cc:26: // On 3.2 kernels, yama doesn't
work for 32bit binaries on 64bit kernels.
On 2014/07/08 04:57:14, jorgelo (OOO till July 11th) wrote:
> Yama, 32-bit, 64-bit

I had fixed the 32bit/64bit thing (see patch set 3). Fixed Yama now too.

[Nico, 2014-07-08 05:01:54]

Committed patchset #3 manually as r281666 (presubmit successful).

[Kees Cook, 2014-07-08 05:14:57]

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
sandbox/linux/services/yama_unittests.cc:27: // This is fixed in 3.4.
Strictly speaking, Yama doesn't _exist_ on stock 3.2 kernels. I think it would
be better to directly test the failure condition instead of using kernel
version.

(i.e. calling prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) returns EINVAL
means the feature is busted.)

[nhiroki, 2014-07-08 05:54:49]

A revert of this CL has been created in
https://codereview.chromium.org/378793002/ by nhiroki@chromium.org.

The reason for reverting is: This seems to be breaking Linux builds:

http://build.chromium.org/p/chromium/builders/Linux/builds/51190
http://build.chromium.org/p/chromium.linux/builders/Linux%20Builder%20%28dbg%....

[Lei Zhang, 2014-07-08 06:02:45]

Bot failed with:

../../sandbox/linux/services/yama_unittests.cc: In function 'bool
sandbox::{anonymous}::HasLinux32Bug()':
../../sandbox/linux/services/yama_unittests.cc:29:13: error: 'base::SysInfo' has
not been declared
../../sandbox/linux/services/yama_unittests.cc:30:25: error: 'base::SysInfo' has
not been declared
../../sandbox/linux/services/yama_unittests.cc:32:13: error: 'base::SysInfo' has
not been declared
../../sandbox/linux/services/yama_unittests.cc:32:79: error: 'StartsWithASCII'
was not declared in this scope

[Jorge Lucangeli Obes, 2014-07-08 13:56:58]

On 2014/07/08 06:02:45, Lei Zhang wrote:
> Bot failed with:
> 
> ../../sandbox/linux/services/yama_unittests.cc: In function 'bool
> sandbox::{anonymous}::HasLinux32Bug()':
> ../../sandbox/linux/services/yama_unittests.cc:29:13: error: 'base::SysInfo'
has
> not been declared
> ../../sandbox/linux/services/yama_unittests.cc:30:25: error: 'base::SysInfo'
has
> not been declared
> ../../sandbox/linux/services/yama_unittests.cc:32:13: error: 'base::SysInfo'
has
> not been declared
> ../../sandbox/linux/services/yama_unittests.cc:32:79: error: 'StartsWithASCII'
> was not declared in this scope

Missing the includes?

[Nico, 2014-07-08 15:29:05]

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
sandbox/linux/services/yama_unittests.cc:27: // This is fixed in 3.4.
On 2014/07/08 05:14:57, Kees Cook wrote:
> Strictly speaking, Yama doesn't _exist_ on stock 3.2 kernels. I think it would
> be better to directly test the failure condition instead of using kernel
> version.
> 
> (i.e. calling prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) returns EINVAL
> means the feature is busted.)

So you'd write this function as

 return prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0) < 0) && errno ==
EINVAL;

?

[Nico, 2014-07-08 15:30:47]

Committed patchset #4 manually as r281757 (presubmit successful).

[eseidel, 2014-07-08 16:09:06]

This broke clang with an unused function warning?
https://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20(dbg)/bu...

[eseidel, 2014-07-08 16:09:35]

A revert of this CL has been created in
https://codereview.chromium.org/374933002/ by eseidel@chromium.org.

The reason for reverting is: Broke clang builders:
https://build.chromium.org/p/chromium.linux/builders/Linux%20Clang%20(dbg)/bu....

[Kees Cook, 2014-07-08 17:49:43]

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
File sandbox/linux/services/yama_unittests.cc (right):

https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
sandbox/linux/services/yama_unittests.cc:27: // This is fixed in 3.4.
On 2014/07/08 15:29:04, Nico (away) wrote:
> On 2014/07/08 05:14:57, Kees Cook wrote:
> > Strictly speaking, Yama doesn't _exist_ on stock 3.2 kernels. I think it
would
> > be better to directly test the failure condition instead of using kernel
> > version.
> > 
> > (i.e. calling prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) returns EINVAL
> > means the feature is busted.)
> 
> So you'd write this function as
> 
>  return prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0) < 0) && errno ==
> EINVAL;
> 
> ?

Correct. (Also note that the fix has landed in Precise's kernel tree, so the
next kernel update will include the fix.)

[Nico, 2014-07-08 17:54:23]

On 2014/07/08 17:49:43, Kees Cook wrote:
>
https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
> File sandbox/linux/services/yama_unittests.cc (right):
> 
>
https://codereview.chromium.org/371113003/diff/40001/sandbox/linux/services/y...
> sandbox/linux/services/yama_unittests.cc:27: // This is fixed in 3.4.
> On 2014/07/08 15:29:04, Nico (away) wrote:
> > On 2014/07/08 05:14:57, Kees Cook wrote:
> > > Strictly speaking, Yama doesn't _exist_ on stock 3.2 kernels. I think it
> would
> > > be better to directly test the failure condition instead of using kernel
> > > version.
> > > 
> > > (i.e. calling prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, ...) returns
EINVAL
> > > means the feature is busted.)
> > 
> > So you'd write this function as
> > 
> >  return prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY, 0, 0, 0) < 0) && errno ==
> > EINVAL;
> > 
> > ?
> 
> Correct. (Also note that the fix has landed in Precise's kernel tree, so the
> next kernel update will include the fix.)

Doing that here: https://codereview.chromium.org/376823005/