Non-SFI NaCl: Allow CLOCK_SYSTEM_TRACE on Chrome OS

Non-SFI NaCl: Allow CLOCK_SYSTEM_TRACE on Chrome OS

chrome://tracing uses TimeTicks::NowFromSystemTraceTime in
base/time/time_posix.cc.

TEST=nacl_loader_unittests  # on i686 and ARM Chrome OS
TEST=trybot
BUG=378063
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=274784

[hamaji, 2014-05-28 07:36:44]

Another option is to return EPERM instead of SIGSYS for unknown clock IDs.
TimeTicks::NowFromSystemTraceTime can fall back to CLOCK_MONOTONIC. But I
thought allowing CLOCK_SYSTEM_TRACE would be a better choice because

1. I don't know the detail of CLOCK_SYSTEM_TRACE, but this name would suggest it
would be better to stick with this clock among the whole system.
2. Our libcs and IRT return EINVAL for this clock and we'll restrict syscalls
from outside nacl_helper. So, user code won't use this anyway.

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

https://codereview.chromium.org/298163008/diff/1/sandbox/linux/services/linux...
sandbox/linux/services/linux_syscalls.h:26: #define CLOCK_SYSTEM_TRACE 11
Not sure this is the appropriate location for this constant. I put this here as
Julien said we might want to do the same check for clock_gettime even from the
base policy.

[Mark Seaborn, 2014-05-28 15:24:02]

What are the steps to reproduce?  Is it:
 * Run a Non-SFI NaCl process at the same time as using chrome://tracing,
 * on Chrome OS only?

Is there a way to automate that testing?

Is chrome://tracing actually safe to use with Non-SFI NaCl?  Does it use any
IPCs that expose information or authority that we don't want to expose to
untrusted code?

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

https://codereview.chromium.org/298163008/diff/1/sandbox/linux/services/linux...
sandbox/linux/services/linux_syscalls.h:26: #define CLOCK_SYSTEM_TRACE 11
On 2014/05/28 07:36:46, hamaji wrote:
> Not sure this is the appropriate location for this constant. I put this here
as
> Julien said we might want to do the same check for clock_gettime even from the
> base policy.

Would it make sense to change base/time/time_posix.cc to share a common
definition of this constant?

Where can I learn what CLOCK_SYSTEM_TRACE does?  time_posix.cc doesn't have any
pointers.  git annotate takes me to
https://code.google.com/p/chromium/issues/detail?id=128650, which is not
particularly informative.

I did find this:
http://code.google.com/p/chromium/issues/detail?id=324025
"ChromeOS 3.10 tracing CLOCK_SYSTEM_TRACE conflicts with CLOCK_TAI"

"The version of the CLOCK_SCHED_TRACE patch in chromeos-3.10 has
CLOCK_SYSTEM_TRACE defined at 12 instead of 11..."

-- This suggests that having a single #define would be important!

[hamaji, 2014-05-28 16:21:47]

Hi sleffler,

Though I found you are not working for Chromium anymore, we'd like to ask you to
give a pointer for CLOCK_SYSTEM_TRACE, as it seems you have introduced this to
time_posix.

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

https://codereview.chromium.org/298163008/diff/1/sandbox/linux/services/linux...
sandbox/linux/services/linux_syscalls.h:26: #define CLOCK_SYSTEM_TRACE 11
On 2014/05/28 15:24:03, Mark Seaborn wrote:
> On 2014/05/28 07:36:46, hamaji wrote:
> > Not sure this is the appropriate location for this constant. I put this here
> as
> > Julien said we might want to do the same check for clock_gettime even from
the
> > base policy.
> 
> Would it make sense to change base/time/time_posix.cc to share a common
> definition of this constant?

Sounds like a good idea. Done.

> 
> Where can I learn what CLOCK_SYSTEM_TRACE does?  time_posix.cc doesn't have
any
> pointers.  git annotate takes me to
> https://code.google.com/p/chromium/issues/detail?id=128650, which is not
> particularly informative.

I have added sleffler as a reviewer. Though it seems he is not working for
Chromium anymore, I hope he will give us a pointer.

> 
> I did find this:
> http://code.google.com/p/chromium/issues/detail?id=324025
> "ChromeOS 3.10 tracing CLOCK_SYSTEM_TRACE conflicts with CLOCK_TAI"
> 
> "The version of the CLOCK_SCHED_TRACE patch in chromeos-3.10 has
> CLOCK_SYSTEM_TRACE defined at 12 instead of 11..."
> 
> -- This suggests that having a single #define would be important!

[mdempsky, 2014-05-28 21:59:51]

Patch seems functionally correct, though I haven't had any luck understanding
what CLOCK_SYSTEM_TRACE is supposed to mean or do, so I don't understand the
consequences of allowing it in the sandbox.  It seems like probably not bad
though, since the main reason we restricted clocks at all was just to prevent
access to the per-process/thread clocks.

[hamaji, 2014-05-29 02:18:15]

I git greped the kernel and found the kernel-side bug:

https://code.google.com/p/chromium/issues/detail?id=210573

+olofj as it seems he reviewed these patches.

[Olof Johansson, 2014-05-29 03:35:13]

Care to provide some context to this email?


-Olof

2014-05-28 19:18 GMT-07:00  <hamaji@chromium.org>:
> I git greped the kernel and found the kernel-side bug:
>
> https://code.google.com/p/chromium/issues/detail?id=210573
>
> +olofj as it seems he reviewed these patches.
>
> https://codereview.chromium.org/298163008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hamaji, 2014-05-29 03:49:52]

On 2014/05/29 03:35:13, Olof Johansson wrote:
> Care to provide some context to this email?

Sorry for lacking the context. mdempsky@ tightened the seccomp-bpf filter so
that clock_gettime allows only CLOCK_REALTIME, CLOCK_MONOTONIC,
CLOCK_PROCESS_CPUTIME_ID, and CLOCK_THREAD_CPUTIME_ID. It was working on Linux
but we noticed chrome tracing on Chrome OS started crashing. Then, I realized we
need to allow CLOCK_SYSTEM_TRACE on Chrome OS. We want to know what
CLOCK_SYSTEM_TRACE is. I think we especially want to know how complex
CLOCK_SYSTEM_TRACE implementation is. If this feature is somewhat complex and
could be a security risk, we can let clock_gettime(CLOCK_SYSTEM_TRACE) return
EPERM so that TimeTicks::NowFromSystemTraceTime can fall back to
CLOCK_MONOTONIC.

[Olof Johansson, 2014-05-29 04:00:34]

2014-05-28 20:49 GMT-07:00  <hamaji@chromium.org>:
> On 2014/05/29 03:35:13, Olof Johansson wrote:
>>
>> Care to provide some context to this email?
>
>
> Sorry for lacking the context. mdempsky@ tightened the seccomp-bpf filter so
> that clock_gettime allows only CLOCK_REALTIME, CLOCK_MONOTONIC,
> CLOCK_PROCESS_CPUTIME_ID, and CLOCK_THREAD_CPUTIME_ID. It was working on
> Linux
> but we noticed chrome tracing on Chrome OS started crashing. Then, I
> realized we
> need to allow CLOCK_SYSTEM_TRACE on Chrome OS. We want to know what
> CLOCK_SYSTEM_TRACE is. I think we especially want to know how complex
> CLOCK_SYSTEM_TRACE implementation is. If this feature is somewhat complex
> and
> could be a security risk, we can let clock_gettime(CLOCK_SYSTEM_TRACE)
> return
> EPERM so that TimeTicks::NowFromSystemTraceTime can fall back to
> CLOCK_MONOTONIC.

It's simple. Code is kernel/trace/trace.c: trace_clock_get{res,time}()
and the plumbing is in kernel/posix-timers.c

Exposing it shouldn't be controversial.


-Olof

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hamaji, 2014-05-29 04:06:32]

Thanks!

This is the link to the code. My ARM Chrome book says its linux version is
3.8.11, so I chose "chromeos-3.8" branch.

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-3....

[jln (very slow on Chromium), 2014-05-29 22:49:00]

If Olof doesn't think this does not create to much attack surface, I think we
can allow it.

Is tracing really useful for Non-SFI NaCl though? Because if not, I would prefer
to EPERM it instead, since you mention that there is a fallback to
CLOCK_MONOTONIC that would preserve at least basic functionality.

[hamaji, 2014-05-30 02:09:22]

On 2014/05/29 22:49:00, jln wrote:
> If Olof doesn't think this does not create to much attack surface, I think we
> can allow it.
> 
> Is tracing really useful for Non-SFI NaCl though? Because if not, I would
prefer
> to EPERM it instead, since you mention that there is a fallback to
> CLOCK_MONOTONIC that would preserve at least basic functionality.

I'm not sure if you are asking the usefulness of chrome://tracing or
CLOCK_SYSTEM_TRACE. For the former question, yes, we are heavily relying on
chrome://tracing. I don't know about the latter. chrome://tracing on non-SFI
NaCl ARM looks working even with EPERM (so that chrome://tracing is falling back
to CLOCK_MONOTONIC only in non-SFI nacl_helper). I might be missing small
glitches, though.

Olof, could you tell us the benefit of CLOCK_SYSTEM_TRACE? From its name, I'm
guessing it would provide more consistent timing info across the whole system?
If falling back to CLOCK_MONOTONIC (while the rest of Chrome keeps using
CLOCK_SYSTEM_TRACE) could increase the error by a few milliseconds for example,
I think we would want CLOCK_SYSTEM_TRACE.

[hamaji, 2014-05-30 02:35:16]

BTW, this is a P1 issue for us :) In case we decide to go with EPERM while I'm
not working, I created another version of this change:

https://codereview.chromium.org/307033002/

If either of them looks good for some of you, could someone add a base/ owner as
a reviewer?

[jln (very slow on Chromium), 2014-05-30 07:31:26]

Yeah, lgtm, feel free to allow CLOCK_SYSTEM_TRACE.

On Thu, May 29, 2014 at 7:35 PM,  <hamaji@chromium.org> wrote:
> BTW, this is a P1 issue for us :) In case we decide to go with EPERM while
> I'm
> not working, I created another version of this change:
>
> https://codereview.chromium.org/307033002/
>
> If either of them looks good for some of you, could someone add a base/
> owner as
> a reviewer?
>
>
> https://codereview.chromium.org/298163008/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Mark Seaborn, 2014-05-30 16:38:20]

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h#newcode580
base/time/time.h:580: #if defined(OS_LINUX) || defined(OS_CHROMEOS)
Shouldn't this be "#if defined(OS_LINUX)", since OS_CHROMEOS implies OS_LINUX?

[Olof Johansson, 2014-05-31 03:15:29]

2014-05-29 19:09 GMT-07:00  <hamaji@chromium.org>:
> On 2014/05/29 22:49:00, jln wrote:
>>
>> If Olof doesn't think this does not create to much attack surface, I think
>> we
>> can allow it.
>
>
>> Is tracing really useful for Non-SFI NaCl though? Because if not, I would
>
> prefer
>>
>> to EPERM it instead, since you mention that there is a fallback to
>> CLOCK_MONOTONIC that would preserve at least basic functionality.
>
>
> I'm not sure if you are asking the usefulness of chrome://tracing or
> CLOCK_SYSTEM_TRACE. For the former question, yes, we are heavily relying on
> chrome://tracing. I don't know about the latter. chrome://tracing on non-SFI
> NaCl ARM looks working even with EPERM (so that chrome://tracing is falling
> back
> to CLOCK_MONOTONIC only in non-SFI nacl_helper). I might be missing small
> glitches, though.
>
> Olof, could you tell us the benefit of CLOCK_SYSTEM_TRACE? From its name,
> I'm
> guessing it would provide more consistent timing info across the whole
> system?

it is needed to coordinate timestamps of Chrome-level trace events
with system/kernel-level trace events in about:tracing.

> If falling back to CLOCK_MONOTONIC (while the rest of Chrome keeps using
> CLOCK_SYSTEM_TRACE) could increase the error by a few milliseconds for
> example,
> I think we would want CLOCK_SYSTEM_TRACE.

Yes, you likely want it.


-Olof

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[hamaji, 2014-06-02 08:15:03]

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h#newcode580
base/time/time.h:580: #if defined(OS_LINUX) || defined(OS_CHROMEOS)
On 2014/05/30 16:38:20, Mark Seaborn wrote:
> Shouldn't this be "#if defined(OS_LINUX)", since OS_CHROMEOS implies OS_LINUX?

I tried OS_LINUX-only first and I was surprised the build failed. It seems
build_nexe.py sets only OS_CHROMEOS, without any other OS_*. If we remove
OS_CHROMEOS from here, we need !defined(__native_client__) in time_posix.cc.

I'll upload the version which does not specify OS_CHROMEOS here, but I'm not
sure if this is the right approach. I'm not sure if build_nexe.py should specify
OS_CHROMEOS.

[hamaji, 2014-06-02 08:15:49]

> > Olof, could you tell us the benefit of CLOCK_SYSTEM_TRACE? From its name,
> > I'm
> > guessing it would provide more consistent timing info across the whole
> > system?
> 
> it is needed to coordinate timestamps of Chrome-level trace events
> with system/kernel-level trace events in about:tracing.
> 
> > If falling back to CLOCK_MONOTONIC (while the rest of Chrome keeps using
> > CLOCK_SYSTEM_TRACE) could increase the error by a few milliseconds for
> > example,
> > I think we would want CLOCK_SYSTEM_TRACE.
> 
> Yes, you likely want it.

Thanks for the info!

[hamaji, 2014-06-03 05:40:15]

Ping?

[jln (very slow on Chromium), 2014-06-03 06:15:44]

lgtm

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc#...
base/time/time_posix.cc:332: if (clock_gettime(kClockSystemTrace, &ts) != 0) {
If you want to, you can also COMPILE_ASSERT that kClockSystemTrace ==
CLOCK_SYSTEM_TRACE here.

[hamaji, 2014-06-03 06:26:48]

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc#...
base/time/time_posix.cc:332: if (clock_gettime(kClockSystemTrace, &ts) != 0) {
On 2014/06/03 06:15:44, jln wrote:
> If you want to, you can also COMPILE_ASSERT that kClockSystemTrace ==
> CLOCK_SYSTEM_TRACE here.

Hmm I don't understand your suggestion. As written in the original comment (now
this comment is moved to time.h), CLOCK_SYSTEM_TRACE is not defined in glibc
header. This is why we define this magic number in Chromium source. If we have
CLOCK_SYSTEM_TRACE, we can just use it instead of defining it by ourselves. So,
I think we cannot check the value.

[Mark Seaborn, 2014-06-03 17:23:27]

LGTM, with the comment fix below, somewhat reluctantly because there's no
end-to-end automated test to check that tracing works.

Without an end-to-end test, this is fairly likely to regress in the future.

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h
File base/time/time.h (right):

https://codereview.chromium.org/298163008/diff/40001/base/time/time.h#newcode580
base/time/time.h:580: #if defined(OS_LINUX) || defined(OS_CHROMEOS)
On 2014/06/02 08:15:04, hamaji wrote:
> On 2014/05/30 16:38:20, Mark Seaborn wrote:
> > Shouldn't this be "#if defined(OS_LINUX)", since OS_CHROMEOS implies
OS_LINUX?
> 
> I tried OS_LINUX-only first and I was surprised the build failed. It seems
> build_nexe.py sets only OS_CHROMEOS, without any other OS_*.

What part of the code is setting OS_CHROMEOS?  build_nexe.py itself doesn't do
that -- nothing in the NaCl repo mentions OS_CHROMEOS.

OS_CHROMEOS shouldn't even be defined when compiling untrusted code, which
should only get OS_NACL.

I suppose it's this part in build/common.gypi?
      ['chromeos==1', {
        'defines': ['OS_CHROMEOS=1'],
      }],

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc#...
base/time/time_posix.cc:324: // ID. build_nexe.py sets OS_CHROMEOS without any
other OS_* macros so
Maybe say it's common.gypi that does that instead of or as well as mentioning
build_nexe.py?

There should really be a TODO here to fix this #define.

[hamaji, 2014-06-03 18:44:08]

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc#...
base/time/time_posix.cc:324: // ID. build_nexe.py sets OS_CHROMEOS without any
other OS_* macros so
On 2014/06/03 17:23:28, Mark Seaborn wrote:
> Maybe say it's common.gypi that does that instead of or as well as mentioning
> build_nexe.py?
> 
> There should really be a TODO here to fix this #define.

Yeah, you are right. Sorry for the wrong description. Update the comment and
added a TODO.

[jln (very slow on Chromium), 2014-06-03 19:10:28]

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc
File base/time/time_posix.cc (right):

https://codereview.chromium.org/298163008/diff/60001/base/time/time_posix.cc#...
base/time/time_posix.cc:332: if (clock_gettime(kClockSystemTrace, &ts) != 0) {
On 2014/06/03 06:26:49, hamaji wrote:
> On 2014/06/03 06:15:44, jln wrote:
> > If you want to, you can also COMPILE_ASSERT that kClockSystemTrace ==
> > CLOCK_SYSTEM_TRACE here.
> 
> Hmm I don't understand your suggestion. As written in the original comment
(now
> this comment is moved to time.h), CLOCK_SYSTEM_TRACE is not defined in glibc
> header. This is why we define this magic number in Chromium source. If we have
> CLOCK_SYSTEM_TRACE, we can just use it instead of defining it by ourselves.
So,
> I think we cannot check the value.

Ahh, sorry I missed that CLOCK_SYSTEM_TRACE was in a #define above. I thought it
was defined in this file but you had trouble getting the right include in
components/

[awong, 2014-06-04 01:39:10]

base LGTM

[hamaji, 2014-06-04 01:40:11]

The CQ bit was checked by hamaji@chromium.org

[commit-bot: I haz the power, 2014-06-04 01:40:32]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/hamaji@chromium.org/298163008/80001

[commit-bot: I haz the power, 2014-06-04 12:16:32]

Change committed as 274784