Re: [PATCH v4 0/2] Control over userfaultfd kernel-fault handling

[Nick Kralevich]

map) {
> 
> perror("mmap");
> 
>
> The whole point of being able to reproduce on 4.5.1 is then to
> simulate if the same exploit would also reproduce on current kernels
> with all enterprise default robustness features enabled. Or did
> anybody already verify it?
>
> Anyway the links I was playing with are all in the cover letter, the
> cover letter is not as important as the actual patches. The actual
> patches looks fine to me.

That's great to hear.

>
> The only improvement I can think of is, what about to add a
> printk_once to suggest to toggle the sysctl if userfaultfd bails out
> because the process lacks the CAP_SYS_PTRACE capability? That would
> facilitate the /etc/sysctl.conf or tuned tweaking in case the apps
> aren't verbose enough.
>
> It's not relevant anymore with this latest patchset, but about the
> previous argument that seccomp couldn't be used in all Android
> processes because of performance concern, I'm slightly confused.

Seccomp causes more problems than just performance. Seccomp is not
designed for whole-of-system protections. Please see my other writeup
at 
https://lore.kernel.org/lkml/cafj0lneo-7yuvgohb4phteuiuw+wpfzqbwxucgaa35zmx11...@mail.gmail.com/

>
> https://android-developers.googleblog.com/2017/07/seccomp-filter-in-android-o.html
>
> "Android O includes a single seccomp filter installed into zygote, the
> process from which all the Android applications are derived. Because
> the filter is installed into zygote—and therefore all apps—the Android
> security team took extra caution to not break existing apps"
>
> Example:
>
> $ uname -mo
> aarch64 Android
> $ cat swapoff.c
> #include 
>
> int main()
> {
> swapoff("");
> }
> $ gcc swapoff.c -o swapoff -O2
> $ ./swapoff
> Bad system call
> $
>
> It's hard to imagine what is more performance critical than the zygote
> process and the actual apps as above?
>
> It's also hard to imagine what kind of performance concern can arise
> by adding seccomp filters also to background system apps that
> generally should consume ~0% of CPU.
>
> If performance is really a concern, the BPF JIT representation with
> the bitmap to be able to run the filter in O(1) sounds a better
> solution than not adding ad-hoc filters and it's being worked on for
> x86-64 and can be ported to aarch64 too. Many of the standalone
> background processes likely wouldn't even use uffd at all so you could
> block the user initiated faults too that way.
>
> Ultimately because of issues as [3] (be them still relevant or not, to
> be double checked), no matter if through selinux, seccomp or a
> different sysctl value, without this patchset applied the default
> behavior of the userfaultfd syscall for all Linux binaries running on
> Android kernels, would deviate from the upstream kernel. So even if we
> would make the pipe mutex logic more complex the deviation would
> remain. Your patchset adds much less risk of breakage than adding a
> timeout to kernel initiated userfaults and it resolves all concerns as
> well as a timeout. We'll also make better use of the "0" value this
> way. So while I'm not certain this is the best for the long term, this
> looks the sweet spot for the short term to resolve many issues at
> once.
>
> Thanks!
> Andrea
>


-- 
Nick Kralevich | n...@google.com

Re: [PATCH 2/2] Add a new sysctl knob: unprivileged_userfaultfd_user_mode_only

[Nick Kralevich]

esh mentioned above:

* 
https://lore.kernel.org/lkml/CABXk95A-E4NYqA5qVrPgDF18YW-z4_udzLwa0cdo2OfqVsy=s...@mail.gmail.com/
* 
https://lore.kernel.org/lkml/CAFJ0LnGfrzvVgtyZQ+UqRM6F3M7iXOhTkUBTc+9sV+=rrfn...@mail.gmail.com/
* https://lore.kernel.org/lkml/202005200921.2BD5A0ADD@keescook/

Thanks,
-- Nick

[1] The use of the term "unprivileged" is unfortunate. In Android,
there's no coarse-grain privileged vs unprivileged process. Each
process, including root processes, have only the privileges they need,
and not a bit more. As a concrete example, Android's init process
(PID=1) is not allowed to open TCP/UDP sockets, but is allowed to
spawn children which can do so. Having each process be differently
privileged, and ensuring that functionality is only given out on a
need-to-have basis, is an important part of modern OS design.

[2] The trend in modern exploits isn't to perform attacks directly
from untrusted code to the kernel. A lot of the attack surface needed
by an attacker isn't reachable directly from untrusted code, but only
indirectly through other processes. The attacker moves laterally
through the system, exploiting a process which has the necessary
capabilities, then escalating to the kernel. Enforcing security
controls system-wide is an important part of denying an attacker the
tools for an effective exploit and preventing this kind of lateral
movement from being useful. Denying an attacker access to kernel
initiated faults in userfaultfd system-wide (except for authorized
processes) is doubly important, as these kinds of faults are extremely
valuable to an exploit writer (see explanation at
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cefdca0a86be517bc390fc4541e3674b8e7803b0
or https://duasynt.com/blog/cve-2016-6187-heap-off-by-one-exploit)

[3] 
https://android.googlesource.com/platform/system/sepolicy/+/7be9e9e372c70a5518f729a0cdcb0d39a28be377/private/domain.te#107
line 107

-- 
Nick Kralevich | n...@google.com

Re: [PATCH 2/2] Add a new sysctl knob: unprivileged_userfaultfd_user_mode_only

[Nick Kralevich]

On Fri, Jul 24, 2020 at 6:40 AM Michael S. Tsirkin  wrote:
>
> On Thu, Jul 23, 2020 at 05:13:28PM -0700, Nick Kralevich wrote:
> > On Thu, Jul 23, 2020 at 10:30 AM Lokesh Gidra  
> > wrote:
> > > From the discussion so far it seems that there is a consensus that
> > > patch 1/2 in this series should be upstreamed in any case. Is there
> > > anything that is pending on that patch?
> >
> > That's my reading of this thread too.
> >
> > > > > Unless I'm mistaken that you can already enforce bit 1 of the second
> > > > > parameter of the userfaultfd syscall to be set with seccomp-bpf, this
> > > > > would be more a question to the Android userland team.
> > > > >
> > > > > The question would be: does it ever happen that a seccomp filter isn't
> > > > > already applied to unprivileged software running without
> > > > > SYS_CAP_PTRACE capability?
> > > >
> > > > Yes.
> > > >
> > > > Android uses selinux as our primary sandboxing mechanism. We do use
> > > > seccomp on a few processes, but we have found that it has a
> > > > surprisingly high performance cost [1] on arm64 devices so turning it
> > > > on system wide is not a good option.
> > > >
> > > > [1] 
> > > > https://lore.kernel.org/linux-security-module/20200606.3F7109A@keescook/T/#m82ace19539ac595682affabdf652c0ffa5d27dad
> >
> > As Jeff mentioned, seccomp is used strategically on Android, but is
> > not applied to all processes. It's too expensive and impractical when
> > simpler implementations (such as this sysctl) can exist. It's also
> > significantly simpler to test a sysctl value for correctness as
> > opposed to a seccomp filter.
>
> Given that selinux is already used system-wide on Android, what is wrong
> with using selinux to control userfaultfd as opposed to seccomp?

Userfaultfd file descriptors will be generally controlled by SELinux.
You can see the patchset at
https://lore.kernel.org/lkml/20200401213903.182112-3-dan...@google.com/
(which is also referenced in the original commit message for this
patchset). However, the SELinux patchset doesn't include the ability
to control FAULT_FLAG_USER / UFFD_USER_MODE_ONLY directly.

SELinux already has the ability to control who gets CAP_SYS_PTRACE,
which combined with this patch, is largely equivalent to direct
UFFD_USER_MODE_ONLY checks. Additionally, with the SELinux patch
above, movement of userfaultfd file descriptors can be mediated by
SELinux, preventing one process from acquiring userfaultfd descriptors
of other processes unless allowed by security policy.

It's an interesting question whether finer-grain SELinux support for
controlling UFFD_USER_MODE_ONLY should be added. I can see some
advantages to implementing this. However, we don't need to decide that
now.

Kernel security checks generally break down into DAC (discretionary
access control) and MAC (mandatory access control) controls. Most
kernel security features check via both of these mechanisms. Security
attributes of the system should be settable without necessarily
relying on an LSM such as SELinux. This patch follows the same basic
model -- system wide control of a hardening feature is provided by the
unprivileged_userfaultfd_user_mode_only sysctl (DAC), and if needed,
SELinux support for this can also be implemented on top of the DAC
controls.

This DAC/MAC split has been successful in several other security
features. For example, the ability to map at page zero is controlled
in DAC via the mmap_min_addr sysctl [1], and via SELinux via the
mmap_zero access vector [2]. Similarly, access to the kernel ring
buffer is controlled both via DAC as the dmesg_restrict sysctl [3], as
well as the SELinux syslog_read [2] check. Indeed, the dmesg_restrict
sysctl is very similar to this patch -- it introduces a capability
(CAP_SYSLOG, CAP_SYS_PTRACE) check on access to a sensitive resource.

If we want to ensure that a security feature will be well tested and
vetted, it's important to not limit its use to LSMs only. This ensures
that kernel and application developers will always be able to test the
effects of a security feature, without relying on LSMs like SELinux.
It also ensures that all distributions can enable this security
mitigation should it be necessary for their unique environments,
without introducing an SELinux dependency. And this patch does not
preclude an SELinux implementation should it be necessary.

Even if we decide to implement fine-grain SELinux controls on
UFFD_USER_MODE_ONLY, we still need this patch. We shouldn't make this
an either/or choice between SELinux and this patch. Both are
necessary.

-- Nick

[1] https://wiki.debian.org/mmap_min_addr
[2] https://selinuxproject.org/page/NB_

Re: [PATCH 2/2] Add a new sysctl knob: unprivileged_userfaultfd_user_mode_only

[Nick Kralevich]

On Thu, Jul 23, 2020 at 10:30 AM Lokesh Gidra  wrote:
> From the discussion so far it seems that there is a consensus that
> patch 1/2 in this series should be upstreamed in any case. Is there
> anything that is pending on that patch?

That's my reading of this thread too.

> > > Unless I'm mistaken that you can already enforce bit 1 of the second
> > > parameter of the userfaultfd syscall to be set with seccomp-bpf, this
> > > would be more a question to the Android userland team.
> > >
> > > The question would be: does it ever happen that a seccomp filter isn't
> > > already applied to unprivileged software running without
> > > SYS_CAP_PTRACE capability?
> >
> > Yes.
> >
> > Android uses selinux as our primary sandboxing mechanism. We do use
> > seccomp on a few processes, but we have found that it has a
> > surprisingly high performance cost [1] on arm64 devices so turning it
> > on system wide is not a good option.
> >
> > [1] 
> > https://lore.kernel.org/linux-security-module/20200606.3F7109A@keescook/T/#m82ace19539ac595682affabdf652c0ffa5d27dad

As Jeff mentioned, seccomp is used strategically on Android, but is
not applied to all processes. It's too expensive and impractical when
simpler implementations (such as this sysctl) can exist. It's also
significantly simpler to test a sysctl value for correctness as
opposed to a seccomp filter.

> > >
> > >
> > > If answer is "no" the behavior of the new sysctl in patch 2/2 (in
> > > subject) should be enforceable with minor changes to the BPF
> > > assembly. Otherwise it'd require more changes.

It would be good to understand what these changes are.

> > > Why exactly is it preferable to enlarge the surface of attack of the
> > > kernel and take the risk there is a real bug in userfaultfd code (not
> > > just a facilitation of exploiting some other kernel bug) that leads to
> > > a privilege escalation, when you still break 99% of userfaultfd users,
> > > if you set with option "2"?

I can see your point if you think about the feature as a whole.
However, distributions (such as Android) have specialized knowledge of
their security environments, and may not want to support the typical
usages of userfaultfd. For such distributions, providing a mechanism
to prevent userfaultfd from being useful as an exploit primitive,
while still allowing the very limited use of userfaultfd for userspace
faults only, is desirable. Distributions shouldn't be forced into
supporting 100% of the use cases envisioned by userfaultfd when their
needs may be more specialized, and this sysctl knob empowers
distributions to make this choice for themselves.

> > > Is the system owner really going to purely run on his systems CRIU
> > > postcopy live migration (which already runs with CAP_SYS_PTRACE) and
> > > nothing else that could break?

This is a great example of a capability which a distribution may not
want to support, due to distribution specific security policies.

> > >
> > > Option "2" to me looks with a single possible user, and incidentally
> > > this single user can already enforce model "2" by only tweaking its
> > > seccomp-bpf filters without applying 2/2. It'd be a bug if android
> > > apps runs unprotected by seccomp regardless of 2/2.

Can you elaborate on what bug is present by processes being
unprotected by seccomp?

Seccomp cannot be universally applied on Android due to previously
mentioned performance concerns. Seccomp is used in Android primarily
as a tool to enforce the list of allowed syscalls, so that such
syscalls can be audited before being included as part of the Android
API.

-- Nick

-- 
Nick Kralevich | n...@google.com

Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Nick Kralevich]

On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin <nico...@belouin.fr> wrote:
> In its current implementation the check is against CAP_SYS_ADMIN,
> however this capability is bloated and inapropriate for this use.
> Indeed the check aims to avoid dedupe against non writable files,
> falling directly in the use case of CAP_DAC_OVERRIDE.
>
> Signed-off-by: Nicolas Belouin <nico...@belouin.fr>
> ---
>  fs/read_write.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index f0d4b16873e8..43cc7e84e29e 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1965,7 +1965,7 @@ int vfs_dedupe_file_range(struct file *file, struct 
> file_dedupe_range *same)
> u64 len;
> int i;
> int ret;
> -   bool is_admin = capable(CAP_SYS_ADMIN);
> +   bool is_admin = capable(CAP_SYS_ADMIN) || capable(CAP_DAC_OVERRIDE);

Can you please reverse the order of the checks? In particular, on an
SELinux based system, a capable() call generates an SELinux denial,
and people often instinctively allow the first operation performed.
Reordering the elements will ensure that the CAP_DAC_OVERRIDE denial
(least permissive) is generated first.

> u16 count = same->dest_count;
>     struct file *dst_file;
> loff_t dst_off;
> --
> 2.14.2
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] [PATCH] fs: Use CAP_DAC_OVERRIDE to allow for file dedupe

[Nick Kralevich]

On Sat, Oct 21, 2017 at 6:28 AM, Nicolas Belouin  wrote:
> In its current implementation the check is against CAP_SYS_ADMIN,
> however this capability is bloated and inapropriate for this use.
> Indeed the check aims to avoid dedupe against non writable files,
> falling directly in the use case of CAP_DAC_OVERRIDE.
>
> Signed-off-by: Nicolas Belouin 
> ---
>  fs/read_write.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index f0d4b16873e8..43cc7e84e29e 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1965,7 +1965,7 @@ int vfs_dedupe_file_range(struct file *file, struct 
> file_dedupe_range *same)
> u64 len;
> int i;
> int ret;
> -   bool is_admin = capable(CAP_SYS_ADMIN);
> +   bool is_admin = capable(CAP_SYS_ADMIN) || capable(CAP_DAC_OVERRIDE);

Can you please reverse the order of the checks? In particular, on an
SELinux based system, a capable() call generates an SELinux denial,
and people often instinctively allow the first operation performed.
Reordering the elements will ensure that the CAP_DAC_OVERRIDE denial
(least permissive) is generated first.

> u16 count = same->dest_count;
> struct file *dst_file;
> loff_t dst_off;
> --
> 2.14.2
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] Re: [PATCHv3 2/2] extract early boot entropy from the passed cmdline

[Nick Kralevich]

On Wed, Aug 30, 2017 at 2:57 AM, Pavel Machek <pa...@ucw.cz> wrote:
> The command line is visible to unpriviledged userspace (/proc/cmdline,
> dmesg). Is that a problem?

These files are not exposed to untrusted processes on Android.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] Re: [PATCHv3 2/2] extract early boot entropy from the passed cmdline

[Nick Kralevich]

On Wed, Aug 30, 2017 at 2:57 AM, Pavel Machek  wrote:
> The command line is visible to unpriviledged userspace (/proc/cmdline,
> dmesg). Is that a problem?

These files are not exposed to untrusted processes on Android.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] [PATCHv2 2/2] extract early boot entropy from the passed cmdline

[Nick Kralevich]

On Wed, Aug 16, 2017 at 3:46 PM, Laura Abbott  wrote:
> From: Daniel Micay 
>
> Existing Android bootloaders usually pass data useful as early entropy
> on the kernel command-line. It may also be the case on other embedded
> systems. Sample command-line from a Google Pixel running CopperheadOS:
>

Why is it better to put this into the kernel, rather than just rely on
the existing userspace functionality which does exactly the same
thing? This is what Android already does today:
https://android-review.googlesource.com/198113

-- Nick

Re: [kernel-hardening] [PATCHv2 2/2] extract early boot entropy from the passed cmdline

[Nick Kralevich]

On Wed, Aug 16, 2017 at 3:46 PM, Laura Abbott  wrote:
> From: Daniel Micay 
>
> Existing Android bootloaders usually pass data useful as early entropy
> on the kernel command-line. It may also be the case on other embedded
> systems. Sample command-line from a Google Pixel running CopperheadOS:
>

Why is it better to put this into the kernel, rather than just rely on
the existing userspace functionality which does exactly the same
thing? This is what Android already does today:
https://android-review.googlesource.com/198113

-- Nick

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Nick Kralevich]

On Fri, Jun 2, 2017 at 1:05 PM, Alan Cox <gno...@lxorguk.ukuu.org.uk> wrote:
> So I'd say it's all the generic tty ioctls except TIOCSTI and TIOCSETD
> but it would be good to see what Android is going with and why.

Android limits tty ioctls to the following whitelist:
  TIOCOUTQ
  FIOCLEX
  TCGETS
  TCSETS
  TIOCGWINSZ
  TIOCSWINSZ
  TIOCSCTTY
  TCSETSW
  TCFLSH
  TIOCSPGRP
  TIOCGPGRP

See unpriv_tty_ioctls at
https://android.googlesource.com/platform/system/sepolicy/+/34b4b73729b288b4109b2225c1445eb58393b8cb/public/ioctl_macros#51


-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Nick Kralevich]

On Fri, Jun 2, 2017 at 1:05 PM, Alan Cox  wrote:
> So I'd say it's all the generic tty ioctls except TIOCSTI and TIOCSETD
> but it would be good to see what Android is going with and why.

Android limits tty ioctls to the following whitelist:
  TIOCOUTQ
  FIOCLEX
  TCGETS
  TCSETS
  TIOCGWINSZ
  TIOCSWINSZ
  TIOCSCTTY
  TCSETSW
  TCFLSH
  TIOCSPGRP
  TIOCGPGRP

See unpriv_tty_ioctls at
https://android.googlesource.com/platform/system/sepolicy/+/34b4b73729b288b4109b2225c1445eb58393b8cb/public/ioctl_macros#51


-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Nick Kralevich]

On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley  wrote:
>> Seccomp requires the program in question to "opt-in" so to speak and
>> set
>> certain restrictions on itself. However as you state above, any
>> TIOCSTI
>> protection doesn't matter if the program correctly allocates a
>> tty/pty pair.
>> This protections seeks to protect users from programs that don't do
>> things
>> correctly. Rather than killing bugs, this feature attempts to kill an
>> entire
>> bug class that shows little sign of slowing down in the world of
>> containers and
>> sandboxes.
>
> Just FYI, you can also restrict TIOCSTI (or any other ioctl command)
> via SELinux ioctl whitelisting, and Android is using that feature to
> restrict TIOCSTI usage in Android O (at least based on the developer
> previews to date, also in AOSP master).

For reference, this is https://android-review.googlesource.com/306278
, where we moved to a whitelist for handling ioctls for ptys.

-- Nick

Re: [kernel-hardening] Re: [PATCH v7 2/2] security: tty: make TIOCSTI ioctl require CAP_SYS_ADMIN

[Nick Kralevich]

On Tue, May 30, 2017 at 11:32 AM, Stephen Smalley  wrote:
>> Seccomp requires the program in question to "opt-in" so to speak and
>> set
>> certain restrictions on itself. However as you state above, any
>> TIOCSTI
>> protection doesn't matter if the program correctly allocates a
>> tty/pty pair.
>> This protections seeks to protect users from programs that don't do
>> things
>> correctly. Rather than killing bugs, this feature attempts to kill an
>> entire
>> bug class that shows little sign of slowing down in the world of
>> containers and
>> sandboxes.
>
> Just FYI, you can also restrict TIOCSTI (or any other ioctl command)
> via SELinux ioctl whitelisting, and Android is using that feature to
> restrict TIOCSTI usage in Android O (at least based on the developer
> previews to date, also in AOSP master).

For reference, this is https://android-review.googlesource.com/306278
, where we moved to a whitelist for handling ioctls for ptys.

