* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-12 16:16 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <7c367905-e8c9-7665-d923-c850e05c757a@tycho.nsa.gov>
On 12.02.2020 18:21, Stephen Smalley wrote:
> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>> Hi Stephen,
>>>>
>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>
>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>
>>>> <SNIP>
>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>>
>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>>
>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>
>>>>>> static inline bool perfmon_capable(void)
>>>>>> {
>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>> }
>>>>>>
>>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>>> privileged process.
>>>>>>
>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>>> based approach to use perf_event_open system call.
>>>>>
>>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>>
>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>
>>>> However there is no capability related messages around. I suppose my refpolicy should
>>>> be modified somehow to observe capability related AVCs.
>>>>
>>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>>> to test the concerned logging.
>>>
>>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>>
>> Yes, I already have it like this:
>> common cap2
>> {
>> <------>mac_override<--># unused by SELinux
>> <------>mac_admin
>> <------>syslog
>> <------>wake_alarm
>> <------>block_suspend
>> <------>audit_read
>> <------>perfmon
>> }
>>
>> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
>
> Some denials may be silenced by dontaudit rules; semodule -DB will strip those and semodule -B will restore them. Other possibility is that the process doesn't have CAP_PERFMON in its effective set and therefore never reaches SELinux at all; denied first by the capability module.
Yes, that all makes sense.
selinux_capable() calls avc_audit() logging but cap_capable() doesn't, so proper order matters.
I am doing debug tracing of the kernel code to reveal the exact reasons.
~Alexey
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: KP Singh @ 2020-02-12 16:26 UTC (permalink / raw)
To: Casey Schaufler
Cc: Alexei Starovoitov, Daniel Borkmann, Jann Horn, KP Singh,
kernel list, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov, James Morris,
Kees Cook, Thomas Garnier, Michael Halcrow, Paul Turner,
Brendan Gregg, Matthew Garrett, Christian Brauner,
Mickaël Salaün, Florent Revest, Brendan Jackman,
Serge E. Hallyn, Mauro Carvalho Chehab, David S. Miller,
Greg Kroah-Hartman, Kernel Team
In-Reply-To: <bee0fd08-b9f2-83e4-2882-475b81c74303@schaufler-ca.com>
On 12-Feb 07:52, Casey Schaufler wrote:
> On 2/11/2020 6:45 PM, Alexei Starovoitov wrote:
> > On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
> >> Another approach could be to have a special nop inside call_int_hook()
> >> macro which would then get patched to avoid these situations. Somewhat
> >> similar like static keys where it could be defined anywhere in text but
> >> with updating of call_int_hook()'s RC for the verdict.
>
> Tell me again why you can't register your BPF hooks like all the
> other security modules do? You keep reintroducing BPF as a special
> case, and I don't see why.
I think we tried to answer this in the discussion we had:
https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#meb1eea982e63be0806f9bba58e91160871803752
BPF should not allocate a wrapper (to be statically regsitered at
init) for each LSM hook and run the programs from within that as this
implies adding overhead across the board for every hook even if
it's never used (i.e. no BPF program is attached to the hook).
We can, with the suggestions discussed here, avoid adding unncessary
overhead for unused hooks. And, as Alexei mentioned, adding overhead
when not really needed is especially bad for LSM hooks like
sock_sendmsg.
The other LSMs do not provide dynamic / mutable hooks, so it makes
sense for them to register the hooks once at load time.
- KP
> > Sounds nice in theory. I couldn't quite picture how that would look
> > in the code, so I hacked:
> > diff --git a/security/security.c b/security/security.c
> > index 565bc9b67276..ce4bc1e5e26c 100644
> > --- a/security/security.c
[...]
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Jann Horn @ 2020-02-12 16:54 UTC (permalink / raw)
To: Kees Cook
Cc: Daniel Colascione, Tim Murray, Nosh Minwalla, Nick Kralevich,
Lokesh Gidra, kernel list, Linux API, SElinux list,
Andrea Arcangeli, Mike Rapoport, Peter Xu, linux-security-module
In-Reply-To: <202002112332.BE71455@keescook>
On Wed, Feb 12, 2020 at 8:51 AM Kees Cook <keescook@chromium.org> wrote:
> On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> Now this I'm very interested in. Can you go into more detail about two
> things:
[...]
> - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> sysctl? (And should this maybe just be another setting for that
> sysctl, like "2"?)
>
> As to the mechanics of the change, I'm not sure I like the idea of adding
> a UAPI flag for this. Why not just retain the permission check done at
> open() and if kernelmode faults aren't allowed, ignore them? This would
> require no changes to existing programs and gains the desired defense.
> (And, I think, the sysctl value could be bumped to "2" as that's a
> better default state -- does qemu actually need kernelmode traps?)
I think this might be necessary for I/O emulation? As in, if before
getting migrated, the guest writes some data into a buffer, then the
guest gets migrated, and then while the postcopy migration stuff is
still running, the guest tells QEMU to write that data from
guest-physical memory to disk or whatever; I think in that case, QEMU
will do something like a pwrite() syscall where the userspace pointer
points into the memory area containing guest-physical memory, which
would return -EFAULT if userfaultfd was restricted to userspace
accesses.
This was described in this old presentation about why userfaultfd is
better than a SIGSEGV handler:
https://drive.google.com/file/d/0BzyAwvVlQckeSzlCSDFmRHVybzQ/view
(slide 6) (recording at https://youtu.be/pC8cWWRVSPw?t=463)
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Alexey Budankov @ 2020-02-12 16:56 UTC (permalink / raw)
To: Stephen Smalley
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <280e6644-c129-15f6-ea5c-0f66bf764e0f@tycho.nsa.gov>
On 12.02.2020 18:45, Stephen Smalley wrote:
> On 2/12/20 10:21 AM, Stephen Smalley wrote:
>> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>>> Hi Stephen,
>>>>>
>>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>>
>>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>>
>>>>> <SNIP>
>>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>>>
>>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>>>
>>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>>
>>>>>>> static inline bool perfmon_capable(void)
>>>>>>> {
>>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>>> }
>>>>>>>
>>>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>>>> privileged process.
>>>>>>>
>>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>>>> based approach to use perf_event_open system call.
>>>>>>
>>>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>>>
>>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>>
>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>
>>>>> However there is no capability related messages around. I suppose my refpolicy should
>>>>> be modified somehow to observe capability related AVCs.
>>>>>
>>>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>>>> to test the concerned logging.
>>>>
>>>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>>>
>>> Yes, I already have it like this:
>>> common cap2
>>> {
>>> <------>mac_override<--># unused by SELinux
>>> <------>mac_admin
>>> <------>syslog
>>> <------>wake_alarm
>>> <------>block_suspend
>>> <------>audit_read
>>> <------>perfmon
>>> }
>>>
>>> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
>>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
>>
>> Some denials may be silenced by dontaudit rules; semodule -DB will strip those and semodule -B will restore them. Other possibility is that the process doesn't have CAP_PERFMON in its effective set and therefore never reaches SELinux at all; denied first by the capability module.
>
> Also, the fact that your denials are showing up in user_systemd_t suggests that something is off in your policy or userspace/distro; I assume that is a domain type for the systemd --user instance, but your shell and commands shouldn't be running in that domain (user_t would be more appropriate for that).
It is user_t for local terminal session:
ps -Z
LABEL PID TTY TIME CMD
user_u:user_r:user_t 11317 pts/9 00:00:00 bash
user_u:user_r:user_t 11796 pts/9 00:00:00 ps
For local terminal root session:
ps -Z
LABEL PID TTY TIME CMD
user_u:user_r:user_su_t 2926 pts/3 00:00:00 bash
user_u:user_r:user_su_t 10995 pts/3 00:00:00 ps
For remote ssh session:
ps -Z
LABEL PID TTY TIME CMD
user_u:user_r:user_t 7540 pts/8 00:00:00 ps
user_u:user_r:user_systemd_t 8875 pts/8 00:00:00 bash
~Alexey
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-02-12 17:08 UTC (permalink / raw)
To: Eric W. Biederman
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Linus Torvalds, Oleg Nesterov, Solar Designer
In-Reply-To: <87tv3vkg1a.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 08:59:29AM -0600, Eric W. Biederman wrote:
> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
>
> > On Mon, Feb 10, 2020 at 07:36:08PM -0600, Eric W. Biederman wrote:
> >> Alexey Gladkov <gladkov.alexey@gmail.com> writes:
> >>
> >> > This allows to flush dcache entries of a task on multiple procfs mounts
> >> > per pid namespace.
> >> >
> >> > The RCU lock is used because the number of reads at the task exit time
> >> > is much larger than the number of procfs mounts.
> >>
> >> A couple of quick comments.
> >>
> >> > Cc: Kees Cook <keescook@chromium.org>
> >> > Cc: Andy Lutomirski <luto@kernel.org>
> >> > Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
> >> > Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
> >> > Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> >> > ---
> >> > fs/proc/base.c | 20 +++++++++++++++-----
> >> > fs/proc/root.c | 27 ++++++++++++++++++++++++++-
> >> > include/linux/pid_namespace.h | 2 ++
> >> > include/linux/proc_fs.h | 2 ++
> >> > 4 files changed, 45 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/fs/proc/base.c b/fs/proc/base.c
> >> > index 4ccb280a3e79..24b7c620ded3 100644
> >> > --- a/fs/proc/base.c
> >> > +++ b/fs/proc/base.c
> >> > @@ -3133,7 +3133,7 @@ static const struct inode_operations proc_tgid_base_inode_operations = {
> >> > .permission = proc_pid_permission,
> >> > };
> >> >
> >> > -static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> >> > +static void proc_flush_task_mnt_root(struct dentry *mnt_root, pid_t pid, pid_t tgid)
> >> Perhaps just rename things like:
> >> > +static void proc_flush_task_root(struct dentry *root, pid_t pid, pid_t tgid)
> >> > {
> >>
> >> I don't think the mnt_ prefix conveys any information, and it certainly
> >> makes everything longer and more cumbersome.
> >>
> >> > struct dentry *dentry, *leader, *dir;
> >> > char buf[10 + 1];
> >> > @@ -3142,7 +3142,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> >> > name.name = buf;
> >> > name.len = snprintf(buf, sizeof(buf), "%u", pid);
> >> > /* no ->d_hash() rejects on procfs */
> >> > - dentry = d_hash_and_lookup(mnt->mnt_root, &name);
> >> > + dentry = d_hash_and_lookup(mnt_root, &name);
> >> > if (dentry) {
> >> > d_invalidate(dentry);
> >> > dput(dentry);
> >> > @@ -3153,7 +3153,7 @@ static void proc_flush_task_mnt(struct vfsmount *mnt, pid_t pid, pid_t tgid)
> >> >
> >> > name.name = buf;
> >> > name.len = snprintf(buf, sizeof(buf), "%u", tgid);
> >> > - leader = d_hash_and_lookup(mnt->mnt_root, &name);
> >> > + leader = d_hash_and_lookup(mnt_root, &name);
> >> > if (!leader)
> >> > goto out;
> >> >
> >> > @@ -3208,14 +3208,24 @@ void proc_flush_task(struct task_struct *task)
> >> > int i;
> >> > struct pid *pid, *tgid;
> >> > struct upid *upid;
> >> > + struct dentry *mnt_root;
> >> > + struct proc_fs_info *fs_info;
> >> >
> >> > pid = task_pid(task);
> >> > tgid = task_tgid(task);
> >> >
> >> > for (i = 0; i <= pid->level; i++) {
> >> > upid = &pid->numbers[i];
> >> > - proc_flush_task_mnt(upid->ns->proc_mnt, upid->nr,
> >> > - tgid->numbers[i].nr);
> >> > +
> >> > + rcu_read_lock();
> >> > + list_for_each_entry_rcu(fs_info, &upid->ns->proc_mounts, pidns_entry) {
> >> > + mnt_root = fs_info->m_super->s_root;
> >> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
> >> > + }
> >> > + rcu_read_unlock();
> >> > +
> >> > + mnt_root = upid->ns->proc_mnt->mnt_root;
> >> > + proc_flush_task_mnt_root(mnt_root, upid->nr, tgid->numbers[i].nr);
> >>
> >> I don't think this following of proc_mnt is needed. It certainly
> >> shouldn't be. The loop through all of the super blocks should be
> >> enough.
> >
> > Yes, thanks!
> >
> >> Once this change goes through. UML can be given it's own dedicated
> >> proc_mnt for the initial pid namespace, and proc_mnt can be removed
> >> entirely.
> >
> > After you deleted the old sysctl syscall we could probably do it.
> >
> >> Unless something has changed recently UML is the only other user of
> >> pid_ns->proc_mnt. That proc_mnt really only exists to make the loop in
> >> proc_flush_task easy to write.
> >
> > Now I think, is there any way to get rid of proc_mounts or even
> > proc_flush_task somehow.
> >
> >> It also probably makes sense to take the rcu_read_lock() over
> >> that entire for loop.
> >
> > Al Viro pointed out to me that I cannot use rcu locks here :(
>
> Fundamentally proc_flush_task is an optimization. Just getting rid of
> dentries earlier. At least at one point it was an important
> optimization because the old process dentries would just sit around
> doing nothing for anyone.
>
> I wonder if instead of invalidating specific dentries we could instead
> fire wake up a shrinker and point it at one or more instances of proc.
>
> The practical challenge I see is something might need to access the
> dentries to see that they are invalid.
>
> We definitely could try without this optimization and see what happens.
When Linus said that a semaphore for proc_mounts is a bad idea, I tried
to come up with some kind of asynchronous way to clear it per superblock.
I gave up with the asynchronous GC because userspace can quite easily get
ahead of it.
Without this optimization the kernel starts to consume a lot of memory
during intensive reading /proc. I tried to do:
while :; do
for x in `seq 0 9`; do sleep 0.1; done;
ls /proc/[0-9]*;
done >/dev/null;
and memory consumption went up without proc_flush_task. Since we have
mounted procfs in each container, this is dangerous.
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v5 01/10] capabilities: introduce CAP_PERFMON to kernel and user space
From: Stephen Smalley @ 2020-02-12 17:09 UTC (permalink / raw)
To: Alexey Budankov
Cc: Alexei Starovoitov, Peter Zijlstra, Arnaldo Carvalho de Melo,
Ingo Molnar, jani.nikula@linux.intel.com,
joonas.lahtinen@linux.intel.com, rodrigo.vivi@intel.com,
benh@kernel.crashing.org, Paul Mackerras, Michael Ellerman,
james.bottomley@hansenpartnership.com, Serge Hallyn, James Morris,
Will Deacon, Mark Rutland, Robert Richter, Alexei Starovoitov,
Jiri Olsa, Andi Kleen, Stephane Eranian, Igor Lubashev,
Alexander Shishkin, Namhyung Kim, Song Liu, Lionel Landwerlin,
Thomas Gleixner, linux-kernel,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
intel-gfx@lists.freedesktop.org, linux-parisc@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, linux-arm-kernel,
linux-perf-users@vger.kernel.org, oprofile-list, Andy Lutomirski
In-Reply-To: <950cc6a4-5823-d607-1210-6f62c96cf67f@linux.intel.com>
On 2/12/20 11:56 AM, Alexey Budankov wrote:
>
>
> On 12.02.2020 18:45, Stephen Smalley wrote:
>> On 2/12/20 10:21 AM, Stephen Smalley wrote:
>>> On 2/12/20 8:53 AM, Alexey Budankov wrote:
>>>> On 12.02.2020 16:32, Stephen Smalley wrote:
>>>>> On 2/12/20 3:53 AM, Alexey Budankov wrote:
>>>>>> Hi Stephen,
>>>>>>
>>>>>> On 22.01.2020 17:07, Stephen Smalley wrote:
>>>>>>> On 1/22/20 5:45 AM, Alexey Budankov wrote:
>>>>>>>>
>>>>>>>> On 21.01.2020 21:27, Alexey Budankov wrote:
>>>>>>>>>
>>>>>>>>> On 21.01.2020 20:55, Alexei Starovoitov wrote:
>>>>>>>>>> On Tue, Jan 21, 2020 at 9:31 AM Alexey Budankov
>>>>>>>>>> <alexey.budankov@linux.intel.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> On 21.01.2020 17:43, Stephen Smalley wrote:
>>>>>>>>>>>> On 1/20/20 6:23 AM, Alexey Budankov wrote:
>>>>>>>>>>>>>
>>>>>> <SNIP>
>>>>>>>>>>>>> Introduce CAP_PERFMON capability designed to secure system performance
>>>>>>>>>>>>
>>>>>>>>>>>> Why _noaudit()? Normally only used when a permission failure is non-fatal to the operation. Otherwise, we want the audit message.
>>>>>>>>
>>>>>>>> So far so good, I suggest using the simplest version for v6:
>>>>>>>>
>>>>>>>> static inline bool perfmon_capable(void)
>>>>>>>> {
>>>>>>>> return capable(CAP_PERFMON) || capable(CAP_SYS_ADMIN);
>>>>>>>> }
>>>>>>>>
>>>>>>>> It keeps the implementation simple and readable. The implementation is more
>>>>>>>> performant in the sense of calling the API - one capable() call for CAP_PERFMON
>>>>>>>> privileged process.
>>>>>>>>
>>>>>>>> Yes, it bloats audit log for CAP_SYS_ADMIN privileged and unprivileged processes,
>>>>>>>> but this bloating also advertises and leverages using more secure CAP_PERFMON
>>>>>>>> based approach to use perf_event_open system call.
>>>>>>>
>>>>>>> I can live with that. We just need to document that when you see both a CAP_PERFMON and a CAP_SYS_ADMIN audit message for a process, try only allowing CAP_PERFMON first and see if that resolves the issue. We have a similar issue with CAP_DAC_READ_SEARCH versus CAP_DAC_OVERRIDE.
>>>>>>
>>>>>> I am trying to reproduce this double logging with CAP_PERFMON.
>>>>>> I am using the refpolicy version with enabled perf_event tclass [1], in permissive mode.
>>>>>> When running perf stat -a I am observing this AVC audit messages:
>>>>>>
>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { open } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { kernel } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>> type=AVC msg=audit(1581496695.666:8691): avc: denied { cpu } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>> type=AVC msg=audit(1581496695.666:8692): avc: denied { write } for pid=2779 comm="perf" scontext=user_u:user_r:user_systemd_t tcontext=user_u:user_r:user_systemd_t tclass=perf_event permissive=1
>>>>>>
>>>>>> However there is no capability related messages around. I suppose my refpolicy should
>>>>>> be modified somehow to observe capability related AVCs.
>>>>>>
>>>>>> Could you please comment or clarify on how to enable caps related AVCs in order
>>>>>> to test the concerned logging.
>>>>>
>>>>> The new perfmon permission has to be defined in your policy; you'll have a message in dmesg about "Permission perfmon in class capability2 not defined in policy.". You can either add it to the common cap2 definition in refpolicy/policy/flask/access_vectors and rebuild your policy or extract your base module as CIL, add it there, and insert the updated module.
>>>>
>>>> Yes, I already have it like this:
>>>> common cap2
>>>> {
>>>> <------>mac_override<--># unused by SELinux
>>>> <------>mac_admin
>>>> <------>syslog
>>>> <------>wake_alarm
>>>> <------>block_suspend
>>>> <------>audit_read
>>>> <------>perfmon
>>>> }
>>>>
>>>> dmesg stopped reporting perfmon as not defined but audit.log still doesn't report CAP_PERFMON denials.
>>>> BTW, audit even doesn't report CAP_SYS_ADMIN denials, however perfmon_capable() does check for it.
>>>
>>> Some denials may be silenced by dontaudit rules; semodule -DB will strip those and semodule -B will restore them. Other possibility is that the process doesn't have CAP_PERFMON in its effective set and therefore never reaches SELinux at all; denied first by the capability module.
>>
>> Also, the fact that your denials are showing up in user_systemd_t suggests that something is off in your policy or userspace/distro; I assume that is a domain type for the systemd --user instance, but your shell and commands shouldn't be running in that domain (user_t would be more appropriate for that).
>
> It is user_t for local terminal session:
> ps -Z
> LABEL PID TTY TIME CMD
> user_u:user_r:user_t 11317 pts/9 00:00:00 bash
> user_u:user_r:user_t 11796 pts/9 00:00:00 ps
>
> For local terminal root session:
> ps -Z
> LABEL PID TTY TIME CMD
> user_u:user_r:user_su_t 2926 pts/3 00:00:00 bash
> user_u:user_r:user_su_t 10995 pts/3 00:00:00 ps
>
> For remote ssh session:
> ps -Z
> LABEL PID TTY TIME CMD
> user_u:user_r:user_t 7540 pts/8 00:00:00 ps
> user_u:user_r:user_systemd_t 8875 pts/8 00:00:00 bash
That's a bug in either your policy or your userspace/distro integration.
In any event, unless user_systemd_t is allowed all capability2
permissions by your policy, you should see the denials if CAP_PERFMON is
set in the effective capability set of the process.
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Daniel Colascione @ 2020-02-12 17:12 UTC (permalink / raw)
To: Kees Cook
Cc: Tim Murray, Nosh Minwalla, Nick Kralevich, Lokesh Gidra,
linux-kernel, Linux API, selinux, Andrea Arcangeli, Mike Rapoport,
Peter Xu, Jann Horn, LSM List
In-Reply-To: <202002112332.BE71455@keescook>
Thanks for taking a look and for the fast reply!
On Tue, Feb 11, 2020 at 11:51 PM Kees Cook <keescook@chromium.org> wrote:
>
> Hi!
>
> Firstly, thanks for working on this! It's been on my TODO list for a
> while. :)
>
> Casey already recommended including the LSM list to CC (since this is a
> new LSM -- there are many LSMs). Additionally, the series should
> probably be sent _to_ the userfaultfd maintainers:
> Andrea Arcangeli <aarcange@redhat.com>
> Mike Rapoport <rppt@linux.ibm.com>
> and I'd also CC a couple other people that have done recent work:
> Peter Xu <peterx@redhat.com>
> Jann Horn <jannh@google.com>
>
> More notes below...
In general, in the event that a patch series doesn't include all
needed parties on the to-line, what's the right way to fix the
situation without spamming everyone and forking the thread? In this
case, since I'm splitting the patch series anyway, I can just expand
the to-line in the reroll.
> On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > Userfaultfd in unprivileged contexts could be potentially very
> > useful. We'd like to harden userfaultfd to make such unprivileged use
> > less risky. This patch series allows SELinux to manage userfaultfd
> > file descriptors and allows administrators to limit userfaultfd to
> > servicing user-mode faults, increasing the difficulty of using
> > userfaultfd in exploit chains invoking delaying kernel faults.
>
> I actually think these are two very different goals and likely the
> series could be split into two for them. One is LSM hooking of
> userfaultfd and the SELinux attachment, and the other is the user-mode
> fault restrictions. And they would likely go via separate trees (LSM
> through James's LSM tree, and probably akpm's -mm tree for the sysctl).
>
> > A new anon_inodes interface allows callers to opt into SELinux
> > management of anonymous file objects. In this mode, anon_inodes
> > creates new ephemeral inodes for anonymous file objects instead of
> > reusing a singleton dummy inode. A new LSM hook gives security modules
> > an opportunity to configure and veto these ephemeral inodes.
> >
> > Existing anon_inodes users must opt into the new functionality.
> >
> > Daniel Colascione (6):
> > Add a new flags-accepting interface for anonymous inodes
> > Add a concept of a "secure" anonymous file
> > Teach SELinux about a new userfaultfd class
> > Wire UFFD up to SELinux
>
> The above is the first "series"... I don't have much opinion about it,
> though I do like the idea of making userfaultfd visible to the LSM.
Yeah. The interesting part there is the anon_inodes API change. I'll
split that half of the series out.
> > Let userfaultfd opt out of handling kernel-mode faults
> > Add a new sysctl for limiting userfaultfd to user mode faults
>
> Now this I'm very interested in. Can you go into more detail about two
> things:
>
> - What is the threat being solved? (I understand the threat, but detailing
> it in the commit log is important for people who don't know it. Existing
> commit cefdca0a86be517bc390fc4541e3674b8e7803b0 gets into some of the
> details already, but I'd like to see reference to external sources like
> https://duasynt.com/blog/linux-kernel-heap-spray)
Sure. I can add a reference to that and a more general discussion of
how delaying kernel fault handling can broaden race windows for other
exploits.
> - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> sysctl? (And should this maybe just be another setting for that
> sysctl, like "2"?)
We want to use UFFD for a new garbage collector in Android, and we
want unprivileged processes to be able to use this new garbage
collector. Giving them CAP_SYS_PTRACE is dangerous.
> As to the mechanics of the change, I'm not sure I like the idea of adding
> a UAPI flag for this. Why not just retain the permission check done at
> open() and if kernelmode faults aren't allowed, ignore them? This would
> require no changes to existing programs and gains the desired defense.
As Jann mentions below, it's a matter of the kernel's contractual
obligation to userspace. Right now, userfaultfd(2) either succeeds or
it fails with one of the enumerated error codes. You're proposing
having the userfaultfd(2) succeed but return a file descriptor that
doesn't do the job the kernel promised it would do. If we were to
adopt your proposal, an application would see UFFD succeed, then see
unexpeced EFAULTs from system calls, which would probably cause the
application to malfunctioning in exciting ways. An explicit "sorry, I
can't do that" error code is better: an application that gets an error
code from userfaultfd(2) can fall back to something else, but an
application that silently gets an underfeatured UFFD doesn't know
anything is wrong until it's too late. The additional flag preserves
the UFFD contract and gives applications a way to probe for feature
support.
> (And, I think, the sysctl value could be bumped to "2" as that's a
> better default state -- does qemu actually need kernelmode traps?)
I prefer a default of one for regular systems because I don't want to
make experimentation with novel ways to use UFFD more difficult, and a
default of two would require users go out of their way to handle user
faults, and few will. For a more constrained system like Android, two
is fine.
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Peter Xu @ 2020-02-12 17:14 UTC (permalink / raw)
To: Jann Horn
Cc: Kees Cook, Daniel Colascione, Tim Murray, Nosh Minwalla,
Nick Kralevich, Lokesh Gidra, kernel list, Linux API,
SElinux list, Andrea Arcangeli, Mike Rapoport,
linux-security-module
In-Reply-To: <CAG48ez0ogRxvCK1aCnviN+nBqp6gmbUD7NjaMKvA7bF=esAc1A@mail.gmail.com>
On Wed, Feb 12, 2020 at 05:54:35PM +0100, Jann Horn wrote:
> On Wed, Feb 12, 2020 at 8:51 AM Kees Cook <keescook@chromium.org> wrote:
> > On Tue, Feb 11, 2020 at 02:55:41PM -0800, Daniel Colascione wrote:
> > > Let userfaultfd opt out of handling kernel-mode faults
> > > Add a new sysctl for limiting userfaultfd to user mode faults
> >
> > Now this I'm very interested in. Can you go into more detail about two
> > things:
> [...]
> > - Why is this needed in addition to the existing vm.unprivileged_userfaultfd
> > sysctl? (And should this maybe just be another setting for that
> > sysctl, like "2"?)
> >
> > As to the mechanics of the change, I'm not sure I like the idea of adding
> > a UAPI flag for this. Why not just retain the permission check done at
> > open() and if kernelmode faults aren't allowed, ignore them? This would
> > require no changes to existing programs and gains the desired defense.
> > (And, I think, the sysctl value could be bumped to "2" as that's a
> > better default state -- does qemu actually need kernelmode traps?)
>
> I think this might be necessary for I/O emulation? As in, if before
> getting migrated, the guest writes some data into a buffer, then the
> guest gets migrated, and then while the postcopy migration stuff is
> still running, the guest tells QEMU to write that data from
> guest-physical memory to disk or whatever; I think in that case, QEMU
> will do something like a pwrite() syscall where the userspace pointer
> points into the memory area containing guest-physical memory, which
> would return -EFAULT if userfaultfd was restricted to userspace
> accesses.
>
> This was described in this old presentation about why userfaultfd is
> better than a SIGSEGV handler:
> https://drive.google.com/file/d/0BzyAwvVlQckeSzlCSDFmRHVybzQ/view
> (slide 6) (recording at https://youtu.be/pC8cWWRVSPw?t=463)
Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
be accessed by any kernel component on the destination host during a
postcopy procedure. It can be as simple as when a vcpu writes to a
missing guest page which still resides on the source host, then KVM
will get a page fault and trap into userfaultfd asking for that page.
The same thing happens to other modules like vhost, etc., as long as a
missing guest page is touched by a kernel module.
Thanks,
--
Peter Xu
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-12 18:45 UTC (permalink / raw)
To: Eric W. Biederman
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <87tv3vkg1a.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> Fundamentally proc_flush_task is an optimization. Just getting rid of
> dentries earlier. At least at one point it was an important
> optimization because the old process dentries would just sit around
> doing nothing for anyone.
I'm pretty sure it's still important. It's very easy to generate a
_ton_ of dentries with /proc.
> I wonder if instead of invalidating specific dentries we could instead
> fire wake up a shrinker and point it at one or more instances of proc.
It shouldn't be the dentries themselves that are a freeing problem.
They're being RCU-free'd anyway because of lookup. It's the
proc_mounts list that is the problem, isn't it?
So it's just fs_info that needs to be rcu-delayed because it contains
that list. Or is there something else?
Linus
^ permalink raw reply
* Re: [PATCH 00/24] user_namespace: introduce fsid mappings
From: Jann Horn @ 2020-02-12 18:53 UTC (permalink / raw)
To: Christian Brauner
Cc: linux-security-module, Kees Cook, Jonathan Corbet, kernel list,
Linux Containers, smbarber, Alexander Viro, Linux API,
linux-fsdevel, Alexey Dobriyan, Eric W. Biederman
In-Reply-To: <20200212145149.zohmc6d3x52bw6j6@wittgenstein>
On Wed, Feb 12, 2020 at 3:51 PM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
> On Tue, Feb 11, 2020 at 09:55:46PM +0100, Jann Horn via Containers wrote:
> > On Tue, Feb 11, 2020 at 5:59 PM Christian Brauner
> > <christian.brauner@ubuntu.com> wrote:
> > > This is the implementation of shiftfs which was cooked up during lunch at
> > > Linux Plumbers 2019 the day after the container's microconference. The
> > > idea is a design-stew from Stéphane, Aleksa, Eric, and myself. Back then
> > > we all were quite busy with other work and couldn't really sit down and
> > > implement it. But I took a few days last week to do this work, including
> > > demos and performance testing.
> > > This implementation does not require us to touch the vfs substantially
> > > at all. Instead, we implement shiftfs via fsid mappings.
> > > With this patch, it took me 20 mins to port both LXD and LXC to support
> > > shiftfs via fsid mappings.
> > >
> > > For anyone wanting to play with this the branch can be pulled from:
> > > https://github.com/brauner/linux/tree/fsid_mappings
> > > https://gitlab.com/brauner/linux/-/tree/fsid_mappings
> > > https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=fsid_mappings
> > >
> > > The main use case for shiftfs for us is in allowing shared writable
> > > storage to multiple containers using non-overlapping id mappings.
> > > In such a scenario you want the fsids to be valid and identical in both
> > > containers for the shared mount. A demo for this exists in [3].
> > > If you don't want to read on, go straight to the other demos below in
> > > [1] and [2].
> >
> > I guess essentially this means that you want to have UID separation
> > between containers to prevent the containers - or their owners - from
> > interfering between each other, but for filesystem access, you don't
> > want to isolate them from each other using DAC controls on the files
> > and folders inside the containers' directory hierarchies, instead
> > relying on mode-0700 parent directories to restrict access to the
> > container owner? Or would you still have separate UIDs for e.g. the
> > container's UID range 0-65535, and then map the shared UID range at
> > 100000, or something like that?
>
> Yes.
> So if you look at the permissions right now for the directory under
> which the rootfs for the container and other stuff resides we have
> root@wittgenstein|/var/lib/lxd/storage-pools/zfs/containers
> > perms *
> d--x------ 100 alp1
> d--x------ 100 f1
> d--x------ 100 f2
>
> We don't really share the rootfs between containers right now since we
> treat them as standalone systems but with fsid mappings that's possible
> too. Layer-sharing-centric runtimes very much will want something like
> that.
[...]
> > > With this patch series we simply introduce the ability to create fsid
> > > mappings that are different from the id mappings of a user namespace.
> > >
> > > In the usual case of running an unprivileged container we will have
> > > setup an id mapping, e.g. 0 100000 100000. The on-disk mapping will
> > > correspond to this id mapping, i.e. all files which we want to appear as
> > > 0:0 inside the user namespace will be chowned to 100000:100000 on the
> > > host. This works, because whenever the kernel needs to do a filesystem
> > > access it will lookup the corresponding uid and gid in the idmapping
> > > tables of the container.
> > > Now think about the case where we want to have an id mapping of 0 100000
> > > 100000 but an on-disk mapping of 0 300000 100000 which is needed to e.g.
> > > share a single on-disk mapping with multiple containers that all have
> > > different id mappings.
> > > This will be problematic. Whenever a filesystem access is requested, the
> > > kernel will now try to lookup a mapping for 300000 in the id mapping
> > > tables of the user namespace but since there is none the files will
> > > appear to be owned by the overflow id, i.e. usually 65534:65534 or
> > > nobody:nogroup.
> > >
> > > With fsid mappings we can solve this by writing an id mapping of 0
> > > 100000 100000 and an fsid mapping of 0 300000 100000. On filesystem
> > > access the kernel will now lookup the mapping for 300000 in the fsid
> > > mapping tables of the user namespace. And since such a mapping exists,
> > > the corresponding files will have correct ownership.
> >
> > Sorry to bring up something as disgusting as setuid execution, but:
>
> No that's exactly what this needs. :)
>
> > What happens when there's a setuid root file with ->i_uid==300000? I
> > guess the only way to make that work inside the containers would be
> > something like make_kuid(current_user_ns(),
> > from_kfsuid(current_user_ns(), inode->i_uid)) in the setuid execve
> > path?
>
> What's the specific callpath you're thinking about?
>
> So if you look at patch
> https://lore.kernel.org/lkml/20200211165753.356508-16-christian.brauner@ubuntu.com/
> it does
> - new->suid = new->fsuid = new->euid;
> - new->sgid = new->fsgid = new->egid;
> + fsuid = from_kuid_munged(new->user_ns, new->euid);
> + kfsuid = make_kfsuid(new->user_ns, fsuid);
> + new->suid = new->euid;
> + new->fsuid = kfsuid;
> +
> + fsgid = from_kgid_munged(new->user_ns, new->egid);
> + kfsgid = make_kfsgid(new->user_ns, fsgid);
> + new->sgid = new->egid;
> + new->fsgid = kfsgid;
Aaah, okay, I missed that.
> One thing I definitely missed though in the setuid path is to adapt
> fs/exec.c:bprm_fill_uid():
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 74d88dab98dd..ad839934fdff 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1547,8 +1547,8 @@ static void bprm_fill_uid(struct linux_binprm *bprm)
> inode_unlock(inode);
>
> /* We ignore suid/sgid if there are no mappings for them in the ns */
> - if (!kuid_has_mapping(bprm->cred->user_ns, uid) ||
> - !kgid_has_mapping(bprm->cred->user_ns, gid))
> + if (!kfsuid_has_mapping(bprm->cred->user_ns, uid) ||
> + !kfsgid_has_mapping(bprm->cred->user_ns, gid))
> return;
>
> if (mode & S_ISUID) {
[...]
> > I want to open /proc/$pid/personality of another process with the same
> > UIDs, may_open() will call inode_permission() -> do_inode_permission()
> > -> generic_permission() -> acl_permission_check(), which will compare
> > current_fsuid() (which is 300000) against inode->i_uid. But
> > inode->i_uid was filled by proc_pid_make_inode()->task_dump_owner(),
> > which set inode->i_uid to 100000, right?
>
> Yes. That should be fixable by something like below, I think. (And we can
> probably shortcut this by adding a helper that does tell us whether there's
> been any fsid mapping setup or not for this user namespace.)
> static int acl_permission_check(struct inode *inode, int mask)
> {
> + kuid_t kuid;
> unsigned int mode = inode->i_mode;
>
> - if (likely(uid_eq(current_fsuid(), inode->i_uid)))
> + if (!is_userns_visible(inode->i_sb->s_iflags)) {
> + kuid = inode->i_uid;
> + } else {
> + kuid = make_kuid(current_user_ns(),
> + from_kfsuid(current_user_ns(), inode->i_uid));
> + }
> +
> + if (likely(uid_eq(current_fsuid(), kuid)))
> mode >>= 6;
> else {&& (mode & S_IRWXG)) {
>
> >
> > Also, e.g. __ptrace_may_access() uses cred->fsuid for a comparison
> > with another task's real/effective/saved UID.
>
> Right, you even introduced this check in 2015 iirc.
> Both of your points make me think that it'd be easiest to introduce
> cred->{kfsuid,kfsgid} and whenever an access decision on a
> is_userns_visible() filesystem has to be made those will be used. This avoids
> having to do on-the fly translations
I guess that might be less ugly.
> and ptrace_may_access() can just grow a
> flag indicating what fscreds it's supposed to look at?
Wouldn't you always end up using the "real" fsuid there?
^ permalink raw reply
* Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM]
From: Casey Schaufler @ 2020-02-12 18:59 UTC (permalink / raw)
To: KP Singh
Cc: Alexei Starovoitov, Daniel Borkmann, Jann Horn, kernel list, bpf,
linux-security-module, Brendan Jackman, Florent Revest,
Thomas Garnier, Alexei Starovoitov, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Kernel Team, Casey Schaufler
In-Reply-To: <20200212162613.GB259057@google.com>
On 2/12/2020 8:26 AM, KP Singh wrote:
> On 12-Feb 07:52, Casey Schaufler wrote:
>> On 2/11/2020 6:45 PM, Alexei Starovoitov wrote:
>>> On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote:
>>>> Another approach could be to have a special nop inside call_int_hook()
>>>> macro which would then get patched to avoid these situations. Somewhat
>>>> similar like static keys where it could be defined anywhere in text but
>>>> with updating of call_int_hook()'s RC for the verdict.
>> Tell me again why you can't register your BPF hooks like all the
>> other security modules do? You keep reintroducing BPF as a special
>> case, and I don't see why.
> I think we tried to answer this in the discussion we had:
>
> https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#meb1eea982e63be0806f9bba58e91160871803752
I understand your arguments, but remain unconvinced.
> BPF should not allocate a wrapper (to be statically regsitered at
> init) for each LSM hook and run the programs from within that as this
> implies adding overhead across the board for every hook even if
> it's never used (i.e. no BPF program is attached to the hook).
SELinux would run faster if it didn't have hooks installed where
there is no policy loaded that would ever fail for them. That's
not the infrastructure's problem.
> We can, with the suggestions discussed here, avoid adding unncessary
> overhead for unused hooks. And, as Alexei mentioned, adding overhead
> when not really needed is especially bad for LSM hooks like
> sock_sendmsg.
You're adding overhead for systems that have BPF built, but not used.
> The other LSMs do not provide dynamic / mutable hooks, so it makes
> sense for them to register the hooks once at load time.
As mentioned above, the hooks may not be mutable, but policy
may make them pointless. That is the security module's problem,
not the infrastructure's.
>
> - KP
>
>>> Sounds nice in theory. I couldn't quite picture how that would look
>>> in the code, so I hacked:
>>> diff --git a/security/security.c b/security/security.c
>>> index 565bc9b67276..ce4bc1e5e26c 100644
>>> --- a/security/security.c
> [...]
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-12 19:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wg52stFtUxMOxs3afkwDWmWn1JXC7RJ7dPsTrJbnxpZVg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
>>
>> Fundamentally proc_flush_task is an optimization. Just getting rid of
>> dentries earlier. At least at one point it was an important
>> optimization because the old process dentries would just sit around
>> doing nothing for anyone.
>
> I'm pretty sure it's still important. It's very easy to generate a
> _ton_ of dentries with /proc.
>
>> I wonder if instead of invalidating specific dentries we could instead
>> fire wake up a shrinker and point it at one or more instances of proc.
>
> It shouldn't be the dentries themselves that are a freeing problem.
> They're being RCU-free'd anyway because of lookup. It's the
> proc_mounts list that is the problem, isn't it?
>
> So it's just fs_info that needs to be rcu-delayed because it contains
> that list. Or is there something else?
The fundamental dcache thing we are playing with is:
dentry = d_hash_and_lookup(proc_root, &name);
if (dentry) {
d_invalidate(dentry);
dput(dentry);
}
As Al pointed out upthread dput and d_invalidate can both sleep.
The dput can potentially go away if we use __d_lookup_rcu instead of
d_lookup.
The challenge is d_invalidate.
It has the fundamentally sleeping detach_mounts loop. Even
shrink_dcache_parent has a cond_sched() in there to ensure it doesn't
live lock the system.
We could and arguabley should set DCACHE_CANT_MOUNT on the proc pid
dentries. Which will prevent having to deal with mounts.
But I don't see an easy way of getting shrink_dcache_parent to run
without sleeping. Ideas?
Eric
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Andrea Arcangeli @ 2020-02-12 19:41 UTC (permalink / raw)
To: Peter Xu
Cc: Jann Horn, Kees Cook, Daniel Colascione, Tim Murray,
Nosh Minwalla, Nick Kralevich, Lokesh Gidra, kernel list,
Linux API, SElinux list, Mike Rapoport, linux-security-module
In-Reply-To: <20200212171416.GD1083891@xz-x1>
Hello everyone,
On Wed, Feb 12, 2020 at 12:14:16PM -0500, Peter Xu wrote:
> Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
> be accessed by any kernel component on the destination host during a
> postcopy procedure. It can be as simple as when a vcpu writes to a
> missing guest page which still resides on the source host, then KVM
> will get a page fault and trap into userfaultfd asking for that page.
> The same thing happens to other modules like vhost, etc., as long as a
> missing guest page is touched by a kernel module.
Correct.
How does the android garbage collection work to make sure there cannot
be kernel faults on the missing memory?
If I understood correctly (I didn't have much time to review sorry)
what's proposed with regard to limiting uffd events from kernel
faults, the only use case I know that could deal with it is the
UFFD_FEATURE_SIGBUS but that's not normal userfaultfd: that's also the
only feature required from uffd to implement a pure malloc library in
userland that never takes the mmap sem for writing to implement
userland mremap/mmap/munmap lib calls (as those will convert to
UFFDIO_ZEROPAGE and MADV_DONTNEED internally to the lib and there will
be always a single vma). We just need to extend UFFDIO_ZEROPAGE to map
the THP zeropage to make this future pure-uffd malloc lib perform
better.
On the other end I'm also planning a mremap_vma_merge userland syscall
that will merge fragmented vmas.
Currently once you have a nice heap all contiguous but with small
objects and you free the fragments you can't build THP anymore even if
you make the memory virtually contiguous again by calling mremap. That
just build up a ton of vmas slowing down the app forever and also
preventing THP collapsing ever again.
mremap_vma_merge will require no new kernel feature, but it
fundamentally must be able to handle kernel faults. If databases
starts to use that, how can you enable this feature without breaking
random apps then?
So it'd be a feature usable only by one user (Android) perhaps? And
only until you start defragging the vmas of small objects?
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-12 19:47 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wg52stFtUxMOxs3afkwDWmWn1JXC7RJ7dPsTrJbnxpZVg@mail.gmail.com>
On Wed, Feb 12, 2020 at 10:45:06AM -0800, Linus Torvalds wrote:
> On Wed, Feb 12, 2020 at 7:01 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> >
> > Fundamentally proc_flush_task is an optimization. Just getting rid of
> > dentries earlier. At least at one point it was an important
> > optimization because the old process dentries would just sit around
> > doing nothing for anyone.
>
> I'm pretty sure it's still important. It's very easy to generate a
> _ton_ of dentries with /proc.
>
> > I wonder if instead of invalidating specific dentries we could instead
> > fire wake up a shrinker and point it at one or more instances of proc.
>
> It shouldn't be the dentries themselves that are a freeing problem.
> They're being RCU-free'd anyway because of lookup. It's the
> proc_mounts list that is the problem, isn't it?
>
> So it's just fs_info that needs to be rcu-delayed because it contains
> that list. Or is there something else?
Large part of the headache is the possibility that some joker has
done something like mounting tmpfs on /proc/<pid>/map_files, or
binding /dev/null on top of /proc/<pid>/syscall, etc.
IOW, that d_invalidate() can very well have to grab namespace_sem.
And possibly do a full-blown fs shutdown of something NFS-mounted,
etc...
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-12 19:49 UTC (permalink / raw)
To: Eric W. Biederman
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <87v9obipk9.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 11:18 AM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> > So it's just fs_info that needs to be rcu-delayed because it contains
> > that list. Or is there something else?
>
> The fundamental dcache thing we are playing with is:
>
> dentry = d_hash_and_lookup(proc_root, &name);
> if (dentry) {
> d_invalidate(dentry);
> dput(dentry);
> }
Ahh. And we can't do that part under the RCU read lock. So it's not
the freeing, it's the list traversal itself.
Fair enough.
Hmm.
I wonder if we could split up d_invalidate(). It already ends up being
two phases: first the unhashing under the d_lock, and then the
recursive shrinking of parents and children.
The recursive shrinking of the parent isn't actually interesting for
the proc shrinking case: we just looked up one child, after all. So we
only care about the d_walk of the children.
So if we only did the first part under the RCU lock, and just
collected the dentries (can we perhaps then re-use the hash list to
collect them to another list?) and then did the child d_walk
afterwards?
Linus
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-12 20:03 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wgwmu4jpmOqW0+Lz0dcem1Fub=ThLHvmLobf_WqCq7bwg@mail.gmail.com>
On Wed, Feb 12, 2020 at 11:49:58AM -0800, Linus Torvalds wrote:
> I wonder if we could split up d_invalidate(). It already ends up being
> two phases: first the unhashing under the d_lock, and then the
> recursive shrinking of parents and children.
>
> The recursive shrinking of the parent isn't actually interesting for
> the proc shrinking case: we just looked up one child, after all. So we
> only care about the d_walk of the children.
>
> So if we only did the first part under the RCU lock, and just
> collected the dentries (can we perhaps then re-use the hash list to
> collect them to another list?) and then did the child d_walk
> afterwards?
What's to prevent racing with fs shutdown while you are doing the second part?
We could, after all, just have them[*] on procfs-private list (anchored in
task_struct) from the very beginning; evict on ->d_prune(), walk the list
on exit... How do you make sure the fs instance won't go away right under
you while you are doing the real work? Suppose you are looking at one
of those dentries and you've found something blocking to do. You can't
pin that dentry; you can pin ->s_active on its superblock (if it's already
zero, you can skip it - fs shutdown already in progress will take care of
the damn thing), but that will lead to quite a bit of cacheline pingpong...
[*] only /proc/<pid> and /proc/*/task/<pid> dentries, obviously.
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Daniel Colascione @ 2020-02-12 20:04 UTC (permalink / raw)
To: Andrea Arcangeli
Cc: Peter Xu, Jann Horn, Kees Cook, Tim Murray, Nosh Minwalla,
Nick Kralevich, Lokesh Gidra, kernel list, Linux API,
SElinux list, Mike Rapoport, linux-security-module
In-Reply-To: <20200212194100.GA29809@redhat.com>
On Wed, Feb 12, 2020 at 11:41 AM Andrea Arcangeli <aarcange@redhat.com> wrote:
>
> Hello everyone,
>
> On Wed, Feb 12, 2020 at 12:14:16PM -0500, Peter Xu wrote:
> > Right. AFAICT QEMU uses it far more than disk IOs. A guest page can
> > be accessed by any kernel component on the destination host during a
> > postcopy procedure. It can be as simple as when a vcpu writes to a
> > missing guest page which still resides on the source host, then KVM
> > will get a page fault and trap into userfaultfd asking for that page.
> > The same thing happens to other modules like vhost, etc., as long as a
> > missing guest page is touched by a kernel module.
>
> Correct.
>
> How does the android garbage collection work to make sure there cannot
> be kernel faults on the missing memory?
We don't pass pointers to the heap into system calls. (Big primitive
arrays, ByteBuffer, etc. are allocated off the regular heap.)
> If I understood correctly (I didn't have much time to review sorry)
> what's proposed with regard to limiting uffd events from kernel
> faults,
I don't understand what you mean. The purpose of preventing UFFD from
handling kernel faults is exploit mitigation.
> the only use case I know that could deal with it is the
> UFFD_FEATURE_SIGBUS but that's not normal userfaultfd: that's also the
> only feature required from uffd to implement a pure malloc library in
> userland that never takes the mmap sem for writing to implement
> userland mremap/mmap/munmap lib calls (as those will convert to
> UFFDIO_ZEROPAGE and MADV_DONTNEED internally to the lib and there will
> be always a single vma). We just need to extend UFFDIO_ZEROPAGE to map
> the THP zeropage to make this future pure-uffd malloc lib perform
> better.
The key requirement here is the ability to prevent unprivileged
processes from using UFFD to widen kernel exploit windows by
preventing UFFD from taking kernel faults. Forcing unprivileged
processes to use UFFD only with UFFD_FEATURE_SIGBUS would satisfy this
requirement, but it's much less flexible and unnecessarily couples two
features.
> On the other end I'm also planning a mremap_vma_merge userland syscall
> that will merge fragmented vmas.
This is probably a separate discussion, but does that operation really
need to be a system call? Historically, userspace has operated mostly
on page ranges and not VMAs per se, and the kernel has been free to
merge and split VMAs as needed for its internal purposes. This
approach has serious negative side effects (like making munmap
fallible: see [1]), but it is what it is.
[1] https://lore.kernel.org/linux-mm/CAKOZuetOD6MkGPVvYFLj5RXh200FaDyu3sQqZviVRhTFFS3fjA@mail.gmail.com/
> Currently once you have a nice heap all contiguous but with small
> objects and you free the fragments you can't build THP anymore even if
> you make the memory virtually contiguous again by calling mremap. That
> just build up a ton of vmas slowing down the app forever and also
> preventing THP collapsing ever again.
Shouldn't the THP background kthread take care of merging VMAs?
> mremap_vma_merge will require no new kernel feature, but it
> fundamentally must be able to handle kernel faults. If databases
> starts to use that, how can you enable this feature without breaking
> random apps then?
Presumably, those apps wouldn't issue the system call on address
ranges managed with a non-kernel-fault UFFD.
> So it'd be a feature usable only by one user (Android) perhaps? And
> only until you start defragging the vmas of small objects?
We shouldn't be fragmenting at all, either at the memory level or the
VMA level. The GC is a moving collector, and we don't punch holes in
the heap.
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-12 20:35 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200212200335.GO23230@ZenIV.linux.org.uk>
On Wed, Feb 12, 2020 at 12:03 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> What's to prevent racing with fs shutdown while you are doing the second part?
I was thinking that only the proc_flush_task() code would do this.
And that holds a ref to the vfsmount through upid->ns.
So I wasn't suggesting doing this in general - just splitting up the
implementation of d_invalidate() so that proc_flush_task_mnt() could
delay the complex part to after having traversed the RCU-protected
list.
But hey - I missed this part of the problem originally, so maybe I'm
just missing something else this time. Wouldn't be the first time.
Linus
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-12 20:38 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wi+1CPShMFvJNPfnrJ8DD8uVKUOQ5TQzQUNGLUkeoahkg@mail.gmail.com>
On Wed, Feb 12, 2020 at 12:35:04PM -0800, Linus Torvalds wrote:
> On Wed, Feb 12, 2020 at 12:03 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > What's to prevent racing with fs shutdown while you are doing the second part?
>
> I was thinking that only the proc_flush_task() code would do this.
>
> And that holds a ref to the vfsmount through upid->ns.
>
> So I wasn't suggesting doing this in general - just splitting up the
> implementation of d_invalidate() so that proc_flush_task_mnt() could
> delay the complex part to after having traversed the RCU-protected
> list.
>
> But hey - I missed this part of the problem originally, so maybe I'm
> just missing something else this time. Wouldn't be the first time.
Wait, I thought the whole point of that had been to allow multiple
procfs instances for the same userns? Confused...
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Al Viro @ 2020-02-12 20:41 UTC (permalink / raw)
To: Linus Torvalds
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200212203833.GQ23230@ZenIV.linux.org.uk>
On Wed, Feb 12, 2020 at 08:38:33PM +0000, Al Viro wrote:
> On Wed, Feb 12, 2020 at 12:35:04PM -0800, Linus Torvalds wrote:
> > On Wed, Feb 12, 2020 at 12:03 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
> > >
> > > What's to prevent racing with fs shutdown while you are doing the second part?
> >
> > I was thinking that only the proc_flush_task() code would do this.
> >
> > And that holds a ref to the vfsmount through upid->ns.
> >
> > So I wasn't suggesting doing this in general - just splitting up the
> > implementation of d_invalidate() so that proc_flush_task_mnt() could
> > delay the complex part to after having traversed the RCU-protected
> > list.
> >
> > But hey - I missed this part of the problem originally, so maybe I'm
> > just missing something else this time. Wouldn't be the first time.
>
> Wait, I thought the whole point of that had been to allow multiple
> procfs instances for the same userns? Confused...
s/userns/pidns/, sorry
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: david.safford @ 2020-02-12 21:08 UTC (permalink / raw)
To: Mimi Zohar, Janne Karhunen, linux-integrity,
linux-security-module
Cc: Ken Goldman, monty.wiseman, Amir Goldstein, linux-fsdevel
In-Reply-To: <1581462616.5125.69.camel@linux.ibm.com>
On Tue, 2020-02-11 at 18:10 -0500, Mimi Zohar wrote:
> On Tue, 2020-02-11 at 11:10 -0500, david.safford@gmail.com wrote:
> > <snip>
> >
> This new feature will require setting up some infrastructure for
> storing the partial measurement list(s) in order to validate a TPM
> quote. Userspace already can save partial measurement list(s) without
> any kernel changes. The entire measurement list does not need to be
> read each time. lseek can read past the last record previously read.
> The only new aspect is truncating the in kernel measurement list in
> order to free kernel memory.
This is a pretty important new feature.
A lot of people can't use IMA because of the memory issue.
Also, I really think we need to let administrators choose the tradeoffs
of keeping the list in memory, on a local file, or only on the
attestation server, as best fits their use cases.
>
> < snip>
>
> Until there is proof that the measurement list can be exported to a
> file before kexec, instead of carrying the measurement list across
> kexec, and a TPM quote can be validated after the kexec, there isn't a
> compelling reason for the kernel needing to truncate the measurement
> list.
If this approach doesn't work with all the kexec use cases, then it is
useless, and the ball is in my court to prove that it does. Fortunately
I have to test that anyway for the coming TLV support.
Working on it...
dave
> Mimi
>
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-12 21:02 UTC (permalink / raw)
To: Al Viro
Cc: Eric W. Biederman, LKML, Kernel Hardening, Linux API,
Linux FS Devel, Linux Security Module, Akinobu Mita,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <20200212204124.GR23230@ZenIV.linux.org.uk>
On Wed, Feb 12, 2020 at 12:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Wed, Feb 12, 2020 at 08:38:33PM +0000, Al Viro wrote:
> >
> > Wait, I thought the whole point of that had been to allow multiple
> > procfs instances for the same userns? Confused...
>
> s/userns/pidns/, sorry
Right, but we still hold the ref to it here...
[ Looks more ]
Oooh. No we don't. Exactly because we don't hold the lock, only the
rcu lifetime, the ref can go away from under us. I see what your
concern is.
Ouch, this is more painful than I expected - the code flow looked so
simple. I really wanted to avoid a new lock during process shutdown,
because that has always been somewhat painful.
Linus
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Eric W. Biederman @ 2020-02-12 21:46 UTC (permalink / raw)
To: Linus Torvalds
Cc: Al Viro, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <CAHk-=wi5FOGV_3tALK3n6E2fK3Oa_yCYkYQtCSaXLSEm2DUCKg@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Feb 12, 2020 at 12:41 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> On Wed, Feb 12, 2020 at 08:38:33PM +0000, Al Viro wrote:
>> >
>> > Wait, I thought the whole point of that had been to allow multiple
>> > procfs instances for the same userns? Confused...
>>
>> s/userns/pidns/, sorry
>
> Right, but we still hold the ref to it here...
>
> [ Looks more ]
>
> Oooh. No we don't. Exactly because we don't hold the lock, only the
> rcu lifetime, the ref can go away from under us. I see what your
> concern is.
>
> Ouch, this is more painful than I expected - the code flow looked so
> simple. I really wanted to avoid a new lock during process shutdown,
> because that has always been somewhat painful.
The good news is proc_flush_task isn't exactly called from process exit.
proc_flush_task is called during zombie clean up. AKA release_task.
So proc_flush_task isn't called with any locks held, and it is
called in a context where it can sleep.
Further after proc_flush_task does it's thing the code goes
and does "write_lock_irq(&task_list_lock);"
So the code is definitely serialized to one processor already.
What would be downside of having a mutex for a list of proc superblocks?
A mutex that is taken for both reading and writing the list.
Eric
^ permalink raw reply
* Re: [PATCH v2 0/6] Harden userfaultfd
From: Andrea Arcangeli @ 2020-02-12 23:41 UTC (permalink / raw)
To: Daniel Colascione
Cc: Peter Xu, Jann Horn, Kees Cook, Tim Murray, Nosh Minwalla,
Nick Kralevich, Lokesh Gidra, kernel list, Linux API,
SElinux list, Mike Rapoport, linux-security-module
In-Reply-To: <CAKOZuevusieaKCt5r-jnQ5ArGfw5Otszq2CAcrqFi6MYxkKwtA@mail.gmail.com>
Hi Daniel,
On Wed, Feb 12, 2020 at 12:04:39PM -0800, Daniel Colascione wrote:
> We don't pass pointers to the heap into system calls. (Big primitive
> arrays, ByteBuffer, etc. are allocated off the regular heap.)
That sounds pretty restrictive, I wonder what you gain by enforcing
that invariant or if it just happened incidentally for some other
reason? Do you need to copy that memory every time if you need to do
I/O on it? Are you sure this won't need to change any time soon to
increase performance?
> I don't understand what you mean. The purpose of preventing UFFD from
> handling kernel faults is exploit mitigation.
That part was clear. What wasn't clear is what the new feature
does exactly and what it blocks, because it's all about blocking or
how does it make things more secure?
> The key requirement here is the ability to prevent unprivileged
> processes from using UFFD to widen kernel exploit windows by
> preventing UFFD from taking kernel faults. Forcing unprivileged
> processes to use UFFD only with UFFD_FEATURE_SIGBUS would satisfy this
> requirement, but it's much less flexible and unnecessarily couples two
> features.
I mentioned it in case you could use something like that model.
> > On the other end I'm also planning a mremap_vma_merge userland syscall
> > that will merge fragmented vmas.
>
> This is probably a separate discussion, but does that operation really
> need to be a system call? Historically, userspace has operated mostly
mremap_vma_merge was not intended as a system call, if implemented as
a system call it wouldn't use uffd.
> on page ranges and not VMAs per se, and the kernel has been free to
Userland doesn't need to know anything.. unless it wants to optimize.
The userland can know full well if it does certain mremap operations
and puts its ranges virtually contiguous in a non linear way, so that
the kernel cannot merge the vmas.
> merge and split VMAs as needed for its internal purposes. This
> approach has serious negative side effects (like making munmap
> fallible: see [1]), but it is what it is.
>
> [1] https://lore.kernel.org/linux-mm/CAKOZuetOD6MkGPVvYFLj5RXh200FaDyu3sQqZviVRhTFFS3fjA@mail.gmail.com/
The fact it's fallible is a secondary concern here. Even if you make
it unlimited, if it grows it slowdown everything and also prevents THP
to be collapsed. Even the scalability of the mmap_sem worsens.
> > Currently once you have a nice heap all contiguous but with small
> > objects and you free the fragments you can't build THP anymore even if
> > you make the memory virtually contiguous again by calling mremap. That
> > just build up a ton of vmas slowing down the app forever and also
> > preventing THP collapsing ever again.
>
> Shouldn't the THP background kthread take care of merging VMAs?
The solution can't depend on any THP feature, because the buildup of
vmas is a scalability issue and a performance regression over time
even if THP is not configured in the kernel. However once that's
solved THP also gets naturally optimized.
What should happen (in my view) is just the simplest solution that can
defrag and forcefully merge the vmas without having to stop or restart
the app.
> Presumably, those apps wouldn't issue the system call on address
> ranges managed with a non-kernel-fault UFFD.
So the new security feature won't have to block kernel faults on those
apps and they can run side by side with the blocked app?
> We shouldn't be fragmenting at all, either at the memory level or the
> VMA level. The GC is a moving collector, and we don't punch holes in
> the heap.
That sounds good.
Thanks,
Andrea
^ permalink raw reply
* Re: [PATCH v8 07/11] proc: flush task dcache entries from all procfs instances
From: Linus Torvalds @ 2020-02-13 0:48 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Al Viro, LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexey Dobriyan,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Greg Kroah-Hartman, Ingo Molnar,
J . Bruce Fields, Jeff Layton, Jonathan Corbet, Kees Cook,
Oleg Nesterov, Solar Designer
In-Reply-To: <87lfp7h422.fsf@x220.int.ebiederm.org>
On Wed, Feb 12, 2020 at 1:48 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
>
> The good news is proc_flush_task isn't exactly called from process exit.
> proc_flush_task is called during zombie clean up. AKA release_task.
Yeah, that at least avoids some of the nasty locking while dying debug problems.
But the one I was more worried about was actually the lock contention
issue with lots of processes. The lock is basically a single global
lock in many situations - yes, it's technically per-ns, but in a lot
of cases you really only have one namespace anyway.
And we've had problems with global locks in this area before, notably
the one you call out:
> Further after proc_flush_task does it's thing the code goes
> and does "write_lock_irq(&task_list_lock);"
Yeah, so it's not introducing a new issue, but it is potentially
making something we already know is bad even worse.
> What would be downside of having a mutex for a list of proc superblocks?
> A mutex that is taken for both reading and writing the list.
That's what the original patch actually was, and I was hoping we could
avoid that thing.
An rwsem would be possibly better, since most cases by far are likely
about reading.
And yes, I'm very aware of the task_list_lock, but it's literally why
I don't want to make a new one.
I'm _hoping_ we can some day come up with something better than task_list_lock.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox