* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-24 17:23 UTC (permalink / raw)
To: Kees Cook
Cc: Casey Schaufler, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris
In-Reply-To: <202002211946.A23A987@keescook>
Hi Kees,
Thanks for the feedback!
On 21-Feb 20:22, Kees Cook wrote:
> On Thu, Feb 20, 2020 at 03:49:05PM -0800, Casey Schaufler wrote:
> > On 2/20/2020 9:52 AM, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> >
> > Sorry about the heavy list pruning - the original set
> > blows thunderbird up.
>
> (I've added some people back; I had to dig this thread back out of lkml
> since I didn't get a direct copy...)
>
> > > The BPF LSM programs are implemented as fexit trampolines to avoid the
> > > overhead of retpolines. These programs cannot be attached to security_*
> > > wrappers as there are quite a few security_* functions that do more than
> > > just calling the LSM callbacks.
> > >
> > > This was discussed on the lists in:
> > >
> > > https://lore.kernel.org/bpf/20200123152440.28956-1-kpsingh@chromium.org/T/#m068becce588a0cdf01913f368a97aea4c62d8266
> > >
> > > Adding a NOP callback after all the static LSM callbacks are called has
> > > the following benefits:
> > >
> > > - The BPF programs run at the right stage of the security_* wrappers.
> > > - They run after all the static LSM hooks allowed the operation,
> > > therefore cannot allow an action that was already denied.
> >
> > I still say that the special call-out to BPF is unnecessary.
> > I remain unconvinced by the arguments. You aren't doing anything
> > so special that the general mechanism won't work.
>
> If I'm understanding this correctly, there are two issues:
>
> 1- BPF needs to be run last due to fexit trampolines (?)
>
> 2- BPF hooks don't know what may be attached at any given time, so
> ALL LSM hooks need to be universally hooked. THIS turns out to create
> a measurable performance problem in that the cost of the indirect call
> on the (mostly/usually) empty BPF policy is too high.
>
> "1" can be solved a lot of ways, and doesn't seem to be a debated part
> of this series.
>
> "2" is interesting -- it creates a performance problem for EVERYONE that
> builds in this kernel feature, regardless of them using it. Excepting
> SELinux, "traditional" LSMs tends to be relatively sparse in their hooking:
>
> $ grep '^ struct hlist_head' include/linux/lsm_hooks.h | wc -l
> 230
> $ for i in apparmor loadpin lockdown safesetid selinux smack tomoyo yama ; \
> do echo -n "$i " && (cd $i && git grep LSM_HOOK_INIT | wc -l) ; done
> apparmor 68
> loadpin 3
> lockdown 1
> safesetid 2
> selinux 202
> smack 108
> tomoyo 28
> yama 4
>
> So, trying to avoid the indirect calls is, as you say, an optimization,
> but it might be a needed one due to the other limitations.
>
> To me, some questions present themselves:
>
> a) What, exactly, are the performance characteristics of:
> "before"
> "with indirect calls"
> "with static keys optimization"
Good suggestion!
I will do some analysis and come back with the numbers.
>
> b) Would there actually be a global benefit to using the static keys
> optimization for other LSMs? (Especially given that they're already
> sparsely populated and policy likely determines utility -- all the
> LSMs would just turn ON all their static keys or turn off ALL their
> static keys depending on having policy loaded.)
As Alexei mentioned, we can use the patches for static calls after
they are merged:
https://lore.kernel.org/lkml/8bc857824f82462a296a8a3c4913a11a7f801e74.1547073843.git.jpoimboe@redhat.com/
to make the framework better (as a separate series) especially given
that we are unsure how they work with BPF.
- KP
>
> If static keys are justified for KRSI (by "a") then it seems the approach
> here should stand. If "b" is also true, then we need an additional
> series to apply this optimization for the other LSMs (but that seems
> distinctly separate from THIS series).
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH v15 02/23] LSM: Create and manage the lsmblob data structure.
From: Mimi Zohar @ 2020-02-24 17:56 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux, Janne Karhunen
Cc: keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20200214234203.7086-3-casey@schaufler-ca.com>
[Cc'ing Janne Karhunen]
On Fri, 2020-02-14 at 15:41 -0800, Casey Schaufler wrote:
<snip>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 453427048999..624ed1a34842 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -75,7 +75,7 @@ struct ima_rule_entry {
> bool (*fowner_op)(kuid_t, kuid_t); /* uid_eq(), uid_gt(), uid_lt() */
> int pcr;
> struct {
> - void *rule; /* LSM file metadata specific */
> + void *rules[LSMBLOB_ENTRIES];
> void *args_p; /* audit value */
> int type; /* audit type */
> } lsm[MAX_LSM_RULES];
> @@ -84,6 +84,16 @@ struct ima_rule_entry {
> struct ima_template_desc *template;
> };
>
> +static inline bool ima_lsm_isset(void *rules[])
> +{
> + int i;
> +
> + for (i = 0; i < LSMBLOB_ENTRIES; i++)
> + if (rules[i])
> + return true;
> + return false;
> +}
> +
Even though ima_lsm_isset() is static, it should really be commented.
> /*
> * Without LSM specific knowledge, the default policy can only be
> * written in terms of .action, .func, .mask, .fsmagic, .uid, and .fowner
> @@ -258,9 +268,11 @@ __setup("ima_appraise_tcb", default_appraise_policy_setup);
> static void ima_lsm_free_rule(struct ima_rule_entry *entry)
> {
> int i;
> + int r;
>
> for (i = 0; i < MAX_LSM_RULES; i++) {
> - kfree(entry->lsm[i].rule);
> + for (r = 0; r < LSMBLOB_ENTRIES; r++)
> + kfree(entry->lsm[i].rules[r]);
> kfree(entry->lsm[i].args_p);
> }
> kfree(entry);
> @@ -295,8 +307,8 @@ static struct ima_rule_entry *ima_lsm_copy_rule(struct ima_rule_entry *entry)
> security_filter_rule_init(nentry->lsm[i].type,
> Audit_equal,
> nentry->lsm[i].args_p,
> - &nentry->lsm[i].rule);
> - if (!nentry->lsm[i].rule)
> + nentry->lsm[i].rules);
> + if (!ima_lsm_isset(nentry->lsm[i].rules))
> pr_warn("rule for LSM \'%s\' is undefined\n",
> (char *)entry->lsm[i].args_p);
Janne, the generic LSM message looks fine, but should there also be an
LSM specific warning the first time it changes?
Mimi
^ permalink raw reply
* Re: [PATCH v15 03/23] LSM: Use lsmblob in security_audit_rule_match
From: Mimi Zohar @ 2020-02-24 18:26 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20200214234203.7086-4-casey@schaufler-ca.com>
On Fri, 2020-02-14 at 15:41 -0800, Casey Schaufler wrote:
<snip>
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 624ed1a34842..ad48b7b60cff 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -476,6 +476,7 @@ static bool ima_match_rules(struct ima_rule_entry *rule, struct inode *inode,
> for (i = 0; i < MAX_LSM_RULES; i++) {
> int rc = 0;
> u32 osid;
> + struct lsmblob blob;
A new local lsmblob variable is defined here with the same name as the
function variable.
Mimi
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-24 18:45 UTC (permalink / raw)
To: KP Singh
Cc: Alexei Starovoitov, Kees Cook, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev, Casey Schaufler
In-Reply-To: <20200224171305.GA21886@chromium.org>
On 2/24/2020 9:13 AM, KP Singh wrote:
> On 24-Feb 08:32, Casey Schaufler wrote:
>> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
>>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
>>>> If I'm understanding this correctly, there are two issues:
>>>>
>>>> 1- BPF needs to be run last due to fexit trampolines (?)
>>> no.
>>> The placement of nop call can be anywhere.
>>> BPF trampoline is automagically converting nop call into a sequence
>>> of directly invoked BPF programs.
>>> No link list traversals and no indirect calls in run-time.
>> Then why the insistence that it be last?
> I think this came out of the discussion about not being able to
> override the other LSMs and introduce a new attack vector with some
> arguments discussed at:
>
> https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
>
> Let's say we have SELinux + BPF runnng on the system. BPF should still
> respect any decisions made by SELinux. This hasn't got anything to
> do with the usage of fexit trampolines.
The discussion sited is more about GPL than anything else.
The LSM rule is that any security module must be able to
accept the decisions of others. SELinux has to accept decisions
made ahead of it. It always has, as LSM checks occur after
"traditional" checks, which may fail. The only reason that you
need to be last in this implementation appears to be that you
refuse to use the general mechanisms. You can't blame SELinux
for that.
>>>> 2- BPF hooks don't know what may be attached at any given time, so
>>>> ALL LSM hooks need to be universally hooked. THIS turns out to create
>>>> a measurable performance problem in that the cost of the indirect call
>>>> on the (mostly/usually) empty BPF policy is too high.
>>> also no.
>> Um, then why not use the infrastructure as is?
> I think what Kees meant is that BPF allows hooking to all the LSM
> hooks and providing static LSM hook callbacks like traditional LSMs
> has a measurable performance overhead and this is indeed correct. This
> is why we want provide with the following characteristics:
I was responding to the "also no", which denies both what Kees said
and what you're saying here.
> - Without introducing a new attack surface, this was the reason for
> creating a mutable security_hook_heads in v1 which ran the hook
> after v1.
Yeah,
> This approach still had the issues of an indirect call and an
> extra check when not used. So this was not truly zero overhead even
> after special casing BPF.
The LSM mechanism is not zero overhead. It never has been. That's why
you can compile it out. You get added value at a price. You get the
ability to use SELinux and KRSI together at a price. If that's unacceptable
you can go the route of seccomp, which doesn't use LSM for many of the
same reasons you're on about.
When LSM was introduced it was expected to be used by the lunatic fringe
people with government mandated security requirements. Today it has a
much greater general application. That's why I'm in the process of
bringing it up to modern use case requirements. Performance is much
more important now than it was before the use of LSM became popular.
> - Having a truly zero performance overhead on the system. There are
> other tracing / attachment mechnaisms in the kernel which provide
> similar guarrantees (using static keys and direct calls) and it
> seems regressive for KRSI to not leverage these known patterns and
> sacrifice performance espeically in hotpaths. This provides users
> to use KRSI alongside other LSMs without paying extra cost for all
> the possible hooks.
This is in direct conflict with the aforementioned "also no".
>>>> So, trying to avoid the indirect calls is, as you say, an optimization,
>>>> but it might be a needed one due to the other limitations.
>>> I'm convinced that avoiding the cost of retpoline in critical path is a
>>> requirement for any new infrastructure in the kernel.
>> Sorry, I haven't gotten that memo.
>>
>>> Not only for security, but for any new infra.
>> The LSM infrastructure ain't new.
> But the ability to attach BPF programs to LSM hooks is new.
Stop right there. No, I mean it. Really, stop right there.
I don't give a flying fig (he said, using the polite expression
rather than the vulgar) about what you want to do within a
security module. Attach a BPF program, randomize arbitrary
memory locations, do traditional Bell & LaPadula, it's all the
same to the LSM infrastructure. If you want to do something
that has to work outside that, the way audit and seccomp do,
you need to take that out of the LSM infrastructure. If you
want the convenience of the LSM infrastructure you don't get
to muck it up.
> Also, we
> have not had the whole implementation of the LSM hook be mutable
> before and this is essentially what the KRSI provides.
It can do that wholly within KRSI hooks. You don't need to
put KRSI specific code into security.c.
>>> Networking stack converted all such places to conditional calls.
>>> In BPF land we converted indirect calls to direct jumps and direct calls.
>>> It took two years to do so. Adding new indirect calls is not an option.
>>> I'm eagerly waiting for Peter's static_call patches to land to convert
>>> a lot more indirect calls. May be existing LSMs will take advantage
>>> of static_call patches too, but static_call is not an option for BPF.
>>> That's why we introduced BPF trampoline in the last kernel release.
>> Sorry, but I don't see how BPF is so overwhelmingly special.
> My analogy here is that if every tracepoint in the kernel were of the
> type:
>
> if (trace_foo_enabled) // <-- Overhead here, solved with static key
> trace_foo(a); // <-- retpoline overhead, solved with fexit trampolines
>
> It would be very hard to justify enabling them on a production system,
> and the same can be said for BPF and KRSI.
The same can be and has been said about the LSM infrastructure.
If BPF and KRSI are that performance critical you shouldn't be
tying them to LSM, which is known to have overhead. If you can't
accept the LSM overhead, get out of the LSM. Or, help us fix the
LSM infrastructure to make its overhead closer to zero. Whether
you believe it or not, a lot of work has gone into keeping the LSM
overhead as small as possible while remaining sufficiently general
to perform its function.
No. If you're too special to play by LSM rules then you're special
enough to get into the kernel using more direct means.
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Kees Cook @ 2020-02-24 21:41 UTC (permalink / raw)
To: Casey Schaufler
Cc: KP Singh, Alexei Starovoitov, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev
In-Reply-To: <00c216e1-bcfd-b7b1-5444-2a2dfa69190b@schaufler-ca.com>
On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
> On 2/24/2020 9:13 AM, KP Singh wrote:
> > On 24-Feb 08:32, Casey Schaufler wrote:
> >> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> >>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> >>>> If I'm understanding this correctly, there are two issues:
> >>>>
> >>>> 1- BPF needs to be run last due to fexit trampolines (?)
> >>> no.
> >>> The placement of nop call can be anywhere.
> >>> BPF trampoline is automagically converting nop call into a sequence
> >>> of directly invoked BPF programs.
> >>> No link list traversals and no indirect calls in run-time.
> >> Then why the insistence that it be last?
> > I think this came out of the discussion about not being able to
> > override the other LSMs and introduce a new attack vector with some
> > arguments discussed at:
> >
> > https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
> >
> > Let's say we have SELinux + BPF runnng on the system. BPF should still
> > respect any decisions made by SELinux. This hasn't got anything to
> > do with the usage of fexit trampolines.
>
> The discussion sited is more about GPL than anything else.
>
> The LSM rule is that any security module must be able to
> accept the decisions of others. SELinux has to accept decisions
> made ahead of it. It always has, as LSM checks occur after
> "traditional" checks, which may fail. The only reason that you
> need to be last in this implementation appears to be that you
> refuse to use the general mechanisms. You can't blame SELinux
> for that.
Okay, this is why I wanted to try to state things plainly. The "in last
position" appears to be the result of a couple design choices:
-the idea of "not wanting to get in the way of other LSMs", while
admirable, needs to actually be a non-goal to be "just" a stacked LSM
(as you're saying here Casey). This position _was_ required for the
non-privileged LSM case to avoid security implications, but that goal
not longer exists here either.
-optimally using the zero-cost call-outs (static key + fexit trampolines)
meant it didn't interact well with the existing stacking mechanism.
So, fine, these appear to be design choices, not *specifically*
requirements. Let's move on, I think there is more to unpack here...
> >>>> 2- BPF hooks don't know what may be attached at any given time, so
> >>>> ALL LSM hooks need to be universally hooked. THIS turns out to create
> >>>> a measurable performance problem in that the cost of the indirect call
> >>>> on the (mostly/usually) empty BPF policy is too high.
> >>> also no.
AIUI, there was some confusion on Alexei's reply here. I, perhaps,
was not as clear as I needed to be. I think the later discussion on
performance overheads gets more into the point, and gets us closer to
the objections Alexei had. More below...
> > This approach still had the issues of an indirect call and an
> > extra check when not used. So this was not truly zero overhead even
> > after special casing BPF.
>
> The LSM mechanism is not zero overhead. It never has been. That's why
> you can compile it out. You get added value at a price. You get the
> ability to use SELinux and KRSI together at a price. If that's unacceptable
> you can go the route of seccomp, which doesn't use LSM for many of the
> same reasons you're on about.
> [...]
> >>>> So, trying to avoid the indirect calls is, as you say, an optimization,
> >>>> but it might be a needed one due to the other limitations.
> >>> I'm convinced that avoiding the cost of retpoline in critical path is a
> >>> requirement for any new infrastructure in the kernel.
> >> Sorry, I haven't gotten that memo.
I agree with Casey here -- it's a nice goal, but those cost evaluations have
not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
sure, but this does appear to be an early optimization.
> [...]
> It can do that wholly within KRSI hooks. You don't need to
> put KRSI specific code into security.c.
This observation is where I keep coming back to.
Yes, the resulting code is not as fast as it could be. The fact that BPF
triggers the worst-case performance of LSM hooking is the "new" part
here, from what I can see.
I suspect the problem is that folks in the BPF subsystem don't want to
be seen as slowing anything down, even other subsystems, so they don't
want to see this done in the traditional LSM hooking way (which contains
indirect calls).
But the LSM subsystem doesn't want special cases (Casey has worked very
hard to generalize everything there for stacking). It is really hard to
accept adding a new special case when there are still special cases yet
to be worked out even in the LSM code itself[2].
> >>> Networking stack converted all such places to conditional calls.
> >>> In BPF land we converted indirect calls to direct jumps and direct calls.
> >>> It took two years to do so. Adding new indirect calls is not an option.
> >>> I'm eagerly waiting for Peter's static_call patches to land to convert
> >>> a lot more indirect calls. May be existing LSMs will take advantage
> >>> of static_call patches too, but static_call is not an option for BPF.
> >>> That's why we introduced BPF trampoline in the last kernel release.
> >> Sorry, but I don't see how BPF is so overwhelmingly special.
> > My analogy here is that if every tracepoint in the kernel were of the
> > type:
> >
> > if (trace_foo_enabled) // <-- Overhead here, solved with static key
> > trace_foo(a); // <-- retpoline overhead, solved with fexit trampolines
This is a helpful distillation; thanks.
static keys (perhaps better described as static branches) make sense to
me; I'm familiar with them being used all over the place[3]. The resulting
"zero performance" branch mechanism is extremely handy.
I had been thinking about the fexit stuff only as a way for BPF to call
into kernel functions directly, and I missed the place where this got
used for calling from the kernel into BPF directly. KP walked me through
the fexit stuff off list. I missed where there NOP stub ("noinline int
bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
The key bit being "bpf_trampoline_update(prog)"
> > It would be very hard to justify enabling them on a production system,
> > and the same can be said for BPF and KRSI.
>
> The same can be and has been said about the LSM infrastructure.
> If BPF and KRSI are that performance critical you shouldn't be
> tying them to LSM, which is known to have overhead. If you can't
> accept the LSM overhead, get out of the LSM. Or, help us fix the
> LSM infrastructure to make its overhead closer to zero. Whether
> you believe it or not, a lot of work has gone into keeping the LSM
> overhead as small as possible while remaining sufficiently general
> to perform its function.
>
> No. If you're too special to play by LSM rules then you're special
> enough to get into the kernel using more direct means.
So, I see the primary conflict here being about the performance
optimizations. AIUI:
- BPF subsystem maintainers do not want any new slowdown associated
with BPF
- LSM subsystem maintainers do not want any new special cases in
LSM stacking
So, unless James is going to take this over Casey's objections, the path
forward I see here is:
- land a "slow" KRSI (i.e. one that hooks every hook with a stub).
- optimize calling for all LSMs
Does this seem right to everyone?
-Kees
[1] There is a "known cost to LSM", but as Casey mentions, it's been
generally deemed "acceptable". There have been some recent attempts to
quantify it, but it's not been very clear:
https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)
[2] Casey's work to generalize the LSM interfaces continues and it quite
complex:
https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
[3] E.g. HARDENED_USERCOPY uses it:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
and so does the heap memory auto-initialization:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
--
Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-24 22:29 UTC (permalink / raw)
To: Kees Cook
Cc: KP Singh, Alexei Starovoitov, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev, Casey Schaufler
In-Reply-To: <202002241136.C4F9F7DFF@keescook>
On 2/24/2020 1:41 PM, Kees Cook wrote:
> On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
>> On 2/24/2020 9:13 AM, KP Singh wrote:
>>> On 24-Feb 08:32, Casey Schaufler wrote:
>>>> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
>>>>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
>>>>>> If I'm understanding this correctly, there are two issues:
>>>>>>
>>>>>> 1- BPF needs to be run last due to fexit trampolines (?)
>>>>> no.
>>>>> The placement of nop call can be anywhere.
>>>>> BPF trampoline is automagically converting nop call into a sequence
>>>>> of directly invoked BPF programs.
>>>>> No link list traversals and no indirect calls in run-time.
>>>> Then why the insistence that it be last?
>>> I think this came out of the discussion about not being able to
>>> override the other LSMs and introduce a new attack vector with some
>>> arguments discussed at:
>>>
>>> https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
>>>
>>> Let's say we have SELinux + BPF runnng on the system. BPF should still
>>> respect any decisions made by SELinux. This hasn't got anything to
>>> do with the usage of fexit trampolines.
>> The discussion sited is more about GPL than anything else.
>>
>> The LSM rule is that any security module must be able to
>> accept the decisions of others. SELinux has to accept decisions
>> made ahead of it. It always has, as LSM checks occur after
>> "traditional" checks, which may fail. The only reason that you
>> need to be last in this implementation appears to be that you
>> refuse to use the general mechanisms. You can't blame SELinux
>> for that.
> Okay, this is why I wanted to try to state things plainly. The "in last
> position" appears to be the result of a couple design choices:
>
> -the idea of "not wanting to get in the way of other LSMs", while
> admirable, needs to actually be a non-goal to be "just" a stacked LSM
> (as you're saying here Casey). This position _was_ required for the
> non-privileged LSM case to avoid security implications, but that goal
> not longer exists here either.
Thanks.
> -optimally using the zero-cost call-outs (static key + fexit trampolines)
> meant it didn't interact well with the existing stacking mechanism.
Exactly.
> So, fine, these appear to be design choices, not *specifically*
> requirements. Let's move on, I think there is more to unpack here...
Right.
>>>>>> 2- BPF hooks don't know what may be attached at any given time, so
>>>>>> ALL LSM hooks need to be universally hooked. THIS turns out to create
>>>>>> a measurable performance problem in that the cost of the indirect call
>>>>>> on the (mostly/usually) empty BPF policy is too high.
>>>>> also no.
> AIUI, there was some confusion on Alexei's reply here. I, perhaps,
> was not as clear as I needed to be. I think the later discussion on
> performance overheads gets more into the point, and gets us closer to
> the objections Alexei had. More below...
Agreed.
>>> This approach still had the issues of an indirect call and an
>>> extra check when not used. So this was not truly zero overhead even
>>> after special casing BPF.
>> The LSM mechanism is not zero overhead. It never has been. That's why
>> you can compile it out. You get added value at a price. You get the
>> ability to use SELinux and KRSI together at a price. If that's unacceptable
>> you can go the route of seccomp, which doesn't use LSM for many of the
>> same reasons you're on about.
>> [...]
>>>>>> So, trying to avoid the indirect calls is, as you say, an optimization,
>>>>>> but it might be a needed one due to the other limitations.
>>>>> I'm convinced that avoiding the cost of retpoline in critical path is a
>>>>> requirement for any new infrastructure in the kernel.
>>>> Sorry, I haven't gotten that memo.
> I agree with Casey here -- it's a nice goal, but those cost evaluations have
> not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
> sure, but this does appear to be an early optimization.
Thanks for helping clarify that.
>> [...]
>> It can do that wholly within KRSI hooks. You don't need to
>> put KRSI specific code into security.c.
> This observation is where I keep coming back to.
>
> Yes, the resulting code is not as fast as it could be. The fact that BPF
> triggers the worst-case performance of LSM hooking is the "new" part
> here, from what I can see.
I haven't put this oar in the water before, but mightn't it be
possible to configure which LSM hooks can have BPF programs installed
at compile time, and thus address this issue for the "production" case?
I fully expect that such a configuration option would be hideously ugly
both to implement and use. If the impact of unused BPF hooks is so
great a concern, perhaps it would be worthwhile.
> I suspect the problem is that folks in the BPF subsystem don't want to
> be seen as slowing anything down, even other subsystems, so they don't
> want to see this done in the traditional LSM hooking way (which contains
> indirect calls).
I get that, and it is a laudable goal, but ...
> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].
... like Kees says, this isn't the only use case we have to deal with.
>>>>> Networking stack converted all such places to conditional calls.
>>>>> In BPF land we converted indirect calls to direct jumps and direct calls.
>>>>> It took two years to do so. Adding new indirect calls is not an option.
>>>>> I'm eagerly waiting for Peter's static_call patches to land to convert
>>>>> a lot more indirect calls. May be existing LSMs will take advantage
>>>>> of static_call patches too, but static_call is not an option for BPF.
>>>>> That's why we introduced BPF trampoline in the last kernel release.
>>>> Sorry, but I don't see how BPF is so overwhelmingly special.
>>> My analogy here is that if every tracepoint in the kernel were of the
>>> type:
>>>
>>> if (trace_foo_enabled) // <-- Overhead here, solved with static key
>>> trace_foo(a); // <-- retpoline overhead, solved with fexit trampolines
> This is a helpful distillation; thanks.
>
> static keys (perhaps better described as static branches) make sense to
> me; I'm familiar with them being used all over the place[3]. The resulting
> "zero performance" branch mechanism is extremely handy.
>
> I had been thinking about the fexit stuff only as a way for BPF to call
> into kernel functions directly, and I missed the place where this got
> used for calling from the kernel into BPF directly. KP walked me through
> the fexit stuff off list. I missed where there NOP stub ("noinline int
> bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
> https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
> The key bit being "bpf_trampoline_update(prog)"
>
>>> It would be very hard to justify enabling them on a production system,
>>> and the same can be said for BPF and KRSI.
>> The same can be and has been said about the LSM infrastructure.
>> If BPF and KRSI are that performance critical you shouldn't be
>> tying them to LSM, which is known to have overhead. If you can't
>> accept the LSM overhead, get out of the LSM. Or, help us fix the
>> LSM infrastructure to make its overhead closer to zero. Whether
>> you believe it or not, a lot of work has gone into keeping the LSM
>> overhead as small as possible while remaining sufficiently general
>> to perform its function.
>>
>> No. If you're too special to play by LSM rules then you're special
>> enough to get into the kernel using more direct means.
> So, I see the primary conflict here being about the performance
> optimizations. AIUI:
>
> - BPF subsystem maintainers do not want any new slowdown associated
> with BPF
Right.
> - LSM subsystem maintainers do not want any new special cases in
> LSM stacking
Right.
> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
>
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs
>
> Does this seem right to everyone?
This would be my first choice. The existing list-of-hooks mechanism is
an "obvious" implementation. I have been thinking about possible ways to
make it better, but as y'all may have guessed, I haven't seen all the cool
new optimization techniques.
> -Kees
>
>
> [1] There is a "known cost to LSM", but as Casey mentions, it's been
> generally deemed "acceptable". There have been some recent attempts to
> quantify it, but it's not been very clear:
> https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)
>
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>
> [3] E.g. HARDENED_USERCOPY uses it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
> and so does the heap memory auto-initialization:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
>
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Alexei Starovoitov @ 2020-02-25 5:41 UTC (permalink / raw)
To: Kees Cook
Cc: Casey Schaufler, KP Singh, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev
In-Reply-To: <202002241136.C4F9F7DFF@keescook>
On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
>
> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
I think the key mistake we made is that we classified KRSI as LSM.
LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
I don't see anything in LSM infra that KRSI can reuse.
The only thing BPF needs is a function to attach to.
It can be a nop function or any other.
security_*() functions are interesting from that angle only.
Hence I propose to reconsider what I was suggesting earlier.
No changes to secruity/ directory.
Attach to security_*() funcs via bpf trampoline.
The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
I think "security" is also loaded word that should be avoided.
I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
>
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs
I'm very much surprised how 'slow' KRSI is an option at all.
'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
functions for every place in the kernel that calls security_*().
This is not an acceptable overhead. Even w/o retpoline
this is not something datacenter servers can use.
Another option is to do this:
diff --git a/include/linux/security.h b/include/linux/security.h
index 64b19f050343..7887ce636fb1 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
return kernel_load_data_str[id];
}
-#ifdef CONFIG_SECURITY
+#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
Single line change to security.h and new file kernel/bpf/override_security.c
that will look like:
int security_binder_set_context_mgr(struct task_struct *mgr)
{
return 0;
}
int security_binder_transaction(struct task_struct *from,
struct task_struct *to)
{
return 0;
}
Essentially it will provide BPF side with a set of nop functions.
CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
on kernel users. Either they build the kernel with CONFIG_SECURITY and their
choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
it's a pro not a con.
^ permalink raw reply related
* Re: [PATCH bpf-next v4 6/8] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: Andrii Nakryiko @ 2020-02-25 6:45 UTC (permalink / raw)
To: KP Singh
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Florent Revest,
Brendan Jackman, Martin KaFai Lau, Song Liu, Yonghong Song,
Serge E. Hallyn, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <20200220175250.10795-7-kpsingh@chromium.org>
On Thu, Feb 20, 2020 at 9:53 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> Since BPF_PROG_TYPE_LSM uses the same attaching mechanism as
> BPF_PROG_TYPE_TRACING, the common logic is refactored into a static
> function bpf_program__attach_btf.
>
> A new API call bpf_program__attach_lsm is still added to avoid userspace
> conflicts if this ever changes in the future.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
> tools/lib/bpf/bpf.c | 3 ++-
> tools/lib/bpf/libbpf.c | 46 ++++++++++++++++++++++++++++++++--------
> tools/lib/bpf/libbpf.h | 4 ++++
> tools/lib/bpf/libbpf.map | 3 +++
> 4 files changed, 46 insertions(+), 10 deletions(-)
>
> diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> index c6dafe563176..73220176728d 100644
> --- a/tools/lib/bpf/bpf.c
> +++ b/tools/lib/bpf/bpf.c
> @@ -235,7 +235,8 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> memset(&attr, 0, sizeof(attr));
> attr.prog_type = load_attr->prog_type;
> attr.expected_attach_type = load_attr->expected_attach_type;
> - if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> + if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS ||
> + attr.prog_type == BPF_PROG_TYPE_LSM) {
> attr.attach_btf_id = load_attr->attach_btf_id;
> } else if (attr.prog_type == BPF_PROG_TYPE_TRACING ||
> attr.prog_type == BPF_PROG_TYPE_EXT) {
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 514b1a524abb..d11139d5e76b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2351,16 +2351,14 @@ static int bpf_object__finalize_btf(struct bpf_object *obj)
>
> static inline bool libbpf_prog_needs_vmlinux_btf(struct bpf_program *prog)
> {
> - if (prog->type == BPF_PROG_TYPE_STRUCT_OPS)
> + if (prog->type == BPF_PROG_TYPE_STRUCT_OPS ||
> + prog->type == BPF_PROG_TYPE_LSM)
> return true;
>
> /* BPF_PROG_TYPE_TRACING programs which do not attach to other programs
> * also need vmlinux BTF
> */
> - if (prog->type == BPF_PROG_TYPE_TRACING && !prog->attach_prog_fd)
> - return true;
> -
> - return false;
> + return prog->type == BPF_PROG_TYPE_TRACING && !prog->attach_prog_fd;
please keep this as is, it allows to add more logic easily, if necessary
> }
>
> static int bpf_object__load_vmlinux_btf(struct bpf_object *obj)
> @@ -4855,7 +4853,8 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> load_attr.insns = insns;
> load_attr.insns_cnt = insns_cnt;
> load_attr.license = license;
[...]
> -struct bpf_link *bpf_program__attach_trace(struct bpf_program *prog)
> +/* Common logic for all BPF program types that attach to a btf_id */
> +static struct bpf_link *bpf_program__attach_btf(struct bpf_program *prog)
> {
> char errmsg[STRERR_BUFSIZE];
> struct bpf_link_fd *link;
> @@ -7376,7 +7388,7 @@ struct bpf_link *bpf_program__attach_trace(struct bpf_program *prog)
> if (pfd < 0) {
> pfd = -errno;
> free(link);
> - pr_warn("program '%s': failed to attach to trace: %s\n",
> + pr_warn("program '%s': failed to attach to: %s\n",
to attach to ... what?.. %s at the end is just an error message
> bpf_program__title(prog, false),
> libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> return ERR_PTR(pfd);
> @@ -7385,10 +7397,26 @@ struct bpf_link *bpf_program__attach_trace(struct bpf_program *prog)
> return (struct bpf_link *)link;
> }
>
[...]
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -227,10 +227,13 @@ LIBBPF_0.0.7 {
> bpf_probe_large_insn_limit;
> bpf_prog_attach_xattr;
> bpf_program__attach;
> + bpf_program__attach_lsm;
> bpf_program__name;
> bpf_program__is_extension;
> + bpf_program__is_lsm;
> bpf_program__is_struct_ops;
> bpf_program__set_extension;
> + bpf_program__set_lsm;
please make sure to add to 0.0.8 version for new revision
> bpf_program__set_struct_ops;
> btf__align_of;
> libbpf_find_kernel_btf;
> --
> 2.20.1
>
^ permalink raw reply
* BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: syzbot @ 2020-02-25 8:19 UTC (permalink / raw)
To: cpaasch, davem, dcaratti, fw, kuba, kuznet, linux-kernel,
linux-security-module, matthieu.baerts, netdev, pabeni, paul,
peter.krystad, syzkaller-bugs, yoshfuji
Hello,
syzbot found the following crash on:
HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
The bug was bisected to:
commit 2303f994b3e187091fd08148066688b08f837efc
Author: Peter Krystad <peter.krystad@linux.intel.com>
Date: Wed Jan 22 00:56:17 2020 +0000
mptcp: Associate MPTCP context with TCP socket
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
BUG: kernel NULL pointer dereference, address: 0000000000000000
#PF: supervisor instruction fetch in kernel mode
#PF: error_code(0x0010) - not-present page
PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
Oops: 0010 [#1] PREEMPT SMP KASAN
CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:0x0
Code: Bad RIP value.
RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
smack_netlabel security/smack/smack_lsm.c:2425 [inline]
smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
security_inode_setsecurity+0xb2/0x140 security/security.c:1364
__vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
vfs_setxattr fs/xattr.c:224 [inline]
setxattr+0x335/0x430 fs/xattr.c:451
__do_sys_fsetxattr fs/xattr.c:506 [inline]
__se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
__x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x440199
Code: 18 89 d0 c3 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 fb 13 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007ffcadc19e48 EFLAGS: 00000246 ORIG_RAX: 00000000000000be
RAX: ffffffffffffffda RBX: 00000000004002c8 RCX: 0000000000440199
RDX: 0000000020000200 RSI: 00000000200001c0 RDI: 0000000000000003
RBP: 00000000006ca018 R08: 0000000000000003 R09: 00000000004002c8
R10: 0000000000000009 R11: 0000000000000246 R12: 0000000000401a20
R13: 0000000000401ab0 R14: 0000000000000000 R15: 0000000000000000
Modules linked in:
CR2: 0000000000000000
---[ end trace 9bbc7bb8e1061a42 ]---
RIP: 0010:0x0
Code: Bad RIP value.
RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH v7 00/12] Introduce CAP_PERFMON to secure system performance monitoring and observability
From: Alexey Budankov @ 2020-02-25 9:55 UTC (permalink / raw)
To: James Morris, Serge Hallyn, Stephen Smalley, Peter Zijlstra,
Arnaldo Carvalho de Melo, Ingo Molnar,
joonas.lahtinen@linux.intel.com, Alexei Starovoitov, Will Deacon,
Paul Mackerras, Helge Deller, Thomas Gleixner
Cc: Andi Kleen, Stephane Eranian, Igor Lubashev, Jiri Olsa,
linux-kernel, intel-gfx@lists.freedesktop.org,
linux-security-module@vger.kernel.org, selinux@vger.kernel.org,
linux-arm-kernel, linuxppc-dev@lists.ozlabs.org,
linux-parisc@vger.kernel.org, oprofile-list,
linux-doc@vger.kernel.org, linux-man
In-Reply-To: <c8de937a-0b3a-7147-f5ef-69f467e87a13@linux.intel.com>
Hi,
Is there anything else I could do in order to move the changes forward
or is something still missing from this patch set?
Could you please share you mind?
Thanks,
Alexey
On 17.02.2020 11:02, Alexey Budankov wrote:
>
> Currently access to perf_events, i915_perf and other performance
> monitoring and observability subsystems of the kernel is open only for
> a privileged process [1] with CAP_SYS_ADMIN capability enabled in the
> process effective set [2].
>
> This patch set introduces CAP_PERFMON capability designed to secure
> system performance monitoring and observability operations so that
> CAP_PERFMON would assist CAP_SYS_ADMIN capability in its governing role
> for performance monitoring and observability subsystems of the kernel.
>
> CAP_PERFMON intends to harden system security and integrity during
> performance monitoring and observability operations by decreasing attack
> surface that is available to a CAP_SYS_ADMIN privileged process [2].
> Providing the access to performance monitoring and observability
> operations under CAP_PERFMON capability singly, without the rest of
> CAP_SYS_ADMIN credentials, excludes chances to misuse the credentials
> and makes the operation more secure. Thus, CAP_PERFMON implements the
> principal of least privilege for performance monitoring and
> observability operations (POSIX IEEE 1003.1e: 2.2.2.39 principle of
> least privilege: A security design principle that states that a process
> or program be granted only those privileges (e.g., capabilities)
> necessary to accomplish its legitimate function, and only for the time
> that such privileges are actually required)
>
> CAP_PERFMON intends to meet the demand to secure system performance
> monitoring and observability operations for adoption in security
> sensitive, restricted, multiuser production environments (e.g. HPC
> clusters, cloud and virtual compute environments), where root or
> CAP_SYS_ADMIN credentials are not available to mass users of a system,
> and securely unblock accessibility of system performance monitoring and
> observability operations beyond root and CAP_SYS_ADMIN use cases.
>
> CAP_PERFMON intends to take over CAP_SYS_ADMIN credentials related to
> system performance monitoring and observability operations and balance
> amount of CAP_SYS_ADMIN credentials following the recommendations in
> the capabilities man page [2] for CAP_SYS_ADMIN: "Note: this capability
> is overloaded; see Notes to kernel developers, below." For backward
> compatibility reasons access to system performance monitoring and
> observability subsystems of the kernel remains open for CAP_SYS_ADMIN
> privileged processes but CAP_SYS_ADMIN capability usage for secure
> system performance monitoring and observability operations is
> discouraged with respect to the designed CAP_PERFMON capability.
>
> Possible alternative solution to this system security hardening,
> capabilities balancing task of making performance monitoring and
> observability operations more secure and accessible could be to use
> the existing CAP_SYS_PTRACE capability to govern system performance
> monitoring and observability subsystems. However CAP_SYS_PTRACE
> capability still provides users with more credentials than are
> required for secure performance monitoring and observability
> operations and this excess is avoided by the designed CAP_PERFMON.
>
> Although software running under CAP_PERFMON can not ensure avoidance of
> related hardware issues, the software can still mitigate those issues
> following the official hardware issues mitigation procedure [3]. The
> bugs in the software itself can be fixed following the standard kernel
> development process [4] to maintain and harden security of system
> performance monitoring and observability operations. Finally, the patch
> set is shaped in the way that simplifies backtracking procedure of
> possible induced issues [5] as much as possible.
>
> The patch set is for tip perf/core repository:
> git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip perf/core
> sha1: fdb64822443ec9fb8c3a74b598a74790ae8d2e22
>
> ---
> Changes in v7:
> - updated and extended kernel.rst and perf-security.rst documentation
> files with the information about CAP_PERFMON capability and its use cases
> - documented the case of double audit logging of CAP_PERFMON and CAP_SYS_ADMIN
> capabilities on a SELinux enabled system
> Changes in v6:
> - avoided noaudit checks in perfmon_capable() to explicitly advertise
> CAP_PERFMON usage thru audit logs to secure system performance
> monitoring and observability
> Changes in v5:
> - renamed CAP_SYS_PERFMON to CAP_PERFMON
> - extended perfmon_capable() with noaudit checks
> Changes in v4:
> - converted perfmon_capable() into an inline function
> - made perf_events kprobes, uprobes, hw breakpoints and namespaces data
> available to CAP_SYS_PERFMON privileged processes
> - applied perfmon_capable() to drivers/perf and drivers/oprofile
> - extended __cmd_ftrace() with support of CAP_SYS_PERFMON
> Changes in v3:
> - implemented perfmon_capable() macros aggregating required capabilities
> checks
> Changes in v2:
> - made perf_events trace points available to CAP_SYS_PERFMON privileged
> processes
> - made perf_event_paranoid_check() treat CAP_SYS_PERFMON equally to
> CAP_SYS_ADMIN
> - applied CAP_SYS_PERFMON to i915_perf, bpf_trace, powerpc and parisc
> system performance monitoring and observability related subsystems
>
> ---
> Alexey Budankov (12):
> capabilities: introduce CAP_PERFMON to kernel and user space
> perf/core: open access to the core for CAP_PERFMON privileged process
> perf/core: open access to probes for CAP_PERFMON privileged process
> perf tool: extend Perf tool with CAP_PERFMON capability support
> drm/i915/perf: open access for CAP_PERFMON privileged process
> trace/bpf_trace: open access for CAP_PERFMON privileged process
> powerpc/perf: open access for CAP_PERFMON privileged process
> parisc/perf: open access for CAP_PERFMON privileged process
> drivers/perf: open access for CAP_PERFMON privileged process
> drivers/oprofile: open access for CAP_PERFMON privileged process
> doc/admin-guide: update perf-security.rst with CAP_PERFMON information
> doc/admin-guide: update kernel.rst with CAP_PERFMON information
>
> Documentation/admin-guide/perf-security.rst | 65 +++++++++++++--------
> Documentation/admin-guide/sysctl/kernel.rst | 16 +++--
> arch/parisc/kernel/perf.c | 2 +-
> arch/powerpc/perf/imc-pmu.c | 4 +-
> drivers/gpu/drm/i915/i915_perf.c | 13 ++---
> drivers/oprofile/event_buffer.c | 2 +-
> drivers/perf/arm_spe_pmu.c | 4 +-
> include/linux/capability.h | 4 ++
> include/linux/perf_event.h | 6 +-
> include/uapi/linux/capability.h | 8 ++-
> kernel/events/core.c | 6 +-
> kernel/trace/bpf_trace.c | 2 +-
> security/selinux/include/classmap.h | 4 +-
> tools/perf/builtin-ftrace.c | 5 +-
> tools/perf/design.txt | 3 +-
> tools/perf/util/cap.h | 4 ++
> tools/perf/util/evsel.c | 10 ++--
> tools/perf/util/util.c | 1 +
> 18 files changed, 98 insertions(+), 61 deletions(-)
>
> ---
> Validation (Intel Skylake, 8 cores, Fedora 29, 5.5.0-rc3+, x86_64):
>
> libcap library [6], [7], [8] and Perf tool can be used to apply
> CAP_PERFMON capability for secure system performance monitoring and
> observability beyond the scope permitted by the system wide
> perf_event_paranoid kernel setting [9] and below are the steps for
> evaluation:
>
> - patch, build and boot the kernel
> - patch, build Perf tool e.g. to /home/user/perf
> ...
> # git clone git://git.kernel.org/pub/scm/libs/libcap/libcap.git libcap
> # pushd libcap
> # patch libcap/include/uapi/linux/capabilities.h with [PATCH 1]
> # make
> # pushd progs
> # ./setcap "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> # ./setcap -v "cap_perfmon,cap_sys_ptrace,cap_syslog=ep" /home/user/perf
> /home/user/perf: OK
> # ./getcap /home/user/perf
> /home/user/perf = cap_sys_ptrace,cap_syslog,cap_perfmon+ep
> # echo 2 > /proc/sys/kernel/perf_event_paranoid
> # cat /proc/sys/kernel/perf_event_paranoid
> 2
> ...
> $ /home/user/perf top
> ... works as expected ...
> $ cat /proc/`pidof perf`/status
> Name: perf
> Umask: 0002
> State: S (sleeping)
> Tgid: 2958
> Ngid: 0
> Pid: 2958
> PPid: 9847
> TracerPid: 0
> Uid: 500 500 500 500
> Gid: 500 500 500 500
> FDSize: 256
> ...
> CapInh: 0000000000000000
> CapPrm: 0000004400080000
> CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> cap_perfmon,cap_sys_ptrace,cap_syslog
> CapBnd: 0000007fffffffff
> CapAmb: 0000000000000000
> NoNewPrivs: 0
> Seccomp: 0
> Speculation_Store_Bypass: thread vulnerable
> Cpus_allowed: ff
> Cpus_allowed_list: 0-7
> ...
>
> Usage of cap_perfmon effectively avoids unused credentials excess:
>
> - with cap_sys_admin:
> CapEff: 0000007fffffffff => 01111111 11111111 11111111 11111111 11111111
>
> - with cap_perfmon:
> CapEff: 0000004400080000 => 01000100 00000000 00001000 00000000 00000000
> 38 34 19
> perfmon syslog sys_ptrace
>
> ---
> [1] https://www.kernel.org/doc/html/latest/admin-guide/perf-security.html
> [2] http://man7.org/linux/man-pages/man7/capabilities.7.html
> [3] https://www.kernel.org/doc/html/latest/process/embargoed-hardware-issues.html
> [4] https://www.kernel.org/doc/html/latest/admin-guide/security-bugs.html
> [5] https://www.kernel.org/doc/html/latest/process/management-style.html#decisions
> [6] http://man7.org/linux/man-pages/man8/setcap.8.html
> [7] https://git.kernel.org/pub/scm/libs/libcap/libcap.git
> [8] https://sites.google.com/site/fullycapable/, posix_1003.1e-990310.pdf
> [9] http://man7.org/linux/man-pages/man2/perf_event_open.2.html
>
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Paul Moore @ 2020-02-25 14:20 UTC (permalink / raw)
To: syzbot
Cc: cpaasch, davem, dcaratti, fw, kuba, kuznet, linux-kernel,
linux-security-module, matthieu.baerts, netdev, pabeni,
peter.krystad, syzkaller-bugs, yoshfuji
In-Reply-To: <000000000000a719a9059f62246e@google.com>
On Tue, Feb 25, 2020 at 3:19 AM syzbot
<syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
>
> The bug was bisected to:
>
> commit 2303f994b3e187091fd08148066688b08f837efc
> Author: Peter Krystad <peter.krystad@linux.intel.com>
> Date: Wed Jan 22 00:56:17 2020 +0000
>
> mptcp: Associate MPTCP context with TCP socket
>
> bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
>
> BUG: kernel NULL pointer dereference, address: 0000000000000000
> #PF: supervisor instruction fetch in kernel mode
> #PF: error_code(0x0010) - not-present page
> PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> Oops: 0010 [#1] PREEMPT SMP KASAN
> CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> RIP: 0010:0x0
> Code: Bad RIP value.
> RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> Call Trace:
> cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> vfs_setxattr fs/xattr.c:224 [inline]
> setxattr+0x335/0x430 fs/xattr.c:451
> __do_sys_fsetxattr fs/xattr.c:506 [inline]
> __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
Netdev folks, I'm not very familiar with the multipath TCP code so I
was wondering if you might help me out a bit with this report. Based
on the stack trace above it looks like for a given AF_INET sock "sk",
inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
possible under normal conditions or is there an issue somewhere?
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Dmitry Vyukov @ 2020-02-25 14:27 UTC (permalink / raw)
To: Paul Moore, Paolo Abeni
Cc: syzbot, cpaasch, David Miller, Davide Caratti, Florian Westphal,
kuba, Alexey Kuznetsov, LKML, linux-security-module,
matthieu.baerts, netdev, peter.krystad, syzkaller-bugs,
Hideaki YOSHIFUJI
In-Reply-To: <CAHC9VhTh6s1m7YBZp-3XO3q2EcjtMKUTcXwRzDTj_LSJd+cHTA@mail.gmail.com>
On Tue, Feb 25, 2020 at 3:20 PM Paul Moore <paul@paul-moore.com> wrote:
>
> On Tue, Feb 25, 2020 at 3:19 AM syzbot
> <syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com> wrote:
> >
> > Hello,
> >
> > syzbot found the following crash on:
> >
> > HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> > git tree: upstream
> > console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> > dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
> >
> > The bug was bisected to:
> >
> > commit 2303f994b3e187091fd08148066688b08f837efc
> > Author: Peter Krystad <peter.krystad@linux.intel.com>
> > Date: Wed Jan 22 00:56:17 2020 +0000
> >
> > mptcp: Associate MPTCP context with TCP socket
> >
> > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> > final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> > console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> > Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
> >
> > BUG: kernel NULL pointer dereference, address: 0000000000000000
> > #PF: supervisor instruction fetch in kernel mode
> > #PF: error_code(0x0010) - not-present page
> > PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> > Oops: 0010 [#1] PREEMPT SMP KASAN
> > CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > RIP: 0010:0x0
> > Code: Bad RIP value.
> > RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> > RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> > RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> > RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> > R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> > R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> > FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Call Trace:
> > cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> > netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> > smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> > smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> > security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> > __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> > vfs_setxattr fs/xattr.c:224 [inline]
> > setxattr+0x335/0x430 fs/xattr.c:451
> > __do_sys_fsetxattr fs/xattr.c:506 [inline]
> > __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> > __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> > do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> > entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Netdev folks, I'm not very familiar with the multipath TCP code so I
> was wondering if you might help me out a bit with this report. Based
> on the stack trace above it looks like for a given AF_INET sock "sk",
> inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
> possible under normal conditions or is there an issue somewhere?
Paolo has submitted some patch for testing for this bug, not sure if
you have seen it, just in case:
https://groups.google.com/forum/#!msg/syzkaller-bugs/dqwnTBh-MQw/LhgSZYGsBgAJ
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Paolo Abeni @ 2020-02-25 14:46 UTC (permalink / raw)
To: Dmitry Vyukov, Paul Moore
Cc: syzbot, cpaasch, David Miller, Davide Caratti, Florian Westphal,
kuba, Alexey Kuznetsov, LKML, linux-security-module,
matthieu.baerts, netdev, peter.krystad, syzkaller-bugs,
Hideaki YOSHIFUJI
In-Reply-To: <CACT4Y+Y3QN9=c5JvJkecCtdQGTxHYRXMhS4f1itwU5JEZmcYtA@mail.gmail.com>
On Tue, 2020-02-25 at 15:27 +0100, Dmitry Vyukov wrote:
> On Tue, Feb 25, 2020 at 3:20 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Feb 25, 2020 at 3:19 AM syzbot
> > <syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com> wrote:
> > > Hello,
> > >
> > > syzbot found the following crash on:
> > >
> > > HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> > > git tree: upstream
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> > > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
> > >
> > > The bug was bisected to:
> > >
> > > commit 2303f994b3e187091fd08148066688b08f837efc
> > > Author: Peter Krystad <peter.krystad@linux.intel.com>
> > > Date: Wed Jan 22 00:56:17 2020 +0000
> > >
> > > mptcp: Associate MPTCP context with TCP socket
> > >
> > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> > > final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> > > console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
> > >
> > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> > > Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
> > >
> > > BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > #PF: supervisor instruction fetch in kernel mode
> > > #PF: error_code(0x0010) - not-present page
> > > PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> > > Oops: 0010 [#1] PREEMPT SMP KASAN
> > > CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > RIP: 0010:0x0
> > > Code: Bad RIP value.
> > > RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> > > RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> > > RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> > > RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> > > R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> > > R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> > > FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > Call Trace:
> > > cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> > > netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> > > smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> > > smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> > > security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> > > __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> > > vfs_setxattr fs/xattr.c:224 [inline]
> > > setxattr+0x335/0x430 fs/xattr.c:451
> > > __do_sys_fsetxattr fs/xattr.c:506 [inline]
> > > __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> > > __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> > > do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> > > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >
> > Netdev folks, I'm not very familiar with the multipath TCP code so I
> > was wondering if you might help me out a bit with this report. Based
> > on the stack trace above it looks like for a given AF_INET sock "sk",
> > inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
> > possible under normal conditions or is there an issue somewhere?
>
> Paolo has submitted some patch for testing for this bug, not sure if
> you have seen it, just in case:
> https://groups.google.com/forum/#!msg/syzkaller-bugs/dqwnTBh-MQw/LhgSZYGsBgAJ
I sent the patch to the syzbot ML only, for testing before posting on
netdev, so Paul likely have not seen it yet, sorry.
@Dmitry: I did not get any reply yet from syzbot, are there any
problems or is this the usual time-frame?
Thank you!
Paolo
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Dmitry Vyukov @ 2020-02-25 15:09 UTC (permalink / raw)
To: Paolo Abeni
Cc: Paul Moore, syzbot, cpaasch, David Miller, Davide Caratti,
Florian Westphal, kuba, Alexey Kuznetsov, LKML,
linux-security-module, matthieu.baerts, netdev, peter.krystad,
syzkaller-bugs, Hideaki YOSHIFUJI
In-Reply-To: <db643ed6efb6fa04fe5753af68d13f1b2ffcf821.camel@redhat.com>
On Tue, Feb 25, 2020 at 3:46 PM Paolo Abeni <pabeni@redhat.com> wrote:
>
> On Tue, 2020-02-25 at 15:27 +0100, Dmitry Vyukov wrote:
> > On Tue, Feb 25, 2020 at 3:20 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Feb 25, 2020 at 3:19 AM syzbot
> > > <syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com> wrote:
> > > > Hello,
> > > >
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> > > > git tree: upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> > > > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
> > > >
> > > > The bug was bisected to:
> > > >
> > > > commit 2303f994b3e187091fd08148066688b08f837efc
> > > > Author: Peter Krystad <peter.krystad@linux.intel.com>
> > > > Date: Wed Jan 22 00:56:17 2020 +0000
> > > >
> > > > mptcp: Associate MPTCP context with TCP socket
> > > >
> > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> > > > final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
> > > >
> > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> > > > Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
> > > >
> > > > BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > #PF: supervisor instruction fetch in kernel mode
> > > > #PF: error_code(0x0010) - not-present page
> > > > PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> > > > Oops: 0010 [#1] PREEMPT SMP KASAN
> > > > CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > > RIP: 0010:0x0
> > > > Code: Bad RIP value.
> > > > RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> > > > RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> > > > RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> > > > RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> > > > R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> > > > R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> > > > FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > Call Trace:
> > > > cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> > > > netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> > > > smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> > > > smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> > > > security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> > > > __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> > > > vfs_setxattr fs/xattr.c:224 [inline]
> > > > setxattr+0x335/0x430 fs/xattr.c:451
> > > > __do_sys_fsetxattr fs/xattr.c:506 [inline]
> > > > __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> > > > __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> > > > do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> > > > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >
> > > Netdev folks, I'm not very familiar with the multipath TCP code so I
> > > was wondering if you might help me out a bit with this report. Based
> > > on the stack trace above it looks like for a given AF_INET sock "sk",
> > > inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
> > > possible under normal conditions or is there an issue somewhere?
> >
> > Paolo has submitted some patch for testing for this bug, not sure if
> > you have seen it, just in case:
> > https://groups.google.com/forum/#!msg/syzkaller-bugs/dqwnTBh-MQw/LhgSZYGsBgAJ
>
> I sent the patch to the syzbot ML only, for testing before posting on
> netdev, so Paul likely have not seen it yet, sorry.
>
> @Dmitry: I did not get any reply yet from syzbot, are there any
> problems or is this the usual time-frame?
The testing is queued after bisection of this guy:
https://syzkaller.appspot.com/bug?id=c0a75a31c5fa84e6e5d3131fd98a5b56e2141b9a
which has been running since 6pm yesterday...
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Paul Moore @ 2020-02-25 15:11 UTC (permalink / raw)
To: Paolo Abeni
Cc: Dmitry Vyukov, syzbot, cpaasch, David Miller, Davide Caratti,
Florian Westphal, kuba, Alexey Kuznetsov, LKML,
linux-security-module, matthieu.baerts, netdev, peter.krystad,
syzkaller-bugs, Hideaki YOSHIFUJI
In-Reply-To: <db643ed6efb6fa04fe5753af68d13f1b2ffcf821.camel@redhat.com>
On Tue, Feb 25, 2020 at 9:46 AM Paolo Abeni <pabeni@redhat.com> wrote:
> On Tue, 2020-02-25 at 15:27 +0100, Dmitry Vyukov wrote:
> > On Tue, Feb 25, 2020 at 3:20 PM Paul Moore <paul@paul-moore.com> wrote:
> > > On Tue, Feb 25, 2020 at 3:19 AM syzbot
> > > <syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com> wrote:
> > > > Hello,
> > > >
> > > > syzbot found the following crash on:
> > > >
> > > > HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> > > > git tree: upstream
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> > > > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
> > > >
> > > > The bug was bisected to:
> > > >
> > > > commit 2303f994b3e187091fd08148066688b08f837efc
> > > > Author: Peter Krystad <peter.krystad@linux.intel.com>
> > > > Date: Wed Jan 22 00:56:17 2020 +0000
> > > >
> > > > mptcp: Associate MPTCP context with TCP socket
> > > >
> > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> > > > final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
> > > >
> > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> > > > Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
> > > >
> > > > BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > #PF: supervisor instruction fetch in kernel mode
> > > > #PF: error_code(0x0010) - not-present page
> > > > PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> > > > Oops: 0010 [#1] PREEMPT SMP KASAN
> > > > CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > > RIP: 0010:0x0
> > > > Code: Bad RIP value.
> > > > RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> > > > RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> > > > RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> > > > RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> > > > R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> > > > R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> > > > FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > Call Trace:
> > > > cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> > > > netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> > > > smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> > > > smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> > > > security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> > > > __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> > > > vfs_setxattr fs/xattr.c:224 [inline]
> > > > setxattr+0x335/0x430 fs/xattr.c:451
> > > > __do_sys_fsetxattr fs/xattr.c:506 [inline]
> > > > __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> > > > __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> > > > do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> > > > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > >
> > > Netdev folks, I'm not very familiar with the multipath TCP code so I
> > > was wondering if you might help me out a bit with this report. Based
> > > on the stack trace above it looks like for a given AF_INET sock "sk",
> > > inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
> > > possible under normal conditions or is there an issue somewhere?
> >
> > Paolo has submitted some patch for testing for this bug, not sure if
> > you have seen it, just in case:
> > https://groups.google.com/forum/#!msg/syzkaller-bugs/dqwnTBh-MQw/LhgSZYGsBgAJ
>
> I sent the patch to the syzbot ML only, for testing before posting on
> netdev, so Paul likely have not seen it yet, sorry.
No problem, as long as someone is working on a fix I'm happy. Would
you mind CC'ing me on the patch once you post it? I'm curious as to
what the problem and solution turns out to be.
Thanks.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: BUG: unable to handle kernel NULL pointer dereference in cipso_v4_sock_setattr
From: Dmitry Vyukov @ 2020-02-25 15:12 UTC (permalink / raw)
To: Paolo Abeni
Cc: Paul Moore, syzbot, cpaasch, David Miller, Davide Caratti,
Florian Westphal, kuba, Alexey Kuznetsov, LKML,
linux-security-module, matthieu.baerts, netdev, peter.krystad,
syzkaller-bugs, Hideaki YOSHIFUJI
In-Reply-To: <CACT4Y+a6Lp=YGYrxDubwrXiMKc5R5WhehR+Q5E1jEu452d0tfA@mail.gmail.com>
On Tue, Feb 25, 2020 at 4:09 PM Dmitry Vyukov <dvyukov@google.com> wrote:
> > > > > Hello,
> > > > >
> > > > > syzbot found the following crash on:
> > > > >
> > > > > HEAD commit: ca7e1fd1 Merge tag 'linux-kselftest-5.6-rc3' of git://git...
> > > > > git tree: upstream
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=179f0931e00000
> > > > > kernel config: https://syzkaller.appspot.com/x/.config?x=a61f2164c515c07f
> > > > > dashboard link: https://syzkaller.appspot.com/bug?extid=f4dfece964792d80b139
> > > > > compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
> > > > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14fdfdede00000
> > > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17667de9e00000
> > > > >
> > > > > The bug was bisected to:
> > > > >
> > > > > commit 2303f994b3e187091fd08148066688b08f837efc
> > > > > Author: Peter Krystad <peter.krystad@linux.intel.com>
> > > > > Date: Wed Jan 22 00:56:17 2020 +0000
> > > > >
> > > > > mptcp: Associate MPTCP context with TCP socket
> > > > >
> > > > > bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=14fbec81e00000
> > > > > final crash: https://syzkaller.appspot.com/x/report.txt?x=16fbec81e00000
> > > > > console output: https://syzkaller.appspot.com/x/log.txt?x=12fbec81e00000
> > > > >
> > > > > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > > > > Reported-by: syzbot+f4dfece964792d80b139@syzkaller.appspotmail.com
> > > > > Fixes: 2303f994b3e1 ("mptcp: Associate MPTCP context with TCP socket")
> > > > >
> > > > > BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > > #PF: supervisor instruction fetch in kernel mode
> > > > > #PF: error_code(0x0010) - not-present page
> > > > > PGD 8e171067 P4D 8e171067 PUD 93fa2067 PMD 0
> > > > > Oops: 0010 [#1] PREEMPT SMP KASAN
> > > > > CPU: 0 PID: 8984 Comm: syz-executor066 Not tainted 5.6.0-rc2-syzkaller #0
> > > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> > > > > RIP: 0010:0x0
> > > > > Code: Bad RIP value.
> > > > > RSP: 0018:ffffc900020b7b80 EFLAGS: 00010246
> > > > > RAX: 1ffff110124ba600 RBX: 0000000000000000 RCX: ffff88809fefa600
> > > > > RDX: ffff8880994cdb18 RSI: 0000000000000000 RDI: ffff8880925d3140
> > > > > RBP: ffffc900020b7bd8 R08: ffffffff870225be R09: fffffbfff140652a
> > > > > R10: fffffbfff140652a R11: 0000000000000000 R12: ffff8880925d35d0
> > > > > R13: ffff8880925d3140 R14: dffffc0000000000 R15: 1ffff110124ba6ba
> > > > > FS: 0000000001a0b880(0000) GS:ffff8880aea00000(0000) knlGS:0000000000000000
> > > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > > CR2: ffffffffffffffd6 CR3: 00000000a6d6f000 CR4: 00000000001406f0
> > > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > > Call Trace:
> > > > > cipso_v4_sock_setattr+0x34b/0x470 net/ipv4/cipso_ipv4.c:1888
> > > > > netlbl_sock_setattr+0x2a7/0x310 net/netlabel/netlabel_kapi.c:989
> > > > > smack_netlabel security/smack/smack_lsm.c:2425 [inline]
> > > > > smack_inode_setsecurity+0x3da/0x4a0 security/smack/smack_lsm.c:2716
> > > > > security_inode_setsecurity+0xb2/0x140 security/security.c:1364
> > > > > __vfs_setxattr_noperm+0x16f/0x3e0 fs/xattr.c:197
> > > > > vfs_setxattr fs/xattr.c:224 [inline]
> > > > > setxattr+0x335/0x430 fs/xattr.c:451
> > > > > __do_sys_fsetxattr fs/xattr.c:506 [inline]
> > > > > __se_sys_fsetxattr+0x130/0x1b0 fs/xattr.c:495
> > > > > __x64_sys_fsetxattr+0xbf/0xd0 fs/xattr.c:495
> > > > > do_syscall_64+0xf7/0x1c0 arch/x86/entry/common.c:294
> > > > > entry_SYSCALL_64_after_hwframe+0x49/0xbe
> > > >
> > > > Netdev folks, I'm not very familiar with the multipath TCP code so I
> > > > was wondering if you might help me out a bit with this report. Based
> > > > on the stack trace above it looks like for a given AF_INET sock "sk",
> > > > inet_sk(sk)->is_icsk is true but inet_csk(sk) is NULL; should this be
> > > > possible under normal conditions or is there an issue somewhere?
> > >
> > > Paolo has submitted some patch for testing for this bug, not sure if
> > > you have seen it, just in case:
> > > https://groups.google.com/forum/#!msg/syzkaller-bugs/dqwnTBh-MQw/LhgSZYGsBgAJ
> >
> > I sent the patch to the syzbot ML only, for testing before posting on
> > netdev, so Paul likely have not seen it yet, sorry.
> >
> > @Dmitry: I did not get any reply yet from syzbot, are there any
> > problems or is this the usual time-frame?
>
> The testing is queued after bisection of this guy:
> https://syzkaller.appspot.com/bug?id=c0a75a31c5fa84e6e5d3131fd98a5b56e2141b9a
> which has been running since 6pm yesterday...
It's stuck in this boot-broken region. And git bisect degrades to
linear behavior on skipped commits (why?).
If only kernel would have proper presubmit testing to avoid at least
boot breakages... :)
[ 6.784292][ T24] kasan: GPF could be caused by NULL-ptr deref or
user memory access
[ 6.787429][ T24] general protection fault: 0000 [#1] PREEMPT SMP KASAN
[ 6.789312][ T24] CPU: 1 PID: 24 Comm: kworker/u4:2 Not tainted
5.3.0-rc1-syzkaller #0
[ 6.791623][ T24] Hardware name: Google Google Compute
Engine/Google Compute Engine, BIOS Google 01/01/2011
[ 6.791623][ T24] Workqueue: events_unbound async_run_entry_fn
[ 6.795328][ T24] RIP: 0010:dma_direct_max_mapping_size+0x5d/0x128
[ 6.795328][ T24] Code: df 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85
c2 00 00 00 4c 8b a3 38 03 00 00 48 b8 00 00 00 00 00 fc ff df 4c 89
e2 48 c1 ea 03 <80> 3c 02 00 0f 85 aa 00 00 00 48 8d bb 48 03 00 00 4d
8b 2c 24 48
[ 6.795328][ T24] RSP: 0000:ffff8880a9adf728 EFLAGS: 00010246
[ 6.806015][ T24] RAX: dffffc0000000000 RBX: ffff88821962b300
RCX: ffffffff8716d938
[ 6.806015][ T24] RDX: 0000000000000000 RSI: 0000000000000040
RDI: ffff88821962b638
[ 6.806015][ T24] RBP: ffff8880a9adf740 R08: ffffed101443d28d
R09: ffffed101443d28d
[ 6.806015][ T24] R10: ffffed101443d28c R11: ffff8880a21e9467
R12: 0000000000000000
[ 6.806015][ T24] R13: ffff88821962b300 R14: ffff8880a28a6a70
R15: 0000000000000200
[ 6.818295][ T24] FS: 0000000000000000(0000)
GS:ffff8880ae900000(0000) knlGS:0000000000000000
[ 6.818446][ T24] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6.818446][ T24] CR2: 0000000000000000 CR3: 0000000008a6d000
CR4: 00000000001406e0
[ 6.818446][ T24] DR0: 0000000000000000 DR1: 0000000000000000
DR2: 0000000000000000
[ 6.818446][ T24] DR3: 0000000000000000 DR6: 00000000fffe0ff0
DR7: 0000000000000400
[ 6.829410][ T24] Call Trace:
[ 6.829410][ T24] dma_max_mapping_size+0xa2/0xc0
[ 6.831705][ T24] __scsi_init_queue+0x197/0x4f0
[ 6.831705][ T24] scsi_mq_alloc_queue+0xb7/0x150
[ 6.831705][ T24] scsi_alloc_sdev+0x7b7/0xb80
[ 6.831705][ T24] scsi_probe_and_add_lun+0x86a/0x37c0
[ 6.831705][ T24] ? __kasan_check_read+0x11/0x20
[ 6.839613][ T24] ? scsi_alloc_sdev+0xb80/0xb80
[ 6.839613][ T24] ? mark_lock+0xc3/0x1190
[ 6.839613][ T24] ? mark_held_locks+0xb8/0x130
[ 6.839613][ T24] ? _raw_spin_unlock_irqrestore+0x82/0xd0
[ 6.839613][ T24] ? __pm_runtime_resume+0xb4/0x110
[ 6.839613][ T24] ? lockdep_hardirqs_on+0x424/0x5c0
[ 6.839613][ T24] ? _raw_spin_unlock_irqrestore+0x82/0xd0
[ 6.839613][ T24] ? trace_hardirqs_on+0x28/0x1a0
[ 6.851380][ T1] slram: not enough parameters.
[ 6.839613][ T24] ? _raw_spin_unlock_irqrestore+0x6d/0xd0
[ 6.839613][ T24] ? __pm_runtime_resume+0xb4/0x110
[ 6.839613][ T24] __scsi_scan_target+0x1fd/0xc90
[ 6.839613][ T24] ? scsi_add_device+0x30/0x30
[ 6.839613][ T24] ? mark_held_locks+0xb8/0x130
[ 6.839613][ T24] ? _raw_spin_unlock_irqrestore+0x82/0xd0
[ 6.839613][ T24] ? __pm_runtime_resume+0xb4/0x110
[ 6.839613][ T24] ? lockdep_hardirqs_on+0x424/0x5c0
[ 6.839613][ T24] ? _raw_spin_unlock_irqrestore+0x82/0xd0
[ 6.839613][ T24] ? trace_hardirqs_on+0x28/0x1a0
[ 6.839613][ T24] scsi_scan_channel.part.8+0xd6/0x140
[ 6.857364][ T1] ftl_cs: FTL header not found.
[ 6.854615][ T24] scsi_scan_host_selected+0x20c/0x300
[ 6.865470][ T1] Ethernet Channel Bonding Driver: v3.7.1 (April 27, 2011)
[ 6.854615][ T24] ? scsi_scan_host+0x3c0/0x3c0
[ 6.854615][ T24] do_scsi_scan_host+0x1b3/0x250
[ 6.854615][ T24] ? lock_downgrade+0x900/0x900
[ 6.854615][ T24] ? scsi_scan_host+0x3c0/0x3c0
[ 6.854615][ T24] do_scan_async+0x3c/0x450
[ 6.854615][ T24] ? scsi_scan_host+0x3c0/0x3c0
[ 6.854615][ T24] async_run_entry_fn+0xf9/0x4b0
[ 6.854615][ T24] process_one_work+0x856/0x1630
[ 6.854615][ T24] ? pwq_dec_nr_in_flight+0x2c0/0x2c0
[ 6.854615][ T24] ? lock_acquire+0x194/0x410
[ 6.854615][ T24] worker_thread+0x85/0xb60
[ 6.854615][ T24] ? __kthread_parkme+0x47/0x1a0
[ 6.854615][ T24] kthread+0x331/0x3f0
[ 6.854615][ T24] ? process_one_work+0x1630/0x1630
[ 6.854615][ T24] ? kthread_cancel_delayed_work_sync+0x10/0x10
[ 6.854615][ T24] ret_from_fork+0x24/0x30
[ 6.854615][ T24] Modules linked in:
[ 6.907103][ T24] ---[ end trace 14068796cd600dc6 ]---
[ 6.908603][ T24] RIP: 0010:dma_direct_max_mapping_size+0x5d/0x128
[ 6.909683][ T1] eql: Equalizer2002: Simon Janes (simon@ncm.com)
and David S. Miller (davem@redhat.com)
[ 6.910361][ T24] Code: df 48 89 fa 48 c1 ea 03 80 3c 02 00 0f 85
c2 00 00 00 4c 8b a3 38 03 00 00 48 b8 00 00 00 00 00 fc ff df 4c 89
e2 48 c1 ea 03 <80> 3c 02 00 0f 85 aa 00 00 00 48 8d bb 48 03 00 00 4d
8b 2c 24 48
[ 6.918535][ T24] RSP: 0000:ffff8880a9adf728 EFLAGS: 00010246
[ 6.919167][ T1] MACsec IEEE 802.1AE
[ 6.920089][ T24] RAX: dffffc0000000000 RBX: ffff88821962b300
RCX: ffffffff8716d938
[ 6.921513][ T1] tun: Universal TUN/TAP device driver, 1.6
[ 6.923553][ T24] RDX: 0000000000000000 RSI: 0000000000000040
RDI: ffff88821962b638
[ 6.932178][ T24] RBP: ffff8880a9adf740 R08: ffffed101443d28d
R09: ffffed101443d28d
[ 6.946172][ T24] R10: ffffed101443d28c R11: ffff8880a21e9467
R12: 0000000000000000
[ 6.952330][ T24] R13: ffff88821962b300 R14: ffff8880a28a6a70
R15: 0000000000000200
[ 6.956636][ T24] FS: 0000000000000000(0000)
GS:ffff8880ae900000(0000) knlGS:0000000000000000
[ 6.964469][ T24] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 6.968832][ T24] CR2: 0000000000000000 CR3: 0000000008a6d000
CR4: 00000000001406e0
[ 6.971517][ T24] DR0: 0000000000000000 DR1: 0000000000000000
DR2: 0000000000000000
[ 6.977961][ T24] DR3: 0000000000000000 DR6: 00000000fffe0ff0
DR7: 0000000000000400
[ 6.983019][ T24] Kernel panic - not syncing: Fatal exception
[ 6.983346][ T1] vcan: Virtual CAN interface driver
[ 6.987331][ T24] Kernel Offset: disabled
[ 6.988609][ T24] Rebooting in 86400 seconds..
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Kees Cook @ 2020-02-25 15:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Casey Schaufler, KP Singh, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev
In-Reply-To: <20200225054125.dttrc3fvllzu4mx5@ast-mbp>
On Mon, Feb 24, 2020 at 09:41:27PM -0800, Alexei Starovoitov wrote:
> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
Isn't the type used to decide which validator to use?
--
Kees Cook
^ permalink raw reply
* suspicious RCU usage from smack code
From: John Garry @ 2020-02-25 17:59 UTC (permalink / raw)
To: casey, jmorris@namei.org, serge@hallyn.com, linux-security-module
Cc: Anders Roxell
Hi guys,
JFYI, When I enable CONFIG_PROVE_RCU=y, I get these:
[ 0.369697] WARNING: suspicious RCU usage
[ 0.374179] 5.6.0-rc3-00002-g619882231229-dirty #1753 Not tainted
[ 0.380974] -----------------------------
[ 0.385455] security/smack/smack_lsm.c:354 RCU-list traversed in
non-reader section!!
[ 0.394183]
[ 0.394183] other info that might help us debug this:
[ 0.394183]
[ 0.403107]
[ 0.403107] rcu_scheduler_active = 1, debug_locks = 1
[ 0.410389] no locks held by kthreadd/2.
[ 0.414770]
[ 0.414770] stack backtrace:
[ 0.419636] CPU: 0 PID: 2 Comm: kthreadd Not tainted
5.6.0-rc3-00002-g619882231229-dirty #1753
[ 0.429204] Call trace:
[ 0.431924] dump_backtrace+0x0/0x298
[ 0.435990] show_stack+0x14/0x20
[ 0.439674] dump_stack+0x118/0x190
[ 0.443548] lockdep_rcu_suspicious+0xe0/0x120
[ 0.448487] smack_cred_prepare+0x2f8/0x310
[ 0.453134] security_prepare_creds+0x64/0xe0
[ 0.457979] prepare_creds+0x25c/0x368
[ 0.462141] copy_creds+0x40/0x620
[ 0.465918] copy_process+0x62c/0x25e0
[ 0.470084] _do_fork+0xc0/0x998
[ 0.473667] kernel_thread+0xa0/0xc8
[ 0.477640] kthreadd+0x2b0/0x408
[ 0.481325] ret_from_fork+0x10/0x18
[ 18.804382] =============================
[ 18.808872] WARNING: suspicious RCU usage
[ 18.813348] 5.6.0-rc3-00002-g619882231229-dirty #1753 Not tainted
[ 18.820145] -----------------------------
[ 18.824621] security/smack/smack_access.c:87 RCU-list traversed in
non-reader section!!
[ 18.833544]
[ 18.833544] other info that might help us debug this:
[ 18.833544]
[ 18.842465]
[ 18.842465] rcu_scheduler_active = 1, debug_locks = 1
[ 18.849741] no locks held by kdevtmpfs/781.
[ 18.854410]
[ 18.854410] stack backtrace:
[ 18.859277] CPU: 1 PID: 781 Comm: kdevtmpfs Not tainted
5.6.0-rc3-00002-g619882231229-dirty #1753
[ 18.869138] Call trace:
[ 18.871860] dump_backtrace+0x0/0x298
[ 18.875929] show_stack+0x14/0x20
[ 18.879612] dump_stack+0x118/0x190
[ 18.883489] lockdep_rcu_suspicious+0xe0/0x120
[ 18.888428] smk_access_entry+0x110/0x128
[ 18.892885] smk_tskacc+0x70/0xe8
[ 18.896568] smk_curacc+0x64/0x78
[ 18.900249] smack_inode_permission+0x110/0x1c8
[ 18.905284] security_inode_permission+0x50/0x98
[ 18.910412] inode_permission+0x70/0x1d0
[ 18.914768] link_path_walk.part.38+0x4a8/0x778
[ 18.919802] path_lookupat+0xd0/0x1a8
[ 18.923871] filename_lookup+0xf0/0x1f8
[ 18.928136] user_path_at_empty+0x48/0x58
[ 18.932590] ksys_chdir+0x8c/0x138
[ 18.936366] devtmpfsd+0x148/0x448
[ 18.940146] kthread+0x1c8/0x1d0
[ 18.943732] ret_from_fork+0x10/0x18
I haven't had a chance to check whether they are bogus or not.
Thanks,
John
^ permalink raw reply
* Re: [RFC PATCH v14 00/10] Landlock LSM
From: J Freyensee @ 2020-02-25 18:49 UTC (permalink / raw)
To: Mickaël Salaün, linux-kernel
Cc: Al Viro, Andy Lutomirski, Arnd Bergmann, Casey Schaufler,
Greg Kroah-Hartman, James Morris, Jann Horn, Jonathan Corbet,
Kees Cook, Michael Kerrisk, Mickaël Salaün,
Serge E . Hallyn, Shuah Khan, Vincent Dagonneau, kernel-hardening,
linux-api, linux-arch, linux-doc, linux-fsdevel, linux-kselftest,
linux-security-module, x86
In-Reply-To: <20200224160215.4136-1-mic@digikod.net>
On 2/24/20 8:02 AM, Mickaël Salaün wrote:
> ## Syscall
>
> Because it is only tested on x86_64, the syscall is only wired up for
> this architecture. The whole x86 family (and probably all the others)
> will be supported in the next patch series.
General question for u. What is it meant "whole x86 family will be
supported". 32-bit x86 will be supported?
Thanks,
Jay
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-25 19:29 UTC (permalink / raw)
To: Kees Cook
Cc: Casey Schaufler, Alexei Starovoitov, LKML,
Linux Security Module list, Alexei Starovoitov, James Morris, bpf,
netdev
In-Reply-To: <202002241136.C4F9F7DFF@keescook>
On 24-Feb 13:41, Kees Cook wrote:
> On Mon, Feb 24, 2020 at 10:45:27AM -0800, Casey Schaufler wrote:
> > On 2/24/2020 9:13 AM, KP Singh wrote:
> > > On 24-Feb 08:32, Casey Schaufler wrote:
> > >> On 2/23/2020 2:08 PM, Alexei Starovoitov wrote:
> > >>> On Fri, Feb 21, 2020 at 08:22:59PM -0800, Kees Cook wrote:
> > >>>> If I'm understanding this correctly, there are two issues:
> > >>>>
> > >>>> 1- BPF needs to be run last due to fexit trampolines (?)
> > >>> no.
> > >>> The placement of nop call can be anywhere.
> > >>> BPF trampoline is automagically converting nop call into a sequence
> > >>> of directly invoked BPF programs.
> > >>> No link list traversals and no indirect calls in run-time.
> > >> Then why the insistence that it be last?
> > > I think this came out of the discussion about not being able to
> > > override the other LSMs and introduce a new attack vector with some
> > > arguments discussed at:
> > >
> > > https://lore.kernel.org/bpf/20200109194302.GA85350@google.com/
> > >
> > > Let's say we have SELinux + BPF runnng on the system. BPF should still
> > > respect any decisions made by SELinux. This hasn't got anything to
> > > do with the usage of fexit trampolines.
> >
> > The discussion sited is more about GPL than anything else.
> >
> > The LSM rule is that any security module must be able to
> > accept the decisions of others. SELinux has to accept decisions
> > made ahead of it. It always has, as LSM checks occur after
> > "traditional" checks, which may fail. The only reason that you
> > need to be last in this implementation appears to be that you
> > refuse to use the general mechanisms. You can't blame SELinux
> > for that.
>
> Okay, this is why I wanted to try to state things plainly. The "in last
> position" appears to be the result of a couple design choices:
>
> -the idea of "not wanting to get in the way of other LSMs", while
> admirable, needs to actually be a non-goal to be "just" a stacked LSM
> (as you're saying here Casey). This position _was_ required for the
> non-privileged LSM case to avoid security implications, but that goal
> not longer exists here either.
>
> -optimally using the zero-cost call-outs (static key + fexit trampolines)
> meant it didn't interact well with the existing stacking mechanism.
>
> So, fine, these appear to be design choices, not *specifically*
> requirements. Let's move on, I think there is more to unpack here...
>
> > >>>> 2- BPF hooks don't know what may be attached at any given time, so
> > >>>> ALL LSM hooks need to be universally hooked. THIS turns out to create
> > >>>> a measurable performance problem in that the cost of the indirect call
> > >>>> on the (mostly/usually) empty BPF policy is too high.
> > >>> also no.
>
> AIUI, there was some confusion on Alexei's reply here. I, perhaps,
> was not as clear as I needed to be. I think the later discussion on
> performance overheads gets more into the point, and gets us closer to
> the objections Alexei had. More below...
>
> > > This approach still had the issues of an indirect call and an
> > > extra check when not used. So this was not truly zero overhead even
> > > after special casing BPF.
> >
> > The LSM mechanism is not zero overhead. It never has been. That's why
> > you can compile it out. You get added value at a price. You get the
> > ability to use SELinux and KRSI together at a price. If that's unacceptable
> > you can go the route of seccomp, which doesn't use LSM for many of the
> > same reasons you're on about.
> > [...]
> > >>>> So, trying to avoid the indirect calls is, as you say, an optimization,
> > >>>> but it might be a needed one due to the other limitations.
> > >>> I'm convinced that avoiding the cost of retpoline in critical path is a
> > >>> requirement for any new infrastructure in the kernel.
> > >> Sorry, I haven't gotten that memo.
>
> I agree with Casey here -- it's a nice goal, but those cost evaluations have
> not yet(?[1]) hit the LSM world. I think it's a desirable goal, to be
> sure, but this does appear to be an early optimization.
>
> > [...]
> > It can do that wholly within KRSI hooks. You don't need to
> > put KRSI specific code into security.c.
>
> This observation is where I keep coming back to.
>
> Yes, the resulting code is not as fast as it could be. The fact that BPF
> triggers the worst-case performance of LSM hooking is the "new" part
> here, from what I can see.
>
> I suspect the problem is that folks in the BPF subsystem don't want to
> be seen as slowing anything down, even other subsystems, so they don't
> want to see this done in the traditional LSM hooking way (which contains
> indirect calls).
>
> But the LSM subsystem doesn't want special cases (Casey has worked very
> hard to generalize everything there for stacking). It is really hard to
> accept adding a new special case when there are still special cases yet
> to be worked out even in the LSM code itself[2].
>
> > >>> Networking stack converted all such places to conditional calls.
> > >>> In BPF land we converted indirect calls to direct jumps and direct calls.
> > >>> It took two years to do so. Adding new indirect calls is not an option.
> > >>> I'm eagerly waiting for Peter's static_call patches to land to convert
> > >>> a lot more indirect calls. May be existing LSMs will take advantage
> > >>> of static_call patches too, but static_call is not an option for BPF.
> > >>> That's why we introduced BPF trampoline in the last kernel release.
> > >> Sorry, but I don't see how BPF is so overwhelmingly special.
> > > My analogy here is that if every tracepoint in the kernel were of the
> > > type:
> > >
> > > if (trace_foo_enabled) // <-- Overhead here, solved with static key
> > > trace_foo(a); // <-- retpoline overhead, solved with fexit trampolines
>
> This is a helpful distillation; thanks.
>
> static keys (perhaps better described as static branches) make sense to
> me; I'm familiar with them being used all over the place[3]. The resulting
> "zero performance" branch mechanism is extremely handy.
>
> I had been thinking about the fexit stuff only as a way for BPF to call
> into kernel functions directly, and I missed the place where this got
> used for calling from the kernel into BPF directly. KP walked me through
> the fexit stuff off list. I missed where there NOP stub ("noinline int
> bpf_lsm_##NAME(__VA_ARGS__) { return 0; }") was being patched by BPF in
> https://lore.kernel.org/lkml/20200220175250.10795-6-kpsingh@chromium.org/
> The key bit being "bpf_trampoline_update(prog)"
>
> > > It would be very hard to justify enabling them on a production system,
> > > and the same can be said for BPF and KRSI.
> >
> > The same can be and has been said about the LSM infrastructure.
> > If BPF and KRSI are that performance critical you shouldn't be
> > tying them to LSM, which is known to have overhead. If you can't
> > accept the LSM overhead, get out of the LSM. Or, help us fix the
> > LSM infrastructure to make its overhead closer to zero. Whether
> > you believe it or not, a lot of work has gone into keeping the LSM
> > overhead as small as possible while remaining sufficiently general
> > to perform its function.
> >
> > No. If you're too special to play by LSM rules then you're special
> > enough to get into the kernel using more direct means.
>
> So, I see the primary conflict here being about the performance
> optimizations. AIUI:
>
> - BPF subsystem maintainers do not want any new slowdown associated
> with BPF
> - LSM subsystem maintainers do not want any new special cases in
> LSM stacking
>
> So, unless James is going to take this over Casey's objections, the path
> forward I see here is:
>
> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> - optimize calling for all LSMs
I will work on v5 which resgisters the nops as standard LSM hooks and
we can follow-up on performance.
- KP
>
> Does this seem right to everyone?
>
> -Kees
>
>
> [1] There is a "known cost to LSM", but as Casey mentions, it's been
> generally deemed "acceptable". There have been some recent attempts to
> quantify it, but it's not been very clear:
> https://lore.kernel.org/linux-security-module/c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov/ (lore is missing half this conversation for some reason)
>
> [2] Casey's work to generalize the LSM interfaces continues and it quite
> complex:
> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>
> [3] E.g. HARDENED_USERCOPY uses it:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/usercopy.c?h=v5.5#n258
> and so does the heap memory auto-initialization:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/mm/slab.h?h=v5.5#n676
>
> --
> Kees Cook
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-25 19:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kees Cook, Casey Schaufler, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev
In-Reply-To: <20200225054125.dttrc3fvllzu4mx5@ast-mbp>
On 24-Feb 21:41, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
> >
> > But the LSM subsystem doesn't want special cases (Casey has worked very
> > hard to generalize everything there for stacking). It is really hard to
> > accept adding a new special case when there are still special cases yet
> > to be worked out even in the LSM code itself[2].
> > [2] Casey's work to generalize the LSM interfaces continues and it quite
> > complex:
> > https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
>
> I think the key mistake we made is that we classified KRSI as LSM.
> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> I don't see anything in LSM infra that KRSI can reuse.
> The only thing BPF needs is a function to attach to.
> It can be a nop function or any other.
> security_*() functions are interesting from that angle only.
> Hence I propose to reconsider what I was suggesting earlier.
> No changes to secruity/ directory.
> Attach to security_*() funcs via bpf trampoline.
> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> I think "security" is also loaded word that should be avoided.
> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
The BPF_PROG_TYPE_OVERRIDE_RETURN seems to be useful in general as
well and we have the implementation already figured out as a part of
the LSM work. I will split that bit into a separate series.
- KP
>
> > So, unless James is going to take this over Casey's objections, the path
> > forward I see here is:
> >
> > - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> > - optimize calling for all LSMs
>
> I'm very much surprised how 'slow' KRSI is an option at all.
> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> functions for every place in the kernel that calls security_*().
> This is not an acceptable overhead. Even w/o retpoline
> this is not something datacenter servers can use.
>
> Another option is to do this:
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..7887ce636fb1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> return kernel_load_data_str[id];
> }
>
> -#ifdef CONFIG_SECURITY
> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
>
> Single line change to security.h and new file kernel/bpf/override_security.c
> that will look like:
> int security_binder_set_context_mgr(struct task_struct *mgr)
> {
> return 0;
> }
>
> int security_binder_transaction(struct task_struct *from,
> struct task_struct *to)
> {
> return 0;
> }
> Essentially it will provide BPF side with a set of nop functions.
> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> it's a pro not a con.
^ permalink raw reply
* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Jann Horn @ 2020-02-25 20:49 UTC (permalink / raw)
To: Mickaël Salaün
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <20200224160215.4136-2-mic@digikod.net>
On Mon, Feb 24, 2020 at 5:05 PM Mickaël Salaün <mic@digikod.net> wrote:
> A Landlock object enables to identify a kernel object (e.g. an inode).
> A Landlock rule is a set of access rights allowed on an object. Rules
> are grouped in rulesets that may be tied to a set of processes (i.e.
> subjects) to enforce a scoped access-control (i.e. a domain).
>
> Because Landlock's goal is to empower any process (especially
> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
> object identification such as file extended attributes. Indeed, we need
> innocuous, composable and modular access-controls.
>
> The main challenge with this constraints is to identify kernel objects
> while this identification is useful (i.e. when a security policy makes
> use of this object). But this identification data should be freed once
> no policy is using it. This ephemeral tagging should not and may not be
> written in the filesystem. We then need to manage the lifetime of a
> rule according to the lifetime of its object. To avoid a global lock,
> this implementation make use of RCU and counters to safely reference
> objects.
>
> A following commit uses this generic object management for inodes.
[...]
> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
> new file mode 100644
> index 000000000000..4a321d5b3f67
> --- /dev/null
> +++ b/security/landlock/Kconfig
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +
> +config SECURITY_LANDLOCK
> + bool "Landlock support"
> + depends on SECURITY
> + default n
(I think "default n" is implicit?)
> + help
> + This selects Landlock, a safe sandboxing mechanism. It enables to
> + restrict processes on the fly (i.e. enforce an access control policy),
> + which can complement seccomp-bpf. The security policy is a set of access
> + rights tied to an object, which could be a file, a socket or a process.
> +
> + See Documentation/security/landlock/ for further information.
> +
> + If you are unsure how to answer this question, answer N.
[...]
> diff --git a/security/landlock/object.c b/security/landlock/object.c
> new file mode 100644
> index 000000000000..38fbbb108120
> --- /dev/null
> +++ b/security/landlock/object.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Landlock LSM - Object and rule management
> + *
> + * Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
> + * Copyright © 2018-2020 ANSSI
> + *
> + * Principles and constraints of the object and rule management:
> + * - Do not leak memory.
> + * - Try as much as possible to free a memory allocation as soon as it is
> + * unused.
> + * - Do not use global lock.
> + * - Do not charge processes other than the one requesting a Landlock
> + * operation.
> + */
> +
> +#include <linux/bug.h>
> +#include <linux/compiler.h>
> +#include <linux/compiler_types.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/fs.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/rbtree.h>
> +#include <linux/rcupdate.h>
> +#include <linux/refcount.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/workqueue.h>
> +
> +#include "object.h"
> +
> +struct landlock_object *landlock_create_object(
> + const enum landlock_object_type type, void *underlying_object)
> +{
> + struct landlock_object *object;
> +
> + if (WARN_ON_ONCE(!underlying_object))
> + return NULL;
> + object = kzalloc(sizeof(*object), GFP_KERNEL);
> + if (!object)
> + return NULL;
> + refcount_set(&object->usage, 1);
> + refcount_set(&object->cleaners, 1);
> + spin_lock_init(&object->lock);
> + INIT_LIST_HEAD(&object->rules);
> + object->type = type;
> + WRITE_ONCE(object->underlying_object, underlying_object);
`object` is not globally visible at this point, so WRITE_ONCE() is unnecessary.
> + return object;
> +}
> +
> +struct landlock_object *landlock_get_object(struct landlock_object *object)
> + __acquires(object->usage)
> +{
> + __acquire(object->usage);
> + /*
> + * If @object->usage equal 0, then it will be ignored by writers, and
> + * underlying_object->object may be replaced, but this is not an issue
> + * for release_object().
> + */
> + if (object && refcount_inc_not_zero(&object->usage)) {
> + /*
> + * It should not be possible to get a reference to an object if
> + * its underlying object is being terminated (e.g. with
> + * landlock_release_object()), because an object is only
> + * modifiable through such underlying object. This is not the
> + * case with landlock_get_object_cleaner().
> + */
> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
> + return object;
> + }
> + return NULL;
> +}
> +
> +static struct landlock_object *get_object_cleaner(
> + struct landlock_object *object)
> + __acquires(object->cleaners)
> +{
> + __acquire(object->cleaners);
> + if (object && refcount_inc_not_zero(&object->cleaners))
> + return object;
> + return NULL;
> +}
I don't get this whole "cleaners" thing. Can you give a quick
description of why this is necessary, and what benefits it has over a
standard refcounting+RCU scheme? I don't immediately see anything that
requires this.
> +/*
> + * There is two cases when an object should be free and the reference to the
> + * underlying object should be put:
> + * - when the last rule tied to this object is removed, which is handled by
> + * landlock_put_rule() and then release_object();
> + * - when the object is being terminated (e.g. no more reference to an inode),
> + * which is handled by landlock_put_object().
> + */
> +static void put_object_free(struct landlock_object *object)
> + __releases(object->cleaners)
> +{
> + __release(object->cleaners);
> + if (!refcount_dec_and_test(&object->cleaners))
> + return;
> + WARN_ON_ONCE(refcount_read(&object->usage));
> + /*
> + * Ensures a safe use of @object in the RCU block from
> + * landlock_put_rule().
> + */
> + kfree_rcu(object, rcu_free);
> +}
> +
> +/*
> + * Destroys a newly created and useless object.
> + */
> +void landlock_drop_object(struct landlock_object *object)
> +{
> + if (WARN_ON_ONCE(!refcount_dec_and_test(&object->usage)))
> + return;
> + __acquire(object->cleaners);
> + put_object_free(object);
> +}
> +
> +/*
> + * Puts the underlying object (e.g. inode) if it is the first request to
> + * release @object, without calling landlock_put_object().
> + *
> + * Return true if this call effectively marks @object as released, false
> + * otherwise.
> + */
> +static bool release_object(struct landlock_object *object)
> + __releases(&object->lock)
> +{
> + void *underlying_object;
> +
> + lockdep_assert_held(&object->lock);
> +
> + underlying_object = xchg(&object->underlying_object, NULL);
> + spin_unlock(&object->lock);
> + might_sleep();
> + if (!underlying_object)
> + return false;
> +
> + switch (object->type) {
> + case LANDLOCK_OBJECT_INODE:
> + break;
> + default:
> + WARN_ON_ONCE(1);
> + }
> + return true;
> +}
> +
> +static void put_object_cleaner(struct landlock_object *object)
> + __releases(object->cleaners)
> +{
> + /* Let's try an early lockless check. */
> + if (list_empty(&object->rules) &&
> + READ_ONCE(object->underlying_object)) {
> + /*
> + * Puts @object if there is no rule tied to it and the
> + * remaining user is the underlying object. This check is
> + * atomic because @object->rules and @object->underlying_object
> + * are protected by @object->lock.
> + */
> + spin_lock(&object->lock);
> + if (list_empty(&object->rules) &&
> + READ_ONCE(object->underlying_object) &&
> + refcount_dec_if_one(&object->usage)) {
> + /*
> + * Releases @object, in place of
> + * landlock_release_object().
> + *
> + * @object is already empty, implying that all its
> + * previous rules are already disabled.
> + *
> + * Unbalance the @object->cleaners counter to reflect
> + * the underlying object release.
> + */
> + if (!WARN_ON_ONCE(!release_object(object))) {
> + __acquire(object->cleaners);
> + put_object_free(object);
> + }
> + } else {
> + spin_unlock(&object->lock);
> + }
> + }
> + put_object_free(object);
> +}
> +
> +/*
> + * Putting an object is easy when the object is being terminated, but it is
> + * much more tricky when the reason is that there is no more rule tied to this
> + * object. Indeed, new rules could be added at the same time.
> + */
> +void landlock_put_object(struct landlock_object *object)
> + __releases(object->usage)
> +{
> + struct landlock_object *object_cleaner;
> +
> + __release(object->usage);
> + might_sleep();
> + if (!object)
> + return;
> + /*
> + * Guards against concurrent termination to be able to terminate
> + * @object if it is empty and not referenced by another rule-appender
> + * other than the underlying object.
> + */
> + object_cleaner = get_object_cleaner(object);
> + if (WARN_ON_ONCE(!object_cleaner)) {
> + __release(object->cleaners);
> + return;
> + }
> + /*
> + * Decrements @object->usage and if it reach zero, also decrement
> + * @object->cleaners. If both reach zero, then release and free
> + * @object.
> + */
> + if (refcount_dec_and_test(&object->usage)) {
> + struct landlock_rule *rule_walker, *rule_walker2;
> +
> + spin_lock(&object->lock);
> + /*
> + * Disables all the rules tied to @object when it is forbidden
> + * to add new rule but still allowed to remove them with
> + * landlock_put_rule(). This is crucial to be able to safely
> + * free a rule according to landlock_rule_is_disabled().
> + */
> + list_for_each_entry_safe(rule_walker, rule_walker2,
> + &object->rules, list)
> + list_del_rcu(&rule_walker->list);
> +
> + /*
> + * Releases @object if it is not already released (e.g. with
> + * landlock_release_object()).
> + */
> + release_object(object);
> + /*
> + * Unbalances the @object->cleaners counter to reflect the
> + * underlying object release.
> + */
> + __acquire(object->cleaners);
> + put_object_free(object);
> + }
> + put_object_cleaner(object_cleaner);
> +}
> +
> +void landlock_put_rule(struct landlock_object *object,
> + struct landlock_rule *rule)
> +{
> + if (!rule)
> + return;
> + WARN_ON_ONCE(!object);
> + /*
> + * Guards against a concurrent @object self-destruction with
> + * landlock_put_object() or put_object_cleaner().
> + */
> + rcu_read_lock();
> + if (landlock_rule_is_disabled(rule)) {
> + rcu_read_unlock();
> + if (refcount_dec_and_test(&rule->usage))
> + kfree_rcu(rule, rcu_free);
> + return;
> + }
> + if (refcount_dec_and_test(&rule->usage)) {
> + struct landlock_object *safe_object;
> +
> + /*
> + * Now, @rule may still be enabled, or in the process of being
> + * untied to @object by put_object_cleaner(). However, we know
> + * that @object will not be freed until rcu_read_unlock() and
> + * until @object->cleaners reach zero. Furthermore, we may not
> + * be the only one willing to free a @rule linked with @object.
> + * If we succeed to hold @object with get_object_cleaner(), we
> + * know that until put_object_cleaner(), we can safely use
> + * @object to remove @rule.
> + */
> + safe_object = get_object_cleaner(object);
> + rcu_read_unlock();
> + if (!safe_object) {
> + __release(safe_object->cleaners);
> + /*
> + * We can safely free @rule because it is already
> + * removed from @object's list.
> + */
> + WARN_ON_ONCE(!landlock_rule_is_disabled(rule));
> + kfree_rcu(rule, rcu_free);
> + } else {
> + spin_lock(&safe_object->lock);
> + if (!landlock_rule_is_disabled(rule))
> + list_del(&rule->list);
> + spin_unlock(&safe_object->lock);
> + kfree_rcu(rule, rcu_free);
> + put_object_cleaner(safe_object);
> + }
> + } else {
> + rcu_read_unlock();
> + }
> + /*
> + * put_object_cleaner() might sleep, but it is only reachable if
> + * !landlock_rule_is_disabled(). Therefore, clean_ref() can not sleep.
> + */
> + might_sleep();
> +}
> +
> +void landlock_release_object(struct landlock_object __rcu *rcu_object)
> +{
> + struct landlock_object *object;
> +
> + if (!rcu_object)
> + return;
> + rcu_read_lock();
> + object = get_object_cleaner(rcu_dereference(rcu_object));
This is not how RCU works. You need the rcu annotation on the access
to the data structure member (or global variable) that's actually
being accessed. A "struct foo __rcu *foo" argument is essentially
always wrong.
> +struct landlock_rule {
> + struct landlock_access access;
> + /*
> + * @list: Linked list with other rules tied to the same object, which
> + * enable to manage their lifetimes. This is also used to identify if
> + * a rule is still valid, thanks to landlock_rule_is_disabled(), which
> + * is important in the matching process because the original object
> + * address might have been recycled.
> + */
> + struct list_head list;
> + union {
> + /*
> + * @usage: Number of rulesets pointing to this rule. This
> + * field is never used by RCU readers.
> + */
> + refcount_t usage;
> + struct rcu_head rcu_free;
> + };
> +};
An object that is subject to RCU but whose refcount must not be
accessed from RCU context? That seems a weird.
> +enum landlock_object_type {
> + LANDLOCK_OBJECT_INODE = 1,
> +};
> +
> +struct landlock_object {
> + /*
> + * @usage: Main usage counter, used to tie an object to it's underlying
> + * object (i.e. create a lifetime) and potentially add new rules.
I can't really follow this by reading this patch on its own. As one
suggestion to make things at least a bit better, how about documenting
here that `usage` always reaches zero before `cleaners` does?
> + */
> + refcount_t usage;
> + /*
> + * @cleaners: Usage counter used to free a rule from @rules (thanks to
> + * put_rule()). Enables to get a reference to this object until it
> + * really become freed. Cf. put_object().
Maybe add: @usage being non-zero counts as one reference to @cleaners.
Once @cleaners has become zero, the object is freed after an RCU grace
period.
> + */
> + refcount_t cleaners;
> + union {
> + /*
> + * The use of this struct is controlled by @usage and
> + * @cleaners, which makes it safe to union it with @rcu_free.
> + */
[...]
> + struct rcu_head rcu_free;
> + };
> +};
[...]
> +static inline bool landlock_rule_is_disabled(
> + struct landlock_rule *rule)
> +{
> + /*
> + * Disabling (i.e. unlinking) a landlock_rule is a one-way operation.
> + * It is not possible to re-enable such a rule, then there is no need
> + * for smp_load_acquire().
> + *
> + * LIST_POISON2 is set by list_del() and list_del_rcu().
> + */
> + return !rule || READ_ONCE(rule->list.prev) == LIST_POISON2;
You're not allowed to do this, the comment above list_del() states:
* Note: list_empty() on entry does not return true after this, the entry is
* in an undefined state.
If you want to be able to test whether the element is on a list
afterwards, use stuff like list_del_init().
> +}
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: Casey Schaufler @ 2020-02-26 0:30 UTC (permalink / raw)
To: Alexei Starovoitov, Kees Cook
Cc: KP Singh, LKML, Linux Security Module list, Alexei Starovoitov,
James Morris, bpf, netdev, Casey Schaufler
In-Reply-To: <20200225054125.dttrc3fvllzu4mx5@ast-mbp>
On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
> On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
>> But the LSM subsystem doesn't want special cases (Casey has worked very
>> hard to generalize everything there for stacking). It is really hard to
>> accept adding a new special case when there are still special cases yet
>> to be worked out even in the LSM code itself[2].
>> [2] Casey's work to generalize the LSM interfaces continues and it quite
>> complex:
>> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> I think the key mistake we made is that we classified KRSI as LSM.
> LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> I don't see anything in LSM infra that KRSI can reuse.
> The only thing BPF needs is a function to attach to.
> It can be a nop function or any other.
> security_*() functions are interesting from that angle only.
> Hence I propose to reconsider what I was suggesting earlier.
> No changes to secruity/ directory.
> Attach to security_*() funcs via bpf trampoline.
> The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> I think "security" is also loaded word that should be avoided.
No argument there.
> I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
>
>> So, unless James is going to take this over Casey's objections, the path
>> forward I see here is:
>>
>> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
>> - optimize calling for all LSMs
> I'm very much surprised how 'slow' KRSI is an option at all.
> 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> functions for every place in the kernel that calls security_*().
> This is not an acceptable overhead. Even w/o retpoline
> this is not something datacenter servers can use.
In the universe I live in data centers will disable hyper-threading,
reducing performance substantially, in the face of hypothetical security
exploits. That's a massively greater performance impact than the handful
of instructions required to do indirect calls. Not to mention the impact
of the BPF programs that have been included. Have you ever looked at what
happens to system performance when polkitd is enabled?
>
> Another option is to do this:
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 64b19f050343..7887ce636fb1 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> return kernel_load_data_str[id];
> }
>
> -#ifdef CONFIG_SECURITY
> +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
>
> Single line change to security.h and new file kernel/bpf/override_security.c
> that will look like:
> int security_binder_set_context_mgr(struct task_struct *mgr)
> {
> return 0;
> }
>
> int security_binder_transaction(struct task_struct *from,
> struct task_struct *to)
> {
> return 0;
> }
> Essentially it will provide BPF side with a set of nop functions.
> CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> it's a pro not a con.
Err, no. All distros use an LSM or two. Unless you can re-implement SELinux
in BPF (good luck with state transitions) you've built a warp drive without
ever having mined dilithium crystals.
^ permalink raw reply
* Re: [PATCH bpf-next v4 3/8] bpf: lsm: provide attachment points for BPF LSM programs
From: KP Singh @ 2020-02-26 5:15 UTC (permalink / raw)
To: Casey Schaufler
Cc: Alexei Starovoitov, Kees Cook, LKML, Linux Security Module list,
Alexei Starovoitov, James Morris, bpf, netdev
In-Reply-To: <4b56177f-8148-177b-e1e5-c98da86b3b01@schaufler-ca.com>
On 25-Feb 16:30, Casey Schaufler wrote:
> On 2/24/2020 9:41 PM, Alexei Starovoitov wrote:
> > On Mon, Feb 24, 2020 at 01:41:19PM -0800, Kees Cook wrote:
> >> But the LSM subsystem doesn't want special cases (Casey has worked very
> >> hard to generalize everything there for stacking). It is really hard to
> >> accept adding a new special case when there are still special cases yet
> >> to be worked out even in the LSM code itself[2].
> >> [2] Casey's work to generalize the LSM interfaces continues and it quite
> >> complex:
> >> https://lore.kernel.org/linux-security-module/20200214234203.7086-1-casey@schaufler-ca.com/
> > I think the key mistake we made is that we classified KRSI as LSM.
> > LSM stacking, lsmblobs that the above set is trying to do are not necessary for KRSI.
> > I don't see anything in LSM infra that KRSI can reuse.
> > The only thing BPF needs is a function to attach to.
> > It can be a nop function or any other.
> > security_*() functions are interesting from that angle only.
> > Hence I propose to reconsider what I was suggesting earlier.
> > No changes to secruity/ directory.
> > Attach to security_*() funcs via bpf trampoline.
> > The key observation vs what I was saying earlier is KRSI and LSM are wrong names.
> > I think "security" is also loaded word that should be avoided.
>
> No argument there.
>
> > I'm proposing to rename BPF_PROG_TYPE_LSM into BPF_PROG_TYPE_OVERRIDE_RETURN.
> >
> >> So, unless James is going to take this over Casey's objections, the path
> >> forward I see here is:
> >>
> >> - land a "slow" KRSI (i.e. one that hooks every hook with a stub).
> >> - optimize calling for all LSMs
> > I'm very much surprised how 'slow' KRSI is an option at all.
> > 'slow' KRSI means that CONFIG_SECURITY_KRSI=y adds indirect calls to nop
> > functions for every place in the kernel that calls security_*().
> > This is not an acceptable overhead. Even w/o retpoline
> > this is not something datacenter servers can use.
>
> In the universe I live in data centers will disable hyper-threading,
> reducing performance substantially, in the face of hypothetical security
> exploits. That's a massively greater performance impact than the handful
> of instructions required to do indirect calls. Not to mention the impact
Indirect calls have worse performance implications than just a few
instructions and are especially not suitable for hotpaths.
There have been multiple efforts to reduce their usage e.g.:
- https://lwn.net/Articles/774743/
- https://lwn.net/Articles/773985/
> of the BPF programs that have been included. Have you ever looked at what
BPF programs are JIT'ed and optimized to native code.
> happens to system performance when polkitd is enabled?
However, let's discuss all this separately when we follow-up with
performance improvements after submitting the initial patch-set.
>
>
> >
> > Another option is to do this:
> > diff --git a/include/linux/security.h b/include/linux/security.h
> > index 64b19f050343..7887ce636fb1 100644
> > --- a/include/linux/security.h
> > +++ b/include/linux/security.h
> > @@ -240,7 +240,7 @@ static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> > return kernel_load_data_str[id];
> > }
> >
> > -#ifdef CONFIG_SECURITY
> > +#if defined(CONFIG_SECURITY) || defined(CONFIG_BPF_OVERRIDE_RETURN)
> >
> > Single line change to security.h and new file kernel/bpf/override_security.c
> > that will look like:
> > int security_binder_set_context_mgr(struct task_struct *mgr)
> > {
> > return 0;
> > }
> >
> > int security_binder_transaction(struct task_struct *from,
> > struct task_struct *to)
> > {
> > return 0;
> > }
> > Essentially it will provide BPF side with a set of nop functions.
> > CONFIG_SECURITY is off. It may seem as a downside that it will force a choice
> > on kernel users. Either they build the kernel with CONFIG_SECURITY and their
> > choice of LSMs or build the kernel with CONFIG_BPF_OVERRIDE_RETURN and use
> > BPF_PROG_TYPE_OVERRIDE_RETURN programs to enforce any kind of policy. I think
> > it's a pro not a con.
>
> Err, no. All distros use an LSM or two. Unless you can re-implement SELinux
The users mentioned here in this context are (I would assume) the more
performance sensitive users who would, potentially, disable
CONFIG_SECURITY because of the current performance characteristics.
We can also discuss this separately and only if we find that we need
it for the BPF_OVERRIDE_RET type attachment.
- KP
> in BPF (good luck with state transitions) you've built a warp drive without
> ever having mined dilithium crystals.
>
>
^ permalink raw reply
* Re: [RFC PATCH v14 01/10] landlock: Add object and rule management
From: Mickaël Salaün @ 2020-02-26 15:31 UTC (permalink / raw)
To: Jann Horn
Cc: kernel list, Al Viro, Andy Lutomirski, Arnd Bergmann,
Casey Schaufler, Greg Kroah-Hartman, James Morris, Jann Horn,
Jonathan Corbet, Kees Cook, Michael Kerrisk,
Mickaël Salaün, Serge E . Hallyn, Shuah Khan,
Vincent Dagonneau, Kernel Hardening, Linux API, linux-arch,
linux-doc, linux-fsdevel, open list:KERNEL SELFTEST FRAMEWORK,
linux-security-module, the arch/x86 maintainers
In-Reply-To: <CAG48ez1FN0B05r35c-EDuQNoW=5ZTy1iBzksbkt+toqs+_tdqg@mail.gmail.com>
On 25/02/2020 21:49, Jann Horn wrote:
> On Mon, Feb 24, 2020 at 5:05 PM Mickaël Salaün <mic@digikod.net> wrote:
>> A Landlock object enables to identify a kernel object (e.g. an inode).
>> A Landlock rule is a set of access rights allowed on an object. Rules
>> are grouped in rulesets that may be tied to a set of processes (i.e.
>> subjects) to enforce a scoped access-control (i.e. a domain).
>>
>> Because Landlock's goal is to empower any process (especially
>> unprivileged ones) to sandbox themselves, we can't rely on a system-wide
>> object identification such as file extended attributes. Indeed, we need
>> innocuous, composable and modular access-controls.
>>
>> The main challenge with this constraints is to identify kernel objects
>> while this identification is useful (i.e. when a security policy makes
>> use of this object). But this identification data should be freed once
>> no policy is using it. This ephemeral tagging should not and may not be
>> written in the filesystem. We then need to manage the lifetime of a
>> rule according to the lifetime of its object. To avoid a global lock,
>> this implementation make use of RCU and counters to safely reference
>> objects.
>>
>> A following commit uses this generic object management for inodes.
> [...]
>> diff --git a/security/landlock/Kconfig b/security/landlock/Kconfig
>> new file mode 100644
>> index 000000000000..4a321d5b3f67
>> --- /dev/null
>> +++ b/security/landlock/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config SECURITY_LANDLOCK
>> + bool "Landlock support"
>> + depends on SECURITY
>> + default n
>
> (I think "default n" is implicit?)
It seems that most (all?) Kconfig are written like this.
>
>> + help
>> + This selects Landlock, a safe sandboxing mechanism. It enables to
>> + restrict processes on the fly (i.e. enforce an access control policy),
>> + which can complement seccomp-bpf. The security policy is a set of access
>> + rights tied to an object, which could be a file, a socket or a process.
>> +
>> + See Documentation/security/landlock/ for further information.
>> +
>> + If you are unsure how to answer this question, answer N.
> [...]
>> diff --git a/security/landlock/object.c b/security/landlock/object.c
>> new file mode 100644
>> index 000000000000..38fbbb108120
>> --- /dev/null
>> +++ b/security/landlock/object.c
>> @@ -0,0 +1,339 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Landlock LSM - Object and rule management
>> + *
>> + * Copyright © 2016-2020 Mickaël Salaün <mic@digikod.net>
>> + * Copyright © 2018-2020 ANSSI
>> + *
>> + * Principles and constraints of the object and rule management:
>> + * - Do not leak memory.
>> + * - Try as much as possible to free a memory allocation as soon as it is
>> + * unused.
>> + * - Do not use global lock.
>> + * - Do not charge processes other than the one requesting a Landlock
>> + * operation.
>> + */
>> +
>> +#include <linux/bug.h>
>> +#include <linux/compiler.h>
>> +#include <linux/compiler_types.h>
>> +#include <linux/err.h>
>> +#include <linux/errno.h>
>> +#include <linux/fs.h>
>> +#include <linux/kernel.h>
>> +#include <linux/list.h>
>> +#include <linux/rbtree.h>
>> +#include <linux/rcupdate.h>
>> +#include <linux/refcount.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/workqueue.h>
>> +
>> +#include "object.h"
>> +
>> +struct landlock_object *landlock_create_object(
>> + const enum landlock_object_type type, void *underlying_object)
>> +{
>> + struct landlock_object *object;
>> +
>> + if (WARN_ON_ONCE(!underlying_object))
>> + return NULL;
>> + object = kzalloc(sizeof(*object), GFP_KERNEL);
>> + if (!object)
>> + return NULL;
>> + refcount_set(&object->usage, 1);
>> + refcount_set(&object->cleaners, 1);
>> + spin_lock_init(&object->lock);
>> + INIT_LIST_HEAD(&object->rules);
>> + object->type = type;
>> + WRITE_ONCE(object->underlying_object, underlying_object);
>
> `object` is not globally visible at this point, so WRITE_ONCE() is unnecessary.
Right. It was written like this to have a uniform use of this pointer,
but I'll remove it.
>
>> + return object;
>> +}
>> +
>> +struct landlock_object *landlock_get_object(struct landlock_object *object)
>> + __acquires(object->usage)
>> +{
>> + __acquire(object->usage);
>> + /*
>> + * If @object->usage equal 0, then it will be ignored by writers, and
>> + * underlying_object->object may be replaced, but this is not an issue
>> + * for release_object().
>> + */
>> + if (object && refcount_inc_not_zero(&object->usage)) {
>> + /*
>> + * It should not be possible to get a reference to an object if
>> + * its underlying object is being terminated (e.g. with
>> + * landlock_release_object()), because an object is only
>> + * modifiable through such underlying object. This is not the
>> + * case with landlock_get_object_cleaner().
>> + */
>> + WARN_ON_ONCE(!READ_ONCE(object->underlying_object));
>> + return object;
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct landlock_object *get_object_cleaner(
>> + struct landlock_object *object)
>> + __acquires(object->cleaners)
>> +{
>> + __acquire(object->cleaners);
>> + if (object && refcount_inc_not_zero(&object->cleaners))
>> + return object;
>> + return NULL;
>> +}
>
> I don't get this whole "cleaners" thing. Can you give a quick
> description of why this is necessary, and what benefits it has over a
> standard refcounting+RCU scheme? I don't immediately see anything that
> requires this.
This indeed needs more documentation here. Here is a comment I'll add to
get_object_cleaner():
This enables to safely get a reference to an object to potentially free
it if it is not already being freed by a concurrent thread. Indeed, the
object's address may still be read and dereferenced while a concurrent
thread is attempting to clean the object. Cf. &struct
landlock_object->usage and &struct landlock_object->cleaners.
See below the explanation about "usage" and "cleaners".
>
>> +/*
>> + * There is two cases when an object should be free and the reference to the
>> + * underlying object should be put:
>> + * - when the last rule tied to this object is removed, which is handled by
>> + * landlock_put_rule() and then release_object();
>> + * - when the object is being terminated (e.g. no more reference to an inode),
>> + * which is handled by landlock_put_object().
>> + */
>> +static void put_object_free(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + __release(object->cleaners);
>> + if (!refcount_dec_and_test(&object->cleaners))
>> + return;
>> + WARN_ON_ONCE(refcount_read(&object->usage));
>> + /*
>> + * Ensures a safe use of @object in the RCU block from
>> + * landlock_put_rule().
>> + */
>> + kfree_rcu(object, rcu_free);
>> +}
>> +
>> +/*
>> + * Destroys a newly created and useless object.
>> + */
>> +void landlock_drop_object(struct landlock_object *object)
>> +{
>> + if (WARN_ON_ONCE(!refcount_dec_and_test(&object->usage)))
>> + return;
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> +}
>> +
>> +/*
>> + * Puts the underlying object (e.g. inode) if it is the first request to
>> + * release @object, without calling landlock_put_object().
>> + *
>> + * Return true if this call effectively marks @object as released, false
>> + * otherwise.
>> + */
>> +static bool release_object(struct landlock_object *object)
>> + __releases(&object->lock)
>> +{
>> + void *underlying_object;
>> +
>> + lockdep_assert_held(&object->lock);
>> +
>> + underlying_object = xchg(&object->underlying_object, NULL);
>> + spin_unlock(&object->lock);
>> + might_sleep();
>> + if (!underlying_object)
>> + return false;
>> +
>> + switch (object->type) {
>> + case LANDLOCK_OBJECT_INODE:
>> + break;
>> + default:
>> + WARN_ON_ONCE(1);
>> + }
>> + return true;
>> +}
>> +
>> +static void put_object_cleaner(struct landlock_object *object)
>> + __releases(object->cleaners)
>> +{
>> + /* Let's try an early lockless check. */
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object)) {
>> + /*
>> + * Puts @object if there is no rule tied to it and the
>> + * remaining user is the underlying object. This check is
>> + * atomic because @object->rules and @object->underlying_object
>> + * are protected by @object->lock.
>> + */
>> + spin_lock(&object->lock);
>> + if (list_empty(&object->rules) &&
>> + READ_ONCE(object->underlying_object) &&
>> + refcount_dec_if_one(&object->usage)) {
>> + /*
>> + * Releases @object, in place of
>> + * landlock_release_object().
>> + *
>> + * @object is already empty, implying that all its
>> + * previous rules are already disabled.
>> + *
>> + * Unbalance the @object->cleaners counter to reflect
>> + * the underlying object release.
>> + */
>> + if (!WARN_ON_ONCE(!release_object(object))) {
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> + }
>> + } else {
>> + spin_unlock(&object->lock);
>> + }
>> + }
>> + put_object_free(object);
>> +}
>> +
>> +/*
>> + * Putting an object is easy when the object is being terminated, but it is
>> + * much more tricky when the reason is that there is no more rule tied to this
>> + * object. Indeed, new rules could be added at the same time.
>> + */
>> +void landlock_put_object(struct landlock_object *object)
>> + __releases(object->usage)
>> +{
>> + struct landlock_object *object_cleaner;
>> +
>> + __release(object->usage);
>> + might_sleep();
>> + if (!object)
>> + return;
>> + /*
>> + * Guards against concurrent termination to be able to terminate
>> + * @object if it is empty and not referenced by another rule-appender
>> + * other than the underlying object.
>> + */
>> + object_cleaner = get_object_cleaner(object);
>> + if (WARN_ON_ONCE(!object_cleaner)) {
>> + __release(object->cleaners);
>> + return;
>> + }
>> + /*
>> + * Decrements @object->usage and if it reach zero, also decrement
>> + * @object->cleaners. If both reach zero, then release and free
>> + * @object.
>> + */
>> + if (refcount_dec_and_test(&object->usage)) {
>> + struct landlock_rule *rule_walker, *rule_walker2;
>> +
>> + spin_lock(&object->lock);
>> + /*
>> + * Disables all the rules tied to @object when it is forbidden
>> + * to add new rule but still allowed to remove them with
>> + * landlock_put_rule(). This is crucial to be able to safely
>> + * free a rule according to landlock_rule_is_disabled().
>> + */
>> + list_for_each_entry_safe(rule_walker, rule_walker2,
>> + &object->rules, list)
>> + list_del_rcu(&rule_walker->list);
>> +
>> + /*
>> + * Releases @object if it is not already released (e.g. with
>> + * landlock_release_object()).
>> + */
>> + release_object(object);
>> + /*
>> + * Unbalances the @object->cleaners counter to reflect the
>> + * underlying object release.
>> + */
>> + __acquire(object->cleaners);
>> + put_object_free(object);
>> + }
>> + put_object_cleaner(object_cleaner);
>> +}
>> +
>> +void landlock_put_rule(struct landlock_object *object,
>> + struct landlock_rule *rule)
>> +{
>> + if (!rule)
>> + return;
>> + WARN_ON_ONCE(!object);
>> + /*
>> + * Guards against a concurrent @object self-destruction with
>> + * landlock_put_object() or put_object_cleaner().
>> + */
>> + rcu_read_lock();
>> + if (landlock_rule_is_disabled(rule)) {
>> + rcu_read_unlock();
>> + if (refcount_dec_and_test(&rule->usage))
>> + kfree_rcu(rule, rcu_free);
>> + return;
>> + }
>> + if (refcount_dec_and_test(&rule->usage)) {
>> + struct landlock_object *safe_object;
>> +
>> + /*
>> + * Now, @rule may still be enabled, or in the process of being
>> + * untied to @object by put_object_cleaner(). However, we know
>> + * that @object will not be freed until rcu_read_unlock() and
>> + * until @object->cleaners reach zero. Furthermore, we may not
>> + * be the only one willing to free a @rule linked with @object.
>> + * If we succeed to hold @object with get_object_cleaner(), we
>> + * know that until put_object_cleaner(), we can safely use
>> + * @object to remove @rule.
>> + */
>> + safe_object = get_object_cleaner(object);
>> + rcu_read_unlock();
>> + if (!safe_object) {
>> + __release(safe_object->cleaners);
>> + /*
>> + * We can safely free @rule because it is already
>> + * removed from @object's list.
>> + */
>> + WARN_ON_ONCE(!landlock_rule_is_disabled(rule));
>> + kfree_rcu(rule, rcu_free);
>> + } else {
>> + spin_lock(&safe_object->lock);
>> + if (!landlock_rule_is_disabled(rule))
>> + list_del(&rule->list);
>> + spin_unlock(&safe_object->lock);
>> + kfree_rcu(rule, rcu_free);
>> + put_object_cleaner(safe_object);
>> + }
>> + } else {
>> + rcu_read_unlock();
>> + }
>> + /*
>> + * put_object_cleaner() might sleep, but it is only reachable if
>> + * !landlock_rule_is_disabled(). Therefore, clean_ref() can not sleep.
>> + */
>> + might_sleep();
>> +}
>> +
>> +void landlock_release_object(struct landlock_object __rcu *rcu_object)
>> +{
>> + struct landlock_object *object;
>> +
>> + if (!rcu_object)
>> + return;
>> + rcu_read_lock();
>> + object = get_object_cleaner(rcu_dereference(rcu_object));
>
> This is not how RCU works. You need the rcu annotation on the access
> to the data structure member (or global variable) that's actually
> being accessed. A "struct foo __rcu *foo" argument is essentially
> always wrong.
Absolutely! I fixed this with the following patch:
diff --git a/security/landlock/fs.c b/security/landlock/fs.c
index 7f3bd4fd04bb..01a48c75f210 100644
--- a/security/landlock/fs.c
+++ b/security/landlock/fs.c
@@ -98,7 +98,9 @@ void landlock_release_inodes(struct super_block *sb)
if (iput_inode)
iput(iput_inode);
- landlock_release_object(inode_landlock(inode)->object);
+ rcu_read_lock();
+ landlock_release_object(rcu_dereference(
+ inode_landlock(inode)->object));
iput_inode = inode;
spin_lock(&sb->s_inode_list_lock);
diff --git a/security/landlock/object.c b/security/landlock/object.c
index 2d373f224989..a0e65a78068d 100644
--- a/security/landlock/object.c
+++ b/security/landlock/object.c
@@ -300,14 +300,16 @@ void landlock_put_rule(struct landlock_object *object,
might_sleep();
}
-void landlock_release_object(struct landlock_object __rcu *rcu_object)
+void landlock_release_object(struct landlock_object *rcu_object)
+ __releases(RCU)
{
struct landlock_object *object;
- if (!rcu_object)
+ if (!rcu_object) {
+ rcu_read_unlock();
return;
- rcu_read_lock();
- object = get_object_cleaner(rcu_dereference(rcu_object));
+ }
+ object = get_object_cleaner(rcu_object);
rcu_read_unlock();
if (unlikely(!object)) {
__release(object->cleaners);
diff --git a/security/landlock/object.h b/security/landlock/object.h
index 15dfc9a75a82..78bfb25d4bcc 100644
--- a/security/landlock/object.h
+++ b/security/landlock/object.h
@@ -12,9 +12,9 @@
#include <linux/compiler_types.h>
#include <linux/list.h>
#include <linux/poison.h>
-#include <linux/rcupdate.h>
#include <linux/refcount.h>
#include <linux/spinlock.h>
+#include <linux/types.h>
struct landlock_access {
/*
@@ -105,7 +105,8 @@ struct landlock_object {
void landlock_put_rule(struct landlock_object *object,
struct landlock_rule *rule);
-void landlock_release_object(struct landlock_object __rcu *rcu_object);
+void landlock_release_object(struct landlock_object *object)
+ __releases(RCU);
struct landlock_object *landlock_create_object(
const enum landlock_object_type type, void *underlying_object);
>
>> +struct landlock_rule {
>> + struct landlock_access access;
>> + /*
>> + * @list: Linked list with other rules tied to the same object, which
>> + * enable to manage their lifetimes. This is also used to identify if
>> + * a rule is still valid, thanks to landlock_rule_is_disabled(), which
>> + * is important in the matching process because the original object
>> + * address might have been recycled.
>> + */
>> + struct list_head list;
>> + union {
>> + /*
>> + * @usage: Number of rulesets pointing to this rule. This
>> + * field is never used by RCU readers.
>> + */
>> + refcount_t usage;
>> + struct rcu_head rcu_free;
>> + };
>> +};
>
> An object that is subject to RCU but whose refcount must not be
> accessed from RCU context? That seems a weird.
The fields "access" and "list" are read (in a RCU-read block) by
ruleset.c:landlock_find_access() (cf. patch 2). The use of the "usage"
counter is in landlock_insert_ruleset_rule() and landlock_put_rule(),
but in these cases the rule is always owned/held by the caller. I should
say something like "This field must only be used when already holding
the rule."
>
>> +enum landlock_object_type {
>> + LANDLOCK_OBJECT_INODE = 1,
>> +};
>> +
>> +struct landlock_object {
>> + /*
>> + * @usage: Main usage counter, used to tie an object to it's underlying
>> + * object (i.e. create a lifetime) and potentially add new rules.
>
> I can't really follow this by reading this patch on its own. As one
> suggestion to make things at least a bit better, how about documenting
> here that `usage` always reaches zero before `cleaners` does?
What about this?
This counter is used to tie an object to its underlying object (e.g. an
inode) and to modify it (e.g. add or remove a rule). If this counter
reaches zero, the object must not be modified, but it may still be used
from within an RCU-read block. When adding a new rule to an object with
a usage counter of zero, the underlying object must be locked and its
object pointer can then be replaced with a new empty object (while
ignoring the disabled object which is being handled by another thread).
This counter always reaches zero before @cleaners does.
>
>> + */
>> + refcount_t usage;
>> + /*
>> + * @cleaners: Usage counter used to free a rule from @rules (thanks to
>> + * put_rule()). Enables to get a reference to this object until it
>> + * really become freed. Cf. put_object().
>
> Maybe add: @usage being non-zero counts as one reference to @cleaners.
> Once @cleaners has become zero, the object is freed after an RCU grace
> period.
What about this?
This counter can only reach zero if the @usage counter already reached
zero. Indeed, @usage being non-zero counts as one reference to
@cleaners. Once @cleaners has become zero, the object is freed after an
RCU grace period. This enables concurrent threads to safely get an
object reference to terminate it if there is no more concurrent cleaners
for this object. This mechanism is required to enable concurrent threads
to safely dereference an object from potentially different pointers
(e.g. the underlying object, or a rule tied to this object), to
potentially terminate and free it (i.e. if there is no more rules tied
to it, or if the underlying object is being terminated).
>
>> + */
>> + refcount_t cleaners;
>> + union {
>> + /*
>> + * The use of this struct is controlled by @usage and
>> + * @cleaners, which makes it safe to union it with @rcu_free.
>> + */
> [...]
>> + struct rcu_head rcu_free;
>> + };
>> +};
> [...]
>> +static inline bool landlock_rule_is_disabled(
>> + struct landlock_rule *rule)
>> +{
>> + /*
>> + * Disabling (i.e. unlinking) a landlock_rule is a one-way operation.
>> + * It is not possible to re-enable such a rule, then there is no need
>> + * for smp_load_acquire().
>> + *
>> + * LIST_POISON2 is set by list_del() and list_del_rcu().
>> + */
>> + return !rule || READ_ONCE(rule->list.prev) == LIST_POISON2;
>
> You're not allowed to do this, the comment above list_del() states:
>
> * Note: list_empty() on entry does not return true after this, the entry is
> * in an undefined state.
list_del() checks READ_ONCE(head->next) == head, but
landlock_rule_is_disabled() checks READ_ONCE(rule->list.prev) ==
LIST_POISON2.
The comment about LIST_POISON2 is right but may be misleading. There is
no use of list_empty() with a landlock_rule->list, only
landlock_object->rules. The only list_del() is in landlock_put_rule()
when there is a guarantee that there is no other reference to it, hence
no possible use of landlock_rule_is_disabled() with this rule. I could
replace it with a call to list_del_rcu() to make it more consistent.
>
> If you want to be able to test whether the element is on a list
> afterwards, use stuff like list_del_init().
There is no need to re-initialize the list but using list_del_init() and
list_empty() could work too. However, there is no list_del_init_rcu()
helper. Moreover, resetting the list's pointer with LIST_POISON2 might
help to detect bugs.
Thanks for this review!
^ permalink raw reply related
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