-- Nick

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley <s...@tycho.nsa.gov> wrote:
>> I can reproduce it on angler (with a back-port of just that patch),
>> although I am unclear on the cause.  The patch is only supposed to
>> enable explicit setting of security labels by userspace on cgroup
>> files, so it isn't supposed to cause any breakage under existing
>> policy.  Prior to the patch, the kernel would always just return -1
>> with errno EOPNOTSUPP upon attempts to set security labels on cgroup
>> files; with the patch, the kernel may instead return -1 with errno
>> EACCES if not allowed.  So I suppose if userspace was explicitly
>> testing for EOPNOTSUPP and not failing hard in that case, it might
>> cause breakage.  Not sure why existing userspace would be trying to
>> relabel cgroup files, unless it is just a recursive restorecon that
>> happens to traverse into a cgroup mount (and in that case, not sure
>> why
>> it would be fatal).  Other possible interaction would be use of
>> setfscreatecon() prior to creating a file in cgroup.
>
> Oh, I see - it is the latter.
>
> For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive, which
> internally looks up the context for that directory from file_contexts
> and does a setfscreatecon() followed by a mkdir().  Previously, that
> was ignored because cgroup did not support anything other than the
> policy-defined label.  But now it will try to use that label, which in
> turn will trigger a denial in enforcing mode and the create will fail.
>
> So this is an incompatible change and needs to be reverted.
> We'll need to wrap it up with a policy capability or something to allow
> it to be enabled only if the policy correctly supports it.  Even
> better, we should instead just allow the policy to specify which
> filesystems should support this behavior (already on the issues list).
>

If Android is the only system affected by this bug, I would prefer to
just fix Android to allow for this patch, rather than having
additional kernel complexity.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

On Mon, Feb 27, 2017 at 11:53 AM, Stephen Smalley  wrote:
>> I can reproduce it on angler (with a back-port of just that patch),
>> although I am unclear on the cause.  The patch is only supposed to
>> enable explicit setting of security labels by userspace on cgroup
>> files, so it isn't supposed to cause any breakage under existing
>> policy.  Prior to the patch, the kernel would always just return -1
>> with errno EOPNOTSUPP upon attempts to set security labels on cgroup
>> files; with the patch, the kernel may instead return -1 with errno
>> EACCES if not allowed.  So I suppose if userspace was explicitly
>> testing for EOPNOTSUPP and not failing hard in that case, it might
>> cause breakage.  Not sure why existing userspace would be trying to
>> relabel cgroup files, unless it is just a recursive restorecon that
>> happens to traverse into a cgroup mount (and in that case, not sure
>> why
>> it would be fatal).  Other possible interaction would be use of
>> setfscreatecon() prior to creating a file in cgroup.
>
> Oh, I see - it is the latter.
>
> For example, init.rc does mkdir /dev/cpuctl/bg_non_interactive, which
> internally looks up the context for that directory from file_contexts
> and does a setfscreatecon() followed by a mkdir().  Previously, that
> was ignored because cgroup did not support anything other than the
> policy-defined label.  But now it will try to use that label, which in
> turn will trigger a denial in enforcing mode and the create will fail.
>
> So this is an incompatible change and needs to be reverted.
> We'll need to wrap it up with a policy capability or something to allow
> it to be enabled only if the policy correctly supports it.  Even
> better, we should instead just allow the policy to specify which
> filesystems should support this behavior (already on the issues list).
>

If Android is the only system affected by this bug, I would prefer to
just fix Android to allow for this patch, rather than having
additional kernel complexity.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

Can you try adding the androidboot.selinux=permissive line to the kernel
command line, to boot in permissive mode? I suspect the policy just needs
to be adjusted.

-- Nick

On Fri, Feb 24, 2017 at 6:01 PM, John Stultz <john.stu...@linaro.org> wrote:
> On Thu, Feb 23, 2017 at 4:01 PM, Paul Moore <p...@paul-moore.com> wrote:
>> On Thu, Feb 23, 2017 at 1:43 PM, John Stultz <john.stu...@linaro.org> wrote:
>>> Hey folks,
>>>I've not been able to figure out why yet, but I wanted to raise the
>>> issue that last night I found I couldn't boot Android on my Hikey
>>> board with Linus' HEAD kernel. It seems to cause logd to crash
>>> repeatedly so I'm not able to get debug info from logcat.
>>>
>>> I do see the following over and over on the console:
>>>
>>> [   12.505838] init: computing context for service 'logd'
>>> [   12.506355] init: starting service 'logd'...
>>> [   12.507683] init: property_set("ro.boottime.logd", "12500792498")
>>> failed: property already set
>>> [   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
>>> 1036, group 1036
>>> [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
>>> user 1036, group 1036
>>> [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
>>> user 1036, group 1036
>>> [   12.510132] init: Opened file '/proc/kmsg', flags 0
>>> [   12.510187] init: Opened file '/dev/kmsg', flags 1
>>> [   12.510353] init: couldn't write 1941 to
>>> /dev/cpuset/system-background/tasks: No such file or directory
>>> [   12.533046] init: Service 'logd' (pid 1941) exited with status 255
>>>
>>>
>>> I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
>>> allow changing labels for cgroupfs"), which was merged in yesterday.
>>> I've not yet been able to figure out the root cause, but reverting
>>> that patch makes things work again.
>>>
>>> So I wanted to raise the issue here so folks were aware.
>>>
>>> If there is anything folks want me to test or try, please let me know.
>>
>> Unfortunately I don't have an Android test system to play with, have
>> any of the SEAndroid folks on the To/CC line seen a similar problem?
>
> So from my very limited knowledge here, adding the patch in question
> seems to make the cgroup mount get the SBLABEL_MNT flag?
> Which I'm guessing this is causing additional selinux restrictions on
> processes accessing cgroup mounts, which causes some of the early
> initialization processes to fail?
>
> Should this change mean the selinux policy needs to be updated?
>
> thanks
> -john



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [Regression?] 1ea0ce4069 ("selinux: allow changing labels for cgroupfs") stops Android from booting

[Nick Kralevich]

Can you try adding the androidboot.selinux=permissive line to the kernel
command line, to boot in permissive mode? I suspect the policy just needs
to be adjusted.

-- Nick

On Fri, Feb 24, 2017 at 6:01 PM, John Stultz  wrote:
> On Thu, Feb 23, 2017 at 4:01 PM, Paul Moore  wrote:
>> On Thu, Feb 23, 2017 at 1:43 PM, John Stultz  wrote:
>>> Hey folks,
>>>I've not been able to figure out why yet, but I wanted to raise the
>>> issue that last night I found I couldn't boot Android on my Hikey
>>> board with Linus' HEAD kernel. It seems to cause logd to crash
>>> repeatedly so I'm not able to get debug info from logcat.
>>>
>>> I do see the following over and over on the console:
>>>
>>> [   12.505838] init: computing context for service 'logd'
>>> [   12.506355] init: starting service 'logd'...
>>> [   12.507683] init: property_set("ro.boottime.logd", "12500792498")
>>> failed: property already set
>>> [   12.508701] init: Created socket '/dev/socket/logd', mode 666, user
>>> 1036, group 1036
>>> [   12.509294] init: Created socket '/dev/socket/logdr', mode 666,
>>> user 1036, group 1036
>>> [   12.509891] init: Created socket '/dev/socket/logdw', mode 222,
>>> user 1036, group 1036
>>> [   12.510132] init: Opened file '/proc/kmsg', flags 0
>>> [   12.510187] init: Opened file '/dev/kmsg', flags 1
>>> [   12.510353] init: couldn't write 1941 to
>>> /dev/cpuset/system-background/tasks: No such file or directory
>>> [   12.533046] init: Service 'logd' (pid 1941) exited with status 255
>>>
>>>
>>> I did some bisection and narrowed it down to 1ea0ce4069 ("selinux:
>>> allow changing labels for cgroupfs"), which was merged in yesterday.
>>> I've not yet been able to figure out the root cause, but reverting
>>> that patch makes things work again.
>>>
>>> So I wanted to raise the issue here so folks were aware.
>>>
>>> If there is anything folks want me to test or try, please let me know.
>>
>> Unfortunately I don't have an Android test system to play with, have
>> any of the SEAndroid folks on the To/CC line seen a similar problem?
>
> So from my very limited knowledge here, adding the patch in question
> seems to make the cgroup mount get the SBLABEL_MNT flag?
> Which I'm guessing this is causing additional selinux restrictions on
> processes accessing cgroup mounts, which causes some of the early
> initialization processes to fail?
>
> Should this change mean the selinux policy needs to be updated?
>
> thanks
> -john



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Aug 2, 2016 at 9:57 AM, Roberts, William C
<william.c.robe...@intel.com> wrote:
> @nnk, disabling ASLR via set_arch() on Android, is that only for 32 bit 
> address spaces where
> you had that problem?

Yes. Only 32 bit address spaces had the fragmentation problem.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Aug 2, 2016 at 9:57 AM, Roberts, William C
 wrote:
> @nnk, disabling ASLR via set_arch() on Android, is that only for 32 bit 
> address spaces where
> you had that problem?

Yes. Only 32 bit address spaces had the fragmentation problem.

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Jul 26, 2016 at 1:59 PM, Jason Cooper <ja...@lakedaemon.net> wrote:
>> > One thing I didn't make clear in my commit message is why this is good. 
>> > Right
>> > now, if you know An address within in a process, you know all offsets done 
>> > with
>> > mmap(). For instance, an offset To libX can yield libY by 
>> > adding/subtracting an
>> > offset. This is meant to make rops a bit harder, or In general any mapping 
>> > offset
>> > mmore difficult to find/guess.
>
> Are you able to quantify how many bits of entropy you're imposing on the
> attacker?  Is this a chair in the hallway or a significant increase in
> the chances of crashing the program before finding the desired address?

Quantifying the effect of many security changes is extremely
difficult, especially for a probabilistic defense like ASLR. I would
urge us to not place too high of a proof bar on this change.
Channeling Spender / grsecurity team, ASLR gets it's benefit not from
it's high benefit, but from it's low cost of implementation
(https://forums.grsecurity.net/viewtopic.php?f=7=3367). This patch
certainly meets the low cost of implementation bar.

In the Project Zero Stagefright post
(http://googleprojectzero.blogspot.com/2015/09/stagefrightened.html),
we see that the linear allocation of memory combined with the low
number of bits in the initial mmap offset resulted in a much more
predictable layout which aided the attacker. The initial random mmap
base range was increased by Daniel Cashman in
d07e22597d1d355829b7b18ac19afa912cf758d1, but we've done nothing to
address page relative attacks.

Inter-mmap randomization will decrease the predictability of later
mmap() allocations, which should help make data structures harder to
find in memory. In addition, this patch will also introduce unmapped
gaps between pages, preventing linear overruns from one mapping to
another another mapping. I am unable to quantify how much this will
improve security, but it should be > 0.

I like Dave Hansen's suggestion that this functionality be limited to
64 bits, where concerns about running out of address space are
essentially nil. I'd be supportive of this change if it was limited to
64 bits.

-- Nick

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Jul 26, 2016 at 1:59 PM, Jason Cooper  wrote:
>> > One thing I didn't make clear in my commit message is why this is good. 
>> > Right
>> > now, if you know An address within in a process, you know all offsets done 
>> > with
>> > mmap(). For instance, an offset To libX can yield libY by 
>> > adding/subtracting an
>> > offset. This is meant to make rops a bit harder, or In general any mapping 
>> > offset
>> > mmore difficult to find/guess.
>
> Are you able to quantify how many bits of entropy you're imposing on the
> attacker?  Is this a chair in the hallway or a significant increase in
> the chances of crashing the program before finding the desired address?

Quantifying the effect of many security changes is extremely
difficult, especially for a probabilistic defense like ASLR. I would
urge us to not place too high of a proof bar on this change.
Channeling Spender / grsecurity team, ASLR gets it's benefit not from
it's high benefit, but from it's low cost of implementation
(https://forums.grsecurity.net/viewtopic.php?f=7=3367). This patch
certainly meets the low cost of implementation bar.

In the Project Zero Stagefright post
(http://googleprojectzero.blogspot.com/2015/09/stagefrightened.html),
we see that the linear allocation of memory combined with the low
number of bits in the initial mmap offset resulted in a much more
predictable layout which aided the attacker. The initial random mmap
base range was increased by Daniel Cashman in
d07e22597d1d355829b7b18ac19afa912cf758d1, but we've done nothing to
address page relative attacks.

Inter-mmap randomization will decrease the predictability of later
mmap() allocations, which should help make data structures harder to
find in memory. In addition, this patch will also introduce unmapped
gaps between pages, preventing linear overruns from one mapping to
another another mapping. I am unable to quantify how much this will
improve security, but it should be > 0.

I like Dave Hansen's suggestion that this functionality be limited to
64 bits, where concerns about running out of address space are
essentially nil. I'd be supportive of this change if it was limited to
64 bits.

-- Nick

-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Jul 26, 2016 at 2:02 PM, Roberts, William C
<william.c.robe...@intel.com> wrote:
>
>
>> -Original Message-----
>> From: Nick Kralevich [mailto:n...@google.com]
>> Sent: Tuesday, July 26, 2016 1:41 PM
>> To: Roberts, William C <william.c.robe...@intel.com>
>> Cc: ja...@lakedaemon.net; linux...@vger.kernel.org; lkml > ker...@vger.kernel.org>; kernel-harden...@lists.openwall.com; Andrew
>> Morton <a...@linux-foundation.org>; Kees Cook <keesc...@chromium.org>;
>> Greg KH <gre...@linuxfoundation.org>; Jeffrey Vander Stoep
>> <je...@google.com>; saly...@android.com; Daniel Cashman
>> <dcash...@android.com>
>> Subject: Re: [PATCH] [RFC] Introduce mmap randomization
>>
>> My apologies in advance if I misunderstand the purposes of this patch.
>>
>> IIUC, this patch adds a random gap between various mmap() mappings, with the
>> goal of ensuring that both the mmap base address and gaps between pages are
>> randomized.
>>
>> If that's the goal, please note that this behavior has caused significant
>> performance problems to Android in the past. Specifically, random gaps 
>> between
>> mmap()ed regions causes memory space fragmentation. After a program runs for
>> a long time, the ability to find large contiguous blocks of memory becomes
>> impossible, and mmap()s fail due to lack of a large enough address space.
>
> Yes and fragmentation is definitely a problem here. Especially when the 
> mmaps()
> are not a consistent length for program life.
>
>>
>> This isn't just a theoretical concern. Android actually hit this on kernels 
>> prior to
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7dbaa46
>> 6780a754154531b44c2086f6618cee3a8
>> . Before that patch, the gaps between mmap()ed pages were randomized.
>> See the discussion at:
>>
>>   http://lists.infradead.org/pipermail/linux-arm-kernel/2011-
>> November/073082.html
>>   http://marc.info/?t=13207095745=1=2
>>
>> We ended up having to work around this problem in the following commits:
>>
>>
>> https://android.googlesource.com/platform/dalvik/+/311886c6c6fcd3b531531f59
>> 2d56caab5e2a259c
>>   https://android.googlesource.com/platform/art/+/51e5386
>>   https://android.googlesource.com/platform/art/+/f94b781
>>
>> If this behavior was re-introduced, it's likely to cause hard-to-reproduce
>> problems, and I suspect Android based distributions would tend to disable 
>> this
>> feature either globally, or for applications which make a large number of 
>> mmap()
>> calls.
>
> Yeah and this is the issue I want to see if we can overcome. I see the 
> biggest benefit
> being on libraries loaded by dl. Perhaps a random flag and modify to linkers. 
> Im just
> spit balling here and collecting the feedback, like this. Thanks for the 
> detail, that
> helps a lot.

Android N introduced library load order randomization, which partially
helps with this.

https://android-review.googlesource.com/178130

There's also https://android-review.googlesource.com/248499 which adds
additional gaps for shared libraries.


>
>>
>> -- Nick
>>
>>
>>
>> On Tue, Jul 26, 2016 at 11:22 AM,  <william.c.robe...@intel.com> wrote:
>> > From: William Roberts <william.c.robe...@intel.com>
>> >
>> > This patch introduces the ability randomize mmap locations where the
>> > address is not requested, for instance when ld is allocating pages for
>> > shared libraries. It chooses to randomize based on the current
>> > personality for ASLR.
>> >
>> > Currently, allocations are done sequentially within unmapped address
>> > space gaps. This may happen top down or bottom up depending on scheme.
>> >
>> > For instance these mmap calls produce contiguous mappings:
>> > int size = getpagesize();
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40026000
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40027000
>> >
>> > Note no gap between.
>> >
>> > After patches:
>> > int size = getpagesize();
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x400b4000
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40055000
>> >
>> > Note gap between.
>> >
>> > Using the test program mentioned here, that allocates fixed sized
>> > blocks till exhaustion:
>> > https://www.linux-mips.org/archives/linux-mips/201

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

On Tue, Jul 26, 2016 at 2:02 PM, Roberts, William C
 wrote:
>
>
>> -Original Message-----
>> From: Nick Kralevich [mailto:n...@google.com]
>> Sent: Tuesday, July 26, 2016 1:41 PM
>> To: Roberts, William C 
>> Cc: ja...@lakedaemon.net; linux...@vger.kernel.org; lkml > ker...@vger.kernel.org>; kernel-harden...@lists.openwall.com; Andrew
>> Morton ; Kees Cook ;
>> Greg KH ; Jeffrey Vander Stoep
>> ; saly...@android.com; Daniel Cashman
>> 
>> Subject: Re: [PATCH] [RFC] Introduce mmap randomization
>>
>> My apologies in advance if I misunderstand the purposes of this patch.
>>
>> IIUC, this patch adds a random gap between various mmap() mappings, with the
>> goal of ensuring that both the mmap base address and gaps between pages are
>> randomized.
>>
>> If that's the goal, please note that this behavior has caused significant
>> performance problems to Android in the past. Specifically, random gaps 
>> between
>> mmap()ed regions causes memory space fragmentation. After a program runs for
>> a long time, the ability to find large contiguous blocks of memory becomes
>> impossible, and mmap()s fail due to lack of a large enough address space.
>
> Yes and fragmentation is definitely a problem here. Especially when the 
> mmaps()
> are not a consistent length for program life.
>
>>
>> This isn't just a theoretical concern. Android actually hit this on kernels 
>> prior to
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7dbaa46
>> 6780a754154531b44c2086f6618cee3a8
>> . Before that patch, the gaps between mmap()ed pages were randomized.
>> See the discussion at:
>>
>>   http://lists.infradead.org/pipermail/linux-arm-kernel/2011-
>> November/073082.html
>>   http://marc.info/?t=13207095745=1=2
>>
>> We ended up having to work around this problem in the following commits:
>>
>>
>> https://android.googlesource.com/platform/dalvik/+/311886c6c6fcd3b531531f59
>> 2d56caab5e2a259c
>>   https://android.googlesource.com/platform/art/+/51e5386
>>   https://android.googlesource.com/platform/art/+/f94b781
>>
>> If this behavior was re-introduced, it's likely to cause hard-to-reproduce
>> problems, and I suspect Android based distributions would tend to disable 
>> this
>> feature either globally, or for applications which make a large number of 
>> mmap()
>> calls.
>
> Yeah and this is the issue I want to see if we can overcome. I see the 
> biggest benefit
> being on libraries loaded by dl. Perhaps a random flag and modify to linkers. 
> Im just
> spit balling here and collecting the feedback, like this. Thanks for the 
> detail, that
> helps a lot.

Android N introduced library load order randomization, which partially
helps with this.

https://android-review.googlesource.com/178130

There's also https://android-review.googlesource.com/248499 which adds
additional gaps for shared libraries.


>
>>
>> -- Nick
>>
>>
>>
>> On Tue, Jul 26, 2016 at 11:22 AM,   wrote:
>> > From: William Roberts 
>> >
>> > This patch introduces the ability randomize mmap locations where the
>> > address is not requested, for instance when ld is allocating pages for
>> > shared libraries. It chooses to randomize based on the current
>> > personality for ASLR.
>> >
>> > Currently, allocations are done sequentially within unmapped address
>> > space gaps. This may happen top down or bottom up depending on scheme.
>> >
>> > For instance these mmap calls produce contiguous mappings:
>> > int size = getpagesize();
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40026000
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40027000
>> >
>> > Note no gap between.
>> >
>> > After patches:
>> > int size = getpagesize();
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x400b4000
>> > mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
>> 0x40055000
>> >
>> > Note gap between.
>> >
>> > Using the test program mentioned here, that allocates fixed sized
>> > blocks till exhaustion:
>> > https://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html,
>> > no difference was noticed in the number of allocations. Most varied
>> > from run to run, but were always within a few allocations of one
>> > another between patched and un-patched runs.
>> >
>> > Performance Measurements:

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

My apologies in advance if I misunderstand the purposes of this patch.

IIUC, this patch adds a random gap between various mmap() mappings,
with the goal of ensuring that both the mmap base address and gaps
between pages are randomized.

If that's the goal, please note that this behavior has caused
significant performance problems to Android in the past. Specifically,
random gaps between mmap()ed regions causes memory space
fragmentation. After a program runs for a long time, the ability to
find large contiguous blocks of memory becomes impossible, and mmap()s
fail due to lack of a large enough address space.

This isn't just a theoretical concern. Android actually hit this on
kernels prior to
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7dbaa466780a754154531b44c2086f6618cee3a8
. Before that patch, the gaps between mmap()ed pages were randomized.
See the discussion at:

  
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/073082.html
  http://marc.info/?t=13207095745=1=2

We ended up having to work around this problem in the following commits:

  
https://android.googlesource.com/platform/dalvik/+/311886c6c6fcd3b531531f592d56caab5e2a259c
  https://android.googlesource.com/platform/art/+/51e5386
  https://android.googlesource.com/platform/art/+/f94b781

If this behavior was re-introduced, it's likely to cause
hard-to-reproduce problems, and I suspect Android based distributions
would tend to disable this feature either globally, or for
applications which make a large number of mmap() calls.

-- Nick



On Tue, Jul 26, 2016 at 11:22 AM,  <william.c.robe...@intel.com> wrote:
> From: William Roberts <william.c.robe...@intel.com>
>
> This patch introduces the ability randomize mmap locations where the
> address is not requested, for instance when ld is allocating pages for
> shared libraries. It chooses to randomize based on the current
> personality for ASLR.
>
> Currently, allocations are done sequentially within unmapped address
> space gaps. This may happen top down or bottom up depending on scheme.
>
> For instance these mmap calls produce contiguous mappings:
> int size = getpagesize();
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40026000
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40027000
>
> Note no gap between.
>
> After patches:
> int size = getpagesize();
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x400b4000
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40055000
>
> Note gap between.
>
> Using the test program mentioned here, that allocates fixed sized blocks
> till exhaustion: 
> https://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html,
> no difference was noticed in the number of allocations. Most varied from
> run to run, but were always within a few allocations of one another
> between patched and un-patched runs.
>
> Performance Measurements:
> Using strace with -T option and filtering for mmap on the program
> ls shows a slowdown of approximate 3.7%
>
> Signed-off-by: William Roberts <william.c.robe...@intel.com>
> ---
>  mm/mmap.c | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index de2c176..7891272 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -1582,6 +1583,24 @@ unacct_error:
> return error;
>  }
>
> +/*
> + * Generate a random address within a range. This differs from 
> randomize_addr() by randomizing
> + * on len sized chunks. This helps prevent fragmentation of the virtual 
> memory map.
> + */
> +static unsigned long randomize_mmap(unsigned long start, unsigned long end, 
> unsigned long len)
> +{
> +   unsigned long slots;
> +
> +   if ((current->personality & ADDR_NO_RANDOMIZE) || !randomize_va_space)
> +   return 0;
> +
> +   slots = (end - start)/len;
> +   if (!slots)
> +   return 0;
> +
> +   return PAGE_ALIGN(start + ((get_random_long() % slots) * len));
> +}
> +
>  unsigned long unmapped_area(struct vm_unmapped_area_info *info)
>  {
> /*
> @@ -1676,6 +1695,8 @@ found:
> if (gap_start < info->low_limit)
> gap_start = info->low_limit;
>
> +   gap_start = randomize_mmap(gap_start, gap_end, length) ? : gap_start;
> +
> /* Adjust gap address to the desired alignment */
> gap_start += (info->align_offset - gap_start) & info->align_mask;
>
> @@ -1775,6 +1796,9 @@ found:
>  found_highest:
> /* Compute highest gap address at the desired alignment */
> gap_end -= info->length;
> +
> +   gap_end = randomize_mmap(gap_start, gap_end, length) ? : gap_end;
> +
> gap_end -= (gap_end - info->align_offset) & info->align_mask;
>
> VM_BUG_ON(gap_end < info->low_limit);
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] [RFC] Introduce mmap randomization

[Nick Kralevich]

My apologies in advance if I misunderstand the purposes of this patch.

IIUC, this patch adds a random gap between various mmap() mappings,
with the goal of ensuring that both the mmap base address and gaps
between pages are randomized.

If that's the goal, please note that this behavior has caused
significant performance problems to Android in the past. Specifically,
random gaps between mmap()ed regions causes memory space
fragmentation. After a program runs for a long time, the ability to
find large contiguous blocks of memory becomes impossible, and mmap()s
fail due to lack of a large enough address space.

This isn't just a theoretical concern. Android actually hit this on
kernels prior to
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=7dbaa466780a754154531b44c2086f6618cee3a8
. Before that patch, the gaps between mmap()ed pages were randomized.
See the discussion at:

  
http://lists.infradead.org/pipermail/linux-arm-kernel/2011-November/073082.html
  http://marc.info/?t=13207095745=1=2

We ended up having to work around this problem in the following commits:

  
https://android.googlesource.com/platform/dalvik/+/311886c6c6fcd3b531531f592d56caab5e2a259c
  https://android.googlesource.com/platform/art/+/51e5386
  https://android.googlesource.com/platform/art/+/f94b781

If this behavior was re-introduced, it's likely to cause
hard-to-reproduce problems, and I suspect Android based distributions
would tend to disable this feature either globally, or for
applications which make a large number of mmap() calls.

-- Nick



On Tue, Jul 26, 2016 at 11:22 AM,   wrote:
> From: William Roberts 
>
> This patch introduces the ability randomize mmap locations where the
> address is not requested, for instance when ld is allocating pages for
> shared libraries. It chooses to randomize based on the current
> personality for ASLR.
>
> Currently, allocations are done sequentially within unmapped address
> space gaps. This may happen top down or bottom up depending on scheme.
>
> For instance these mmap calls produce contiguous mappings:
> int size = getpagesize();
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40026000
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40027000
>
> Note no gap between.
>
> After patches:
> int size = getpagesize();
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x400b4000
> mmap(NULL, size, flags, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x40055000
>
> Note gap between.
>
> Using the test program mentioned here, that allocates fixed sized blocks
> till exhaustion: 
> https://www.linux-mips.org/archives/linux-mips/2011-05/msg00252.html,
> no difference was noticed in the number of allocations. Most varied from
> run to run, but were always within a few allocations of one another
> between patched and un-patched runs.
>
> Performance Measurements:
> Using strace with -T option and filtering for mmap on the program
> ls shows a slowdown of approximate 3.7%
>
> Signed-off-by: William Roberts 
> ---
>  mm/mmap.c | 24 
>  1 file changed, 24 insertions(+)
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index de2c176..7891272 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -43,6 +43,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>
>  #include 
>  #include 
> @@ -1582,6 +1583,24 @@ unacct_error:
> return error;
>  }
>
> +/*
> + * Generate a random address within a range. This differs from 
> randomize_addr() by randomizing
> + * on len sized chunks. This helps prevent fragmentation of the virtual 
> memory map.
> + */
> +static unsigned long randomize_mmap(unsigned long start, unsigned long end, 
> unsigned long len)
> +{
> +   unsigned long slots;
> +
> +   if ((current->personality & ADDR_NO_RANDOMIZE) || !randomize_va_space)
> +   return 0;
> +
> +   slots = (end - start)/len;
> +   if (!slots)
> +   return 0;
> +
> +   return PAGE_ALIGN(start + ((get_random_long() % slots) * len));
> +}
> +
>  unsigned long unmapped_area(struct vm_unmapped_area_info *info)
>  {
> /*
> @@ -1676,6 +1695,8 @@ found:
> if (gap_start < info->low_limit)
> gap_start = info->low_limit;
>
> +   gap_start = randomize_mmap(gap_start, gap_end, length) ? : gap_start;
> +
> /* Adjust gap address to the desired alignment */
> gap_start += (info->align_offset - gap_start) & info->align_mask;
>
> @@ -1775,6 +1796,9 @@ found:
>  found_highest:
> /* Compute highest gap address at the desired alignment */
> gap_end -= info->length;
> +
> +   gap_end = randomize_mmap(gap_start, gap_end, length) ? : gap_end;
> +
> gap_end -= (gap_end - info->align_offset) & info->align_mask;
>
> VM_BUG_ON(gap_end < info->low_limit);
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz <john.stu...@linaro.org> wrote:
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc//timerslack_ns
> interface can have finer grained security policies applied to it.
>
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!

Looks good. Thanks!

Reviewed-by: Nick Kralevich <n...@google.com>

>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Cc: linux-security-mod...@vger.kernel.org
> Cc: seli...@tycho.nsa.gov
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2:
>  * Initial swing at adding settimerslack LSM hook
> v3:
>  * Fix current/p switchup bug noted by NickK
>  * Add gettimerslack hook suggested by NickK
>
>  fs/proc/base.c| 10 ++
>  include/linux/lsm_hooks.h | 13 +
>  include/linux/security.h  | 12 
>  security/security.c   | 14 ++
>  security/selinux/hooks.c  | 12 
>  5 files changed, 61 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> goto out;
> }
>
> +   err = security_task_settimerslack(p, slack_ns);
> +   if (err) {
> +   count = err;
> +   goto out;
> +   }
> +
> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void 
> *v)
> goto out;
> }
>
> +   ret = security_task_gettimerslack(p);
> +   if (ret)
> +   goto out;
> +
> task_lock(p);
> seq_printf(m, "%llu\n", p->timer_slack_ns);
> task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
>   * Check permission before moving memory owned by process @p.
>   * @p contains the task_struct for process.
>   * Return 0 if permission is granted.
> + * @task_settimerslack:
> + * Check permission before setting timerslack value of @p to @slack.
> + * @p contains the task_struct of a process.
> + * @slack contains the new slack value.
> + * Return 0 if permission is granted.
> + * @task_gettimerslack:
> + * Check permission before returning the timerslack value of @p.
> + * @p contains the task_struct of a process.
> + * Return 0 if permission is granted.
>   * @task_kill:
>   * Check permission before sending signal @sig to @p.  @info can be NULL,
>   * the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
> int (*task_setscheduler)(struct task_struct *p);
> int (*task_getscheduler)(struct task_struct *p);
> int (*task_movememory)(struct task_struct *p);
> +   int (*task_settimerslack)(struct task_struct *p, u64 slack);
> +   int (*task_gettimerslack)(struct task_struct *p);
> int (*task_kill)(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
> int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
> struct list_head task_setscheduler;
> struct list_head task_getscheduler;
> struct list_head task_movememory;
> +   struct list_head task_settimerslack;
> +   struct list_head task_gettimerslack;
> struct list_head task_kill;
> struct list_head task_wait;
> s

Re: [RFC][PATCH 2/2 v3] security: Add task_settimerslack/task_gettimerslack LSM hook

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz  wrote:
> As requested, this patch implements a task_settimerslack and
> task_gettimerslack LSM hooks so that the /proc//timerslack_ns
> interface can have finer grained security policies applied to it.
>
> I've kept the CAP_SYS_NICE check in the timerslack_ns_write/show
> functions, as hiding it in the LSM hook seems too opaque, and doesn't
> seem like a widely enough adopted practice.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!

Looks good. Thanks!

Reviewed-by: Nick Kralevich 

>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Cc: linux-security-mod...@vger.kernel.org
> Cc: seli...@tycho.nsa.gov
> Signed-off-by: John Stultz 
> ---
> v2:
>  * Initial swing at adding settimerslack LSM hook
> v3:
>  * Fix current/p switchup bug noted by NickK
>  * Add gettimerslack hook suggested by NickK
>
>  fs/proc/base.c| 10 ++
>  include/linux/lsm_hooks.h | 13 +
>  include/linux/security.h  | 12 
>  security/security.c   | 14 ++
>  security/selinux/hooks.c  | 12 
>  5 files changed, 61 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index c94abae..cc66aa8 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2286,6 +2286,12 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> goto out;
> }
>
> +   err = security_task_settimerslack(p, slack_ns);
> +   if (err) {
> +   count = err;
> +   goto out;
> +   }
> +
> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2314,6 +2320,10 @@ static int timerslack_ns_show(struct seq_file *m, void 
> *v)
> goto out;
> }
>
> +   ret = security_task_gettimerslack(p);
> +   if (ret)
> +   goto out;
> +
> task_lock(p);
> seq_printf(m, "%llu\n", p->timer_slack_ns);
> task_unlock(p);
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..290483e 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,15 @@
>   * Check permission before moving memory owned by process @p.
>   * @p contains the task_struct for process.
>   * Return 0 if permission is granted.
> + * @task_settimerslack:
> + * Check permission before setting timerslack value of @p to @slack.
> + * @p contains the task_struct of a process.
> + * @slack contains the new slack value.
> + * Return 0 if permission is granted.
> + * @task_gettimerslack:
> + * Check permission before returning the timerslack value of @p.
> + * @p contains the task_struct of a process.
> + * Return 0 if permission is granted.
>   * @task_kill:
>   * Check permission before sending signal @sig to @p.  @info can be NULL,
>   * the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1482,8 @@ union security_list_options {
> int (*task_setscheduler)(struct task_struct *p);
> int (*task_getscheduler)(struct task_struct *p);
> int (*task_movememory)(struct task_struct *p);
> +   int (*task_settimerslack)(struct task_struct *p, u64 slack);
> +   int (*task_gettimerslack)(struct task_struct *p);
> int (*task_kill)(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
> int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1743,8 @@ struct security_hook_heads {
> struct list_head task_setscheduler;
> struct list_head task_getscheduler;
> struct list_head task_movememory;
> +   struct list_head task_settimerslack;
> +   struct list_head task_gettimerslack;
> struct list_head task_kill;
> struct list_head task_wait;
> struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..ab70f47 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,8 @@ int security_task_setrlimit(struct task_struct *p, 
> unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int 

Re: [RFC][PATCH 1/2 v3] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz <john.stu...@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.

Looks good. Thanks!

Reviewed-by: Nick Kralevich <n...@google.com>

>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
> v3: Tweaked where CAP_SYS_NICE check is made, suggeded by NickK
>
>  fs/proc/base.c | 34 --
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..c94abae 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,16 +2281,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> +   if (!capable(CAP_SYS_NICE)) {
> count = -EPERM;
> +   goto out;
> +   }
> +
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> +out:
> put_task_struct(p);
>
> return count;
> @@ -2300,19 +2303,22 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +   int err = 0;
>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> +   if (!capable(CAP_SYS_NICE)) {
> err = -EPERM;
> +   goto out;
> +   }
>
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
> +
> +out:
> put_task_struct(p);
>
> return err;
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v3] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Mon, Jul 18, 2016 at 1:11 PM, John Stultz  wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.

Looks good. Thanks!

Reviewed-by: Nick Kralevich 

>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Signed-off-by: John Stultz 
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
> v3: Tweaked where CAP_SYS_NICE check is made, suggeded by NickK
>
>  fs/proc/base.c | 34 --
>  1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..c94abae 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,16 +2281,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> +   if (!capable(CAP_SYS_NICE)) {
> count = -EPERM;
> +   goto out;
> +   }
> +
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> +out:
> put_task_struct(p);
>
> return count;
> @@ -2300,19 +2303,22 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +   int err = 0;
>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> +   if (!capable(CAP_SYS_NICE)) {
> err = -EPERM;
> +   goto out;
> +   }
>
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
> +
> +out:
> put_task_struct(p);
>
> return err;
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 1:03 PM, John Stultz <john.stu...@linaro.org> wrote:
> On Fri, Jul 15, 2016 at 12:55 PM, Nick Kralevich <n...@google.com> wrote:
>> This should also have a similar LSM check for reads. For the SELinux
>> implementation, this can map to the PROCESS__GETSCHED permission.
>
> Ok. I'll wire that in as well.
>
> Would adding both selinux_task_get and set methods in the same patch
> be ok? Or would folks prefer they be split into two?

I would prefer 1 patch.


-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 1:03 PM, John Stultz  wrote:
> On Fri, Jul 15, 2016 at 12:55 PM, Nick Kralevich  wrote:
>> This should also have a similar LSM check for reads. For the SELinux
>> implementation, this can map to the PROCESS__GETSCHED permission.
>
> Ok. I'll wire that in as well.
>
> Would adding both selinux_task_get and set methods in the same patch
> be ok? Or would folks prefer they be split into two?

I would prefer 1 patch.


-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz <john.stu...@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.
>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
>
>  fs/proc/base.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..8f4f8d7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2277,19 +2277,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (err < 0)
> return err;
>
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
> +
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;

The capable(CAP_SYS_NICE) permission check should be moved to this
point, since it doesn't make sense to return EPERM if the task
structure doesn't exist.

>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> -   count = -EPERM;
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> put_task_struct(p);
>
> @@ -2300,22 +2300,21 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;

This should also have a similar LSM check for reads. For the SELinux
implementation, this can map to the PROCESS__GETSCHED permission.

  security/selinux/hooks.c:

  static int selinux_task_gettimerslack(struct task_struct *p)
  {
return current_has_perm(p, PROCESS__GETSCHED);
  }

>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;

As above, recommend moving the capable(CAP_SYS_NICE) check to this point.

>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> -   err = -EPERM;
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
>
> put_task_struct(p);
>
> -   return err;
> +   return 0;
>  }
>
>  static int timerslack_ns_open(struct inode *inode, struct file *filp)
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz  wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.
>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Signed-off-by: John Stultz 
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
>
>  fs/proc/base.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..8f4f8d7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2277,19 +2277,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (err < 0)
> return err;
>
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
> +
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;

The capable(CAP_SYS_NICE) permission check should be moved to this
point, since it doesn't make sense to return EPERM if the task
structure doesn't exist.

>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> -   count = -EPERM;
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> put_task_struct(p);
>
> @@ -2300,22 +2300,21 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;

This should also have a similar LSM check for reads. For the SELinux
implementation, this can map to the PROCESS__GETSCHED permission.

  security/selinux/hooks.c:

  static int selinux_task_gettimerslack(struct task_struct *p)
  {
return current_has_perm(p, PROCESS__GETSCHED);
  }

>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;

As above, recommend moving the capable(CAP_SYS_NICE) check to this point.

>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> -   err = -EPERM;
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
>
> put_task_struct(p);
>
> -   return err;
> +   return 0;
>  }
>
>  static int timerslack_ns_open(struct inode *inode, struct file *filp)
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 2/2 v2] security: Add task_settimerslack LSM hook

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz <john.stu...@linaro.org> wrote:
> As requested, this patch implements a task_settimerslack LSM hook
> so that the /proc//timerslack_ns interface can have finer
> grained security policies applied to it.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!
>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2: Initial swing at adding LSM hook
>
>  fs/proc/base.c| 7 +++
>  include/linux/lsm_hooks.h | 7 +++
>  include/linux/security.h  | 6 ++
>  security/security.c   | 7 +++
>  security/selinux/hooks.c  | 6 ++
>  5 files changed, 33 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 8f4f8d7..7f10b37 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2284,6 +2284,12 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> +   err = security_task_settimerslack(current, slack_ns);

The first argument should be "p", not "current". "p" is the target
process you're trying to adjust.

> +   if (err) {
> +   count = err;
> +   goto out;
> +   }
> +
> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2291,6 +2297,7 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> p->timer_slack_ns = slack_ns;
> task_unlock(p);
>
> +out:
> put_task_struct(p);
>
> return count;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..ed546c4 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,11 @@
>   * Check permission before moving memory owned by process @p.
>   * @p contains the task_struct for process.
>   * Return 0 if permission is granted.
> + * @task_settimerslack:
> + * Check permission before setting timerslack value of @p to @slack.
> + * @p contains the task_struct of a process.
> + * @slack contains the new slack value.
> + * Return 0 if permission is granted.
>   * @task_kill:
>   * Check permission before sending signal @sig to @p.  @info can be NULL,
>   * the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1478,7 @@ union security_list_options {
> int (*task_setscheduler)(struct task_struct *p);
> int (*task_getscheduler)(struct task_struct *p);
> int (*task_movememory)(struct task_struct *p);
> +   int (*task_settimerslack)(struct task_struct *p, u64 slack);
> int (*task_kill)(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
> int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1738,7 @@ struct security_hook_heads {
> struct list_head task_setscheduler;
> struct list_head task_getscheduler;
> struct list_head task_movememory;
> +   struct list_head task_settimerslack;
> struct list_head task_kill;
> struct list_head task_wait;
> struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..1736e2b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,7 @@ int security_task_setrlimit(struct task_struct *p, 
> unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int security_task_movememory(struct task_struct *p);
> +int security_task_settimerslack(struct task_struct *p, u64 slack);
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
>  int security_task_wait(struct task_struct *p);
> @@ -950,6 +951,11 @@ static inline int security_task_movememory(s

Re: [RFC][PATCH 2/2 v2] security: Add task_settimerslack LSM hook

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz  wrote:
> As requested, this patch implements a task_settimerslack LSM hook
> so that the /proc//timerslack_ns interface can have finer
> grained security policies applied to it.
>
> Don't really know what I'm doing here, so close review would be
> appreciated!
>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Signed-off-by: John Stultz 
> ---
> v2: Initial swing at adding LSM hook
>
>  fs/proc/base.c| 7 +++
>  include/linux/lsm_hooks.h | 7 +++
>  include/linux/security.h  | 6 ++
>  security/security.c   | 7 +++
>  security/selinux/hooks.c  | 6 ++
>  5 files changed, 33 insertions(+)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 8f4f8d7..7f10b37 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2284,6 +2284,12 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> +   err = security_task_settimerslack(current, slack_ns);

The first argument should be "p", not "current". "p" is the target
process you're trying to adjust.

> +   if (err) {
> +   count = err;
> +   goto out;
> +   }
> +
> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2291,6 +2297,7 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> p->timer_slack_ns = slack_ns;
> task_unlock(p);
>
> +out:
> put_task_struct(p);
>
> return count;
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 7ae3976..ed546c4 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -627,6 +627,11 @@
>   * Check permission before moving memory owned by process @p.
>   * @p contains the task_struct for process.
>   * Return 0 if permission is granted.
> + * @task_settimerslack:
> + * Check permission before setting timerslack value of @p to @slack.
> + * @p contains the task_struct of a process.
> + * @slack contains the new slack value.
> + * Return 0 if permission is granted.
>   * @task_kill:
>   * Check permission before sending signal @sig to @p.  @info can be NULL,
>   * the constant 1, or a pointer to a siginfo structure.  If @info is 1 or
> @@ -1473,6 +1478,7 @@ union security_list_options {
> int (*task_setscheduler)(struct task_struct *p);
> int (*task_getscheduler)(struct task_struct *p);
> int (*task_movememory)(struct task_struct *p);
> +   int (*task_settimerslack)(struct task_struct *p, u64 slack);
> int (*task_kill)(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
> int (*task_wait)(struct task_struct *p);
> @@ -1732,6 +1738,7 @@ struct security_hook_heads {
> struct list_head task_setscheduler;
> struct list_head task_getscheduler;
> struct list_head task_movememory;
> +   struct list_head task_settimerslack;
> struct list_head task_kill;
> struct list_head task_wait;
> struct list_head task_prctl;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 14df373..1736e2b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -325,6 +325,7 @@ int security_task_setrlimit(struct task_struct *p, 
> unsigned int resource,
>  int security_task_setscheduler(struct task_struct *p);
>  int security_task_getscheduler(struct task_struct *p);
>  int security_task_movememory(struct task_struct *p);
> +int security_task_settimerslack(struct task_struct *p, u64 slack);
>  int security_task_kill(struct task_struct *p, struct siginfo *info,
> int sig, u32 secid);
>  int security_task_wait(struct task_struct *p);
> @@ -950,6 +951,11 @@ static inline int security_task_movememory(struct 
> task_struct *p)
> return 0;
>  }
>
> +static inline int security_task_settimerslack(struct task_struct *p, u64 
> slack)
> +{
> +   return 0;
> +}
> +
>  static inline int security_task_kill(struct task_struct *p,
>  struct siginfo *info, int sig,
>  u32 secid)
> diff --git a/security/securi

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz <john.stu...@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.
>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
>
>  fs/proc/base.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..8f4f8d7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2277,19 +2277,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (err < 0)
> return err;
>
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
> +

Since you're going the LSM route (from your second patch of this
series), the capability check above should be moved to the LSM hook in
security/commoncap.c.  Only one security call to
security_task_settimerslack is needed, which will cover the standard
capabilities check as well as the SELinux check.

> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>

Per your patch #2, you'd call security_task_settimerslack here. This
would call into the capability LSM hook you added in
security/commoncap.c

> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> -   count = -EPERM;
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> put_task_struct(p);
>
> @@ -2300,22 +2300,21 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> -   err = -EPERM;
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
>
> put_task_struct(p);
>
> -   return err;
> +   return 0;
>  }
>
>  static int timerslack_ns_open(struct inode *inode, struct file *filp)
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH 1/2 v2] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Fri, Jul 15, 2016 at 10:24 AM, John Stultz  wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE and removes CAP_SYS_PTRACE, simplifying some
> of the code flow as well.
>
> This is technically an ABI change, but as the feature just
> landed in 4.6, I suspect no one is yet using it.
>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Signed-off-by: John Stultz 
> ---
> v2: Removed CAP_SYS_PTRACE check and simplified code flow
>
>  fs/proc/base.c | 33 -
>  1 file changed, 16 insertions(+), 17 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..8f4f8d7 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2277,19 +2277,19 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (err < 0)
> return err;
>
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
> +

Since you're going the LSM route (from your second patch of this
series), the capability check above should be moved to the LSM hook in
security/commoncap.c.  Only one security call to
security_task_settimerslack is needed, which will cover the standard
capabilities check as well as the SELinux check.

> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>

Per your patch #2, you'd call security_task_settimerslack here. This
would call into the capability LSM hook you added in
security/commoncap.c

> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   if (slack_ns == 0)
> -   p->timer_slack_ns = p->default_timer_slack_ns;
> -   else
> -   p->timer_slack_ns = slack_ns;
> -   task_unlock(p);
> -   } else
> -   count = -EPERM;
> +   task_lock(p);
> +   if (slack_ns == 0)
> +   p->timer_slack_ns = p->default_timer_slack_ns;
> +   else
> +   p->timer_slack_ns = slack_ns;
> +   task_unlock(p);
>
> put_task_struct(p);
>
> @@ -2300,22 +2300,21 @@ static int timerslack_ns_show(struct seq_file *m, 
> void *v)
>  {
> struct inode *inode = m->private;
> struct task_struct *p;
> -   int err =  0;
> +
> +   if (!capable(CAP_SYS_NICE))
> +   return -EPERM;
>
> p = get_proc_task(inode);
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> -   task_lock(p);
> -   seq_printf(m, "%llu\n", p->timer_slack_ns);
> -   task_unlock(p);
> -   } else
> -   err = -EPERM;
> +   task_lock(p);
> +   seq_printf(m, "%llu\n", p->timer_slack_ns);
> +   task_unlock(p);
>
> put_task_struct(p);
>
> -   return err;
> +   return 0;
>  }
>
>  static int timerslack_ns_open(struct inode *inode, struct file *filp)
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Thu, Jul 14, 2016 at 11:50 AM, John Stultz <john.stu...@linaro.org> wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE.
>
> For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
> access/modify timerslack values, but I'm fine with removing
> this if others agree.
>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: "Serge E. Hallyn" <se...@hallyn.com>
> Cc: Andrew Morton <a...@linux-foundation.org>
> Cc: Thomas Gleixner <t...@linutronix.de>
> CC: Arjan van de Ven <ar...@linux.intel.com>
> Cc: Oren Laadan <or...@cellrox.com>
> Cc: Ruchi Kandoi <kandoiru...@google.com>
> Cc: Rom Lemarchand <rom...@android.com>
> Cc: Todd Kjos <tk...@google.com>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Nick Kralevich <n...@google.com>
> Cc: Dmitry Shmidt <dimitr...@google.com>
> Cc: Elliott Hughes <e...@google.com>
> Cc: Android Kernel Team <kernel-t...@android.com>
> Signed-off-by: John Stultz <john.stu...@linaro.org>
> ---
>  fs/proc/base.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..d32033e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,7 +2281,8 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +   capable(CAP_SYS_NICE)) {


Like others, I would prefer if we remove the ptrace_may_access()
checks. It seems unnecessary and makes the code slightly more
complicated than necessary.

However, if we can't do that, at a minimum, we should reorder the
checks so that capable(CAP_SYS_NICE) is checked first. Both
ptrace_may_access() and capable() will generate denials if the
appropriate SELinux permission isn't present. People will immediately
see the denial and start adding ptrace allow rules, defeating the
purpose of this change.

In addition, the current capable() check is very course grain. In
Android, for instance, system_server should have the ability to adjust
timerslack values for applications, but shouldn't necessarily apply
those same timerslack values to other privileged processes such as
init. I would recommend we add an LSM hook (include/linux/lsm_hooks.h)
here, to allow security modules to make policy decisions over whether
a timerslack value should be modifiable at all. It would also be good
to provide an SELinux implementation of the hook, which could be as
simple as:

  security/selinux/hooks.c:

  static int selinux_task_settimerslack(struct task_struct *p,
timerslack_value value)
  {
return current_has_perm(p, PROCESS__SETSCHED);
  }

This would allow SELinux fine grain control over which timer slack
values could be adjusted.

> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2306,7 +2307,8 @@ static int timerslack_ns_show(struct seq_file *m, void 
> *v)
> if (!p)
> return -ESRCH;
>
> -       if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +   capable(CAP_SYS_NICE)) {
> task_lock(p);
> seq_printf(m, "%llu\n", p->timer_slack_ns);
> task_unlock(p);
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [RFC][PATCH] proc: Relax /proc//timerslack_ns capability requirements

[Nick Kralevich]

On Thu, Jul 14, 2016 at 11:50 AM, John Stultz  wrote:
> When an interface to allow a task to change another tasks
> timerslack was first proposed, it was suggested that something
> greater then CAP_SYS_NICE would be needed, as a task could be
> delayed further then what normally could be done with nice
> adjustments.
>
> So CAP_SYS_PTRACE was adopted instead for what became the
> /proc//timerslack_ns interface. However, for Android (where
> this feature originates), giving the system_server
> CAP_SYS_PTRACE would allow it to observe and modify all tasks
> memory. This is considered too high a privilege level for only
> needing to change the timerslack.
>
> After some discussion, it was realized that a CAP_SYS_NICE
> process can set a task as SCHED_FIFO, so they could fork some
> spinning processes and set them all SCHED_FIFO 99, in effect
> delaying all other tasks for an infinite amount of time.
>
> So as a CAP_SYS_NICE task can already cause trouble for other
> tasks, using it as a required capability for accessing and
> modifying /proc//timerslack_ns seems sufficient.
>
> Thus, this patch loosens the capability requirements to
> CAP_SYS_NICE.
>
> For ABI preservation, it still allows CAP_SYS_PTRACE tasks to
> access/modify timerslack values, but I'm fine with removing
> this if others agree.
>
> Cc: Kees Cook 
> Cc: "Serge E. Hallyn" 
> Cc: Andrew Morton 
> Cc: Thomas Gleixner 
> CC: Arjan van de Ven 
> Cc: Oren Laadan 
> Cc: Ruchi Kandoi 
> Cc: Rom Lemarchand 
> Cc: Todd Kjos 
> Cc: Colin Cross 
> Cc: Nick Kralevich 
> Cc: Dmitry Shmidt 
> Cc: Elliott Hughes 
> Cc: Android Kernel Team 
> Signed-off-by: John Stultz 
> ---
>  fs/proc/base.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index a11eb71..d32033e 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2281,7 +2281,8 @@ static ssize_t timerslack_ns_write(struct file *file, 
> const char __user *buf,
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +   capable(CAP_SYS_NICE)) {


Like others, I would prefer if we remove the ptrace_may_access()
checks. It seems unnecessary and makes the code slightly more
complicated than necessary.

However, if we can't do that, at a minimum, we should reorder the
checks so that capable(CAP_SYS_NICE) is checked first. Both
ptrace_may_access() and capable() will generate denials if the
appropriate SELinux permission isn't present. People will immediately
see the denial and start adding ptrace allow rules, defeating the
purpose of this change.

In addition, the current capable() check is very course grain. In
Android, for instance, system_server should have the ability to adjust
timerslack values for applications, but shouldn't necessarily apply
those same timerslack values to other privileged processes such as
init. I would recommend we add an LSM hook (include/linux/lsm_hooks.h)
here, to allow security modules to make policy decisions over whether
a timerslack value should be modifiable at all. It would also be good
to provide an SELinux implementation of the hook, which could be as
simple as:

  security/selinux/hooks.c:

  static int selinux_task_settimerslack(struct task_struct *p,
timerslack_value value)
  {
return current_has_perm(p, PROCESS__SETSCHED);
  }

This would allow SELinux fine grain control over which timer slack
values could be adjusted.

> task_lock(p);
> if (slack_ns == 0)
> p->timer_slack_ns = p->default_timer_slack_ns;
> @@ -2306,7 +2307,8 @@ static int timerslack_ns_show(struct seq_file *m, void 
> *v)
> if (!p)
> return -ESRCH;
>
> -   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS)) {
> +   if (ptrace_may_access(p, PTRACE_MODE_ATTACH_FSCREDS) ||
> +   capable(CAP_SYS_NICE)) {
> task_lock(p);
> seq_printf(m, "%llu\n", p->timer_slack_ns);
> task_unlock(p);
> --
> 1.9.1
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037

Re: [PATCH] mm: reorder can_do_mlock to fix audit denial

[Nick Kralevich]

On Mon, Mar 2, 2015 at 9:20 AM, Jeff Vander Stoep  wrote:
> A userspace call to mmap(MAP_LOCKED) may result in the successful
> locking of memory while also producing a confusing audit log denial.
> can_do_mlock checks capable and rlimit. If either of these return
> positive can_do_mlock returns true. The capable check leads to an LSM
> hook used by apparmour and selinux which produce the audit denial.
> Reordering so rlimit is checked first eliminates the denial on success,
> only recording a denial when the lock is unsuccessful as a result of
> the denial.
>

Acked-By: Nick Kralevich 

> Signed-off-by: Jeff Vander Stoep 
> ---
>  mm/mlock.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/mm/mlock.c b/mm/mlock.c
> index 73cf098..8a54cd2 100644
> --- a/mm/mlock.c
> +++ b/mm/mlock.c
> @@ -26,10 +26,10 @@
>
>  int can_do_mlock(void)
>  {
> -   if (capable(CAP_IPC_LOCK))
> -   return 1;
> if (rlimit(RLIMIT_MEMLOCK) != 0)
> return 1;
> +   if (capable(CAP_IPC_LOCK))
> +   return 1;
> return 0;
>  }
>  EXPORT_SYMBOL(can_do_mlock);
> --
> 2.2.0.rc0.207.ga3a616c
>



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] mm: reorder can_do_mlock to fix audit denial

[Nick Kralevich]

On Mon, Mar 2, 2015 at 9:20 AM, Jeff Vander Stoep je...@google.com wrote:
 A userspace call to mmap(MAP_LOCKED) may result in the successful
 locking of memory while also producing a confusing audit log denial.
 can_do_mlock checks capable and rlimit. If either of these return
 positive can_do_mlock returns true. The capable check leads to an LSM
 hook used by apparmour and selinux which produce the audit denial.
 Reordering so rlimit is checked first eliminates the denial on success,
 only recording a denial when the lock is unsuccessful as a result of
 the denial.


Acked-By: Nick Kralevich n...@google.com

 Signed-off-by: Jeff Vander Stoep je...@google.com
 ---
  mm/mlock.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/mm/mlock.c b/mm/mlock.c
 index 73cf098..8a54cd2 100644
 --- a/mm/mlock.c
 +++ b/mm/mlock.c
 @@ -26,10 +26,10 @@

  int can_do_mlock(void)
  {
 -   if (capable(CAP_IPC_LOCK))
 -   return 1;
 if (rlimit(RLIMIT_MEMLOCK) != 0)
 return 1;
 +   if (capable(CAP_IPC_LOCK))
 +   return 1;
 return 0;
  }
  EXPORT_SYMBOL(can_do_mlock);
 --
 2.2.0.rc0.207.ga3a616c




-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] avc: remove unnecessary pointer reassignment

[Nick Kralevich]

Acked-By: Nick Kralevich 

On Thu, Feb 26, 2015 at 1:54 PM, Jeff Vander Stoep  wrote:
> Commit f01e1af445fa ("selinux: don't pass in NULL avd to 
> avc_has_perm_noaudit")
> made this pointer reassignment unnecessary. Avd should continue to reference
> the stack-based copy.
>
> Signed-off-by: Jeff Vander Stoep 
> ---
>  security/selinux/avc.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/security/selinux/avc.c b/security/selinux/avc.c
> index afcc0ae..3c17dda 100644
> --- a/security/selinux/avc.c
> +++ b/security/selinux/avc.c
> @@ -724,12 +724,10 @@ inline int avc_has_perm_noaudit(u32 ssid, u32 tsid,
> rcu_read_lock();
>
> node = avc_lookup(ssid, tsid, tclass);
> -   if (unlikely(!node)) {
> +   if (unlikely(!node))
> node = avc_compute_av(ssid, tsid, tclass, avd);
> -   } else {
> +   else
> memcpy(avd, >ae.avd, sizeof(*avd));
> -   avd = >ae.avd;
> -   }
>
> denied = requested & ~(avd->allowed);
> if (unlikely(denied))
> --
> 2.2.0.rc0.207.ga3a616c
>
> ___
> Selinux mailing list
> seli...@tycho.nsa.gov
> To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
> To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] avc: remove unnecessary pointer reassignment

[Nick Kralevich]

Acked-By: Nick Kralevich n...@google.com

On Thu, Feb 26, 2015 at 1:54 PM, Jeff Vander Stoep je...@google.com wrote:
 Commit f01e1af445fa (selinux: don't pass in NULL avd to 
 avc_has_perm_noaudit)
 made this pointer reassignment unnecessary. Avd should continue to reference
 the stack-based copy.

 Signed-off-by: Jeff Vander Stoep je...@google.com
 ---
  security/selinux/avc.c | 6 ++
  1 file changed, 2 insertions(+), 4 deletions(-)

 diff --git a/security/selinux/avc.c b/security/selinux/avc.c
 index afcc0ae..3c17dda 100644
 --- a/security/selinux/avc.c
 +++ b/security/selinux/avc.c
 @@ -724,12 +724,10 @@ inline int avc_has_perm_noaudit(u32 ssid, u32 tsid,
 rcu_read_lock();

 node = avc_lookup(ssid, tsid, tclass);
 -   if (unlikely(!node)) {
 +   if (unlikely(!node))
 node = avc_compute_av(ssid, tsid, tclass, avd);
 -   } else {
 +   else
 memcpy(avd, node-ae.avd, sizeof(*avd));
 -   avd = node-ae.avd;
 -   }

 denied = requested  ~(avd-allowed);
 if (unlikely(denied))
 --
 2.2.0.rc0.207.ga3a616c

 ___
 Selinux mailing list
 seli...@tycho.nsa.gov
 To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
 To get help, send an email containing help to selinux-requ...@tycho.nsa.gov.



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.

[Nick Kralevich]

Acked-By: Nick Kralevich 

On Thu, Jan 22, 2015 at 12:51 AM, Greg KH  wrote:
> On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
>> Add security hooks to the binder and implement the hooks for SELinux.
>> The security hooks enable security modules such as SELinux to implement
>> controls over binder IPC.  The security hooks include support for
>> controlling what process can become the binder context manager
>> (binder_set_context_mgr), controlling the ability of a process
>> to invoke a binder transaction/IPC to another process (binder_transaction),
>> controlling the ability of a process to transfer a binder reference to
>> another process (binder_transfer_binder), and controlling the ability
>> of a process to transfer an open file to another process 
>> (binder_transfer_file).
>>
>> These hooks have been included in the Android kernel trees since Android 4.3.
>
> Very interesting, I missed the fact that these were added in that tree,
> thanks for digging it out and submitting it.
>
> I'd like some acks from some Android developers before I take these.
> Or, if it's easier for them to go through the security tree, that's fine
> with me as well.
>
> thanks,
>
> greg k-h



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] Add security hooks to binder and implement the hooks for SELinux.

[Nick Kralevich]

Acked-By: Nick Kralevich n...@google.com

On Thu, Jan 22, 2015 at 12:51 AM, Greg KH gre...@linuxfoundation.org wrote:
 On Wed, Jan 21, 2015 at 10:54:10AM -0500, Stephen Smalley wrote:
 Add security hooks to the binder and implement the hooks for SELinux.
 The security hooks enable security modules such as SELinux to implement
 controls over binder IPC.  The security hooks include support for
 controlling what process can become the binder context manager
 (binder_set_context_mgr), controlling the ability of a process
 to invoke a binder transaction/IPC to another process (binder_transaction),
 controlling the ability of a process to transfer a binder reference to
 another process (binder_transfer_binder), and controlling the ability
 of a process to transfer an open file to another process 
 (binder_transfer_file).

 These hooks have been included in the Android kernel trees since Android 4.3.

 Very interesting, I missed the fact that these were added in that tree,
 thanks for digging it out and submitting it.

 I'd like some acks from some Android developers before I take these.
 Or, if it's easier for them to go through the security tree, that's fine
 with me as well.

 thanks,

 greg k-h



-- 
Nick Kralevich | Android Security | n...@google.com | 650.214.4037
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/