* [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-01-24 0:23 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20200124002306.3552-1-casey@schaufler-ca.com>
Add an entry /proc/.../attr/context which displays the full
process security "context" in compound format:'
lsm1\0value\0lsm2\0value\0...
This entry is not writable.
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Cc: linux-api@vger.kernel.org
---
Documentation/security/lsm.rst | 14 ++++++++
fs/proc/base.c | 1 +
security/security.c | 63 ++++++++++++++++++++++++++++++++++
3 files changed, 78 insertions(+)
diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
index aadf47c808c0..a4979060f5d3 100644
--- a/Documentation/security/lsm.rst
+++ b/Documentation/security/lsm.rst
@@ -199,3 +199,17 @@ capability-related fields:
- ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
- ``fs/proc/array.c``::c:func:`task_cap()`
+
+LSM External Interfaces
+=======================
+
+The LSM infrastructure does not generally provide external interfaces.
+The individual security modules provide what external interfaces they
+require. The infrastructure does provide an interface for the special
+case where multiple security modules provide a process context. This
+is provided in compound context format.
+
+- `lsm1\0value\0lsm2\0value\0`
+
+The special file ``/proc/pid/attr/context`` provides the security
+context of the identified process.
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 950c200cb9ad..d13c2cf50e4b 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
ATTR(NULL, "keycreate", 0666),
ATTR(NULL, "sockcreate", 0666),
ATTR(NULL, "display", 0666),
+ ATTR(NULL, "context", 0666),
#ifdef CONFIG_SECURITY_SMACK
DIR("smack", 0555,
proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
diff --git a/security/security.c b/security/security.c
index 6a77c8b2ffbc..fdd0c85df89e 100644
--- a/security/security.c
+++ b/security/security.c
@@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task)
panic("%s: Early task alloc failed.\n", __func__);
}
+/**
+ * append_ctx - append a lsm/context pair to a compound context
+ * @ctx: the existing compound context
+ * @ctxlen: size of the old context, including terminating nul byte
+ * @lsm: new lsm name, nul terminated
+ * @new: new context, possibly nul terminated
+ * @newlen: maximum size of @new
+ *
+ * replace @ctx with a new compound context, appending @newlsm and @new
+ * to @ctx. On exit the new data replaces the old, which is freed.
+ * @ctxlen is set to the new size, which includes a trailing nul byte.
+ *
+ * Returns 0 on success, -ENOMEM if no memory is available.
+ */
+static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
+ int newlen)
+{
+ char *final;
+ int llen;
+
+ llen = strlen(lsm) + 1;
+ newlen = strnlen(new, newlen) + 1;
+
+ final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
+ if (final == NULL)
+ return -ENOMEM;
+ if (*ctxlen)
+ memcpy(final, *ctx, *ctxlen);
+ memcpy(final + *ctxlen, lsm, llen);
+ memcpy(final + *ctxlen + llen, new, newlen);
+ kfree(*ctx);
+ *ctx = final;
+ *ctxlen = *ctxlen + llen + newlen;
+ return 0;
+}
+
/*
* Hook list operation macros.
*
@@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
char **value)
{
struct security_hook_list *hp;
+ char *final = NULL;
+ char *cp;
+ int rc = 0;
+ int finallen = 0;
int display = lsm_task_display(current);
int slot = 0;
@@ -2068,6 +2108,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
return -ENOMEM;
}
+ if (!strcmp(name, "context")) {
+ hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
+ list) {
+ rc = hp->hook.getprocattr(p, "current", &cp);
+ if (rc == -EINVAL || rc == -ENOPROTOOPT)
+ continue;
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
+ cp, rc);
+ if (rc < 0) {
+ kfree(final);
+ return rc;
+ }
+ }
+ if (final == NULL)
+ return -EINVAL;
+ *value = final;
+ return finallen;
+ }
+
hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
continue;
--
2.24.1
^ permalink raw reply related
* [PATCH v14 23/23] AppArmor: Remove the exclusive flag
From: Casey Schaufler @ 2020-01-24 0:23 UTC (permalink / raw)
To: casey.schaufler, jmorris, linux-security-module, selinux
Cc: casey, keescook, john.johansen, penguin-kernel, paul, sds
In-Reply-To: <20200124002306.3552-1-casey@schaufler-ca.com>
With the inclusion of the "display" process attribute
mechanism AppArmor no longer needs to be treated as an
"exclusive" security module. Remove the flag that indicates
it is exclusive. Remove the stub getpeersec_dgram AppArmor
hook as it has no effect in the single LSM case and
interferes in the multiple LSM case.
Reviewed-by: Kees Cook <keescook@chromium.org>
Reviewed-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
security/apparmor/lsm.c | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 16b992235c11..37d003568e82 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -1120,22 +1120,6 @@ static int apparmor_socket_getpeersec_stream(struct socket *sock,
return error;
}
-/**
- * apparmor_socket_getpeersec_dgram - get security label of packet
- * @sock: the peer socket
- * @skb: packet data
- * @secid: pointer to where to put the secid of the packet
- *
- * Sets the netlabel socket state on sk from parent
- */
-static int apparmor_socket_getpeersec_dgram(struct socket *sock,
- struct sk_buff *skb, u32 *secid)
-
-{
- /* TODO: requires secid support */
- return -ENOPROTOOPT;
-}
-
/**
* apparmor_sock_graft - Initialize newly created socket
* @sk: child sock
@@ -1239,8 +1223,6 @@ static struct security_hook_list apparmor_hooks[] __lsm_ro_after_init = {
#endif
LSM_HOOK_INIT(socket_getpeersec_stream,
apparmor_socket_getpeersec_stream),
- LSM_HOOK_INIT(socket_getpeersec_dgram,
- apparmor_socket_getpeersec_dgram),
LSM_HOOK_INIT(sock_graft, apparmor_sock_graft),
#ifdef CONFIG_NETWORK_SECMARK
LSM_HOOK_INIT(inet_conn_request, apparmor_inet_conn_request),
@@ -1909,7 +1891,7 @@ static int __init apparmor_init(void)
DEFINE_LSM(apparmor) = {
.name = "apparmor",
- .flags = LSM_FLAG_LEGACY_MAJOR | LSM_FLAG_EXCLUSIVE,
+ .flags = LSM_FLAG_LEGACY_MAJOR,
.enabled = &apparmor_enabled,
.blobs = &apparmor_blob_sizes,
.init = apparmor_init,
--
2.24.1
^ permalink raw reply related
* Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: KP Singh @ 2020-01-24 1:25 UTC (permalink / raw)
To: Casey Schaufler; +Cc: LKML, Linux Security Module list, bpf
In-Reply-To: <f571b719-e11f-416e-4232-f99036e38f15@schaufler-ca.com>
On 23-Jan 15:50, Casey Schaufler wrote:
> On 1/23/2020 2:24 PM, KP Singh wrote:
> > On 23-Jan 11:09, Casey Schaufler wrote:
> >> On 1/23/2020 9:59 AM, KP Singh wrote:
> >>> On 23-Jan 09:03, Casey Schaufler wrote:
> >>>> On 1/23/2020 7:24 AM, KP Singh wrote:
> >>>>> From: KP Singh <kpsingh@google.com>
> >>>>>
> >>>>> - The list of hooks registered by an LSM is currently immutable as they
> >>>>> are declared with __lsm_ro_after_init and they are attached to a
> >>>>> security_hook_heads struct.
> >>>>> - For the BPF LSM we need to de/register the hooks at runtime. Making
> >>>>> the existing security_hook_heads mutable broadens an
> >>>>> attack vector, so a separate security_hook_heads is added for only
> >>>>> those that ~must~ be mutable.
> >>>>> - These mutable hooks are run only after all the static hooks have
> >>>>> successfully executed.
> >>>>>
> >>>>> This is based on the ideas discussed in:
> >>>>>
> >>>>> https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> >>>>>
> >>>>> Reviewed-by: Brendan Jackman <jackmanb@google.com>
> >>>>> Reviewed-by: Florent Revest <revest@google.com>
> >>>>> Reviewed-by: Thomas Garnier <thgarnie@google.com>
> >>>>> Signed-off-by: KP Singh <kpsingh@google.com>
> >>>>> ---
> >>>>> MAINTAINERS | 1 +
> >>>>> include/linux/bpf_lsm.h | 72 +++++++++++++++++++++++++++++++++++++++++
> >>>>> security/bpf/Kconfig | 1 +
> >>>>> security/bpf/Makefile | 2 +-
> >>>>> security/bpf/hooks.c | 20 ++++++++++++
> >>>>> security/bpf/lsm.c | 7 ++++
> >>>>> security/security.c | 25 +++++++-------
> >>>>> 7 files changed, 116 insertions(+), 12 deletions(-)
> >>>>> create mode 100644 include/linux/bpf_lsm.h
> >>>>> create mode 100644 security/bpf/hooks.c
> >>>>>
> >>>>> diff --git a/MAINTAINERS b/MAINTAINERS
> >>>>> index e2b7f76a1a70..c606b3d89992 100644
> >>>>> --- a/MAINTAINERS
> >>>>> +++ b/MAINTAINERS
> >>>>> @@ -3209,6 +3209,7 @@ L: linux-security-module@vger.kernel.org
> >>>>> L: bpf@vger.kernel.org
> >>>>> S: Maintained
> >>>>> F: security/bpf/
> >>>>> +F: include/linux/bpf_lsm.h
> >>>>>
> >>>>> BROADCOM B44 10/100 ETHERNET DRIVER
> >>>>> M: Michael Chan <michael.chan@broadcom.com>
> >>>>> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> >>>>> new file mode 100644
> >>>>> index 000000000000..57c20b2cd2f4
> >>>>> --- /dev/null
> >>>>> +++ b/include/linux/bpf_lsm.h
> >>>>> @@ -0,0 +1,72 @@
> >>>>> +/* SPDX-License-Identifier: GPL-2.0 */
> >>>>> +
> >>>>> +/*
> >>>>> + * Copyright 2019 Google LLC.
> >>>>> + */
> >>>>> +
> >>>>> +#ifndef _LINUX_BPF_LSM_H
> >>>>> +#define _LINUX_BPF_LSM_H
> >>>>> +
> >>>>> +#include <linux/bpf.h>
> >>>>> +#include <linux/lsm_hooks.h>
> >>>>> +
> >>>>> +#ifdef CONFIG_SECURITY_BPF
> >>>>> +
> >>>>> +/* Mutable hooks defined at runtime and executed after all the statically
> >>>>> + * defined LSM hooks.
> >>>>> + */
> >>>>> +extern struct security_hook_heads bpf_lsm_hook_heads;
> >>>>> +
> >>>>> +int bpf_lsm_srcu_read_lock(void);
> >>>>> +void bpf_lsm_srcu_read_unlock(int idx);
> >>>>> +
> >>>>> +#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...) \
> >>>>> + do { \
> >>>>> + struct security_hook_list *P; \
> >>>>> + int _idx; \
> >>>>> + \
> >>>>> + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> >>>>> + break; \
> >>>>> + \
> >>>>> + _idx = bpf_lsm_srcu_read_lock(); \
> >>>>> + hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \
> >>>>> + P->hook.FUNC(__VA_ARGS__); \
> >>>>> + bpf_lsm_srcu_read_unlock(_idx); \
> >>>>> + } while (0)
> >>>>> +
> >>>>> +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) ({ \
> >>>>> + int _ret = 0; \
> >>>>> + do { \
> >>>>> + struct security_hook_list *P; \
> >>>>> + int _idx; \
> >>>>> + \
> >>>>> + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> >>>>> + break; \
> >>>>> + \
> >>>>> + _idx = bpf_lsm_srcu_read_lock(); \
> >>>>> + \
> >>>>> + hlist_for_each_entry(P, \
> >>>>> + &bpf_lsm_hook_heads.FUNC, list) { \
> >>>>> + _ret = P->hook.FUNC(__VA_ARGS__); \
> >>>>> + if (_ret && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> >>>>> + break; \
> >>>>> + } \
> >>>>> + bpf_lsm_srcu_read_unlock(_idx); \
> >>>>> + } while (0); \
> >>>>> + IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? _ret : 0; \
> >>>>> +})
> >>>>> +
> >>>>> +#else /* !CONFIG_SECURITY_BPF */
> >>>>> +
> >>>>> +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) (0)
> >>>>> +#define CALL_BPF_LSM_VOID_HOOKS(...)
> >>>>> +
> >>>>> +static inline int bpf_lsm_srcu_read_lock(void)
> >>>>> +{
> >>>>> + return 0;
> >>>>> +}
> >>>>> +static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> >>>>> +
> >>>>> +#endif /* CONFIG_SECURITY_BPF */
> >>>>> +
> >>>>> +#endif /* _LINUX_BPF_LSM_H */
> >>>>> diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> >>>>> index a5f6c67ae526..595e4ad597ae 100644
> >>>>> --- a/security/bpf/Kconfig
> >>>>> +++ b/security/bpf/Kconfig
> >>>>> @@ -6,6 +6,7 @@ config SECURITY_BPF
> >>>>> bool "BPF-based MAC and audit policy"
> >>>>> depends on SECURITY
> >>>>> depends on BPF_SYSCALL
> >>>>> + depends on SRCU
> >>>>> help
> >>>>> This enables instrumentation of the security hooks with
> >>>>> eBPF programs.
> >>>>> diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> >>>>> index c78a8a056e7e..c526927c337d 100644
> >>>>> --- a/security/bpf/Makefile
> >>>>> +++ b/security/bpf/Makefile
> >>>>> @@ -2,4 +2,4 @@
> >>>>> #
> >>>>> # Copyright 2019 Google LLC.
> >>>>>
> >>>>> -obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
> >>>>> +obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
> >>>>> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> >>>>> new file mode 100644
> >>>>> index 000000000000..b123d9cb4cd4
> >>>>> --- /dev/null
> >>>>> +++ b/security/bpf/hooks.c
> >>>>> @@ -0,0 +1,20 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0
> >>>>> +
> >>>>> +/*
> >>>>> + * Copyright 2019 Google LLC.
> >>>>> + */
> >>>>> +
> >>>>> +#include <linux/bpf_lsm.h>
> >>>>> +#include <linux/srcu.h>
> >>>>> +
> >>>>> +DEFINE_STATIC_SRCU(security_hook_srcu);
> >>>>> +
> >>>>> +int bpf_lsm_srcu_read_lock(void)
> >>>>> +{
> >>>>> + return srcu_read_lock(&security_hook_srcu);
> >>>>> +}
> >>>>> +
> >>>>> +void bpf_lsm_srcu_read_unlock(int idx)
> >>>>> +{
> >>>>> + return srcu_read_unlock(&security_hook_srcu, idx);
> >>>>> +}
> >>>>> diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> >>>>> index dc9ac03c7aa0..a25a068e1781 100644
> >>>>> --- a/security/bpf/lsm.c
> >>>>> +++ b/security/bpf/lsm.c
> >>>>> @@ -4,6 +4,7 @@
> >>>>> * Copyright 2019 Google LLC.
> >>>>> */
> >>>>>
> >>>>> +#include <linux/bpf_lsm.h>
> >>>>> #include <linux/lsm_hooks.h>
> >>>>>
> >>>>> /* This is only for internal hooks, always statically shipped as part of the
> >>>>> @@ -12,6 +13,12 @@
> >>>>> */
> >>>>> static struct security_hook_list bpf_lsm_hooks[] __lsm_ro_after_init = {};
> >>>>>
> >>>>> +/* Security hooks registered dynamically by the BPF LSM and must be accessed
> >>>>> + * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable
> >>>>> + * hooks dynamically allocated by the BPF LSM are appeneded here.
> >>>>> + */
> >>>>> +struct security_hook_heads bpf_lsm_hook_heads;
> >>>>> +
> >>>>> static int __init bpf_lsm_init(void)
> >>>>> {
> >>>>> security_add_hooks(bpf_lsm_hooks, ARRAY_SIZE(bpf_lsm_hooks), "bpf");
> >>>>> diff --git a/security/security.c b/security/security.c
> >>>>> index 30a8aa700557..95a46ca25dcd 100644
> >>>>> --- a/security/security.c
> >>>>> +++ b/security/security.c
> >>>>> @@ -27,6 +27,7 @@
> >>>>> #include <linux/backing-dev.h>
> >>>>> #include <linux/string.h>
> >>>>> #include <linux/msg.h>
> >>>>> +#include <linux/bpf_lsm.h>
> >>>>> #include <net/flow.h>
> >>>>>
> >>>>> #define MAX_LSM_EVM_XATTR 2
> >>>>> @@ -657,20 +658,22 @@ static void __init lsm_early_task(struct task_struct *task)
> >>>>> \
> >>>>> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> >>>>> P->hook.FUNC(__VA_ARGS__); \
> >>>>> + CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__); \
> >>>> I'm sorry if I wasn't clear on the v2 review.
> >>>> This does not belong in the infrastructure. You should be
> >>>> doing all the bpf_lsm hook processing in you module.
> >>>> bpf_lsm_task_alloc() should loop though all the bpf
> >>>> task_alloc hooks if they have to be handled differently
> >>>> from "normal" LSM hooks.
> >>> The BPF LSM does not define static hooks (the ones registered to
> >>> security_hook_heads in security.c with __lsm_ro_after_init) for each
> >>> LSM hook. If it tries to do that one ends with what was in v1:
> >>>
> >>> https://lore.kernel.org/bpf/20191220154208.15895-7-kpsingh@chromium.org
> >>>
> >>> This gets quite ugly (security/bpf/hooks.h from v1) and was noted by
> >>> the BPF maintainers:
> >>>
> >>> https://lore.kernel.org/bpf/20191222012722.gdqhppxpfmqfqbld@ast-mbp.dhcp.thefacebook.com/
> >>>
> >>> As I mentioned, some of the ideas we used here are based on:
> >>>
> >>> https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> >>>
> >>> Which gave each LSM the ability to add mutable hooks at runtime. If
> >>> you prefer we can make this generic and allow the LSMs to register
> >>> mutable hooks with the BPF LSM be the only LSM that uses it (and
> >>> enforce it with a whitelist).
> >>>
> >>> Would this generic approach be something you would consider better
> >>> than just calling the BPF mutable hooks directly?
> >> What I think makes sense is for the BPF LSM to have a hook
> >> for each of the interfaces and for that hook to handle the
> >> mutable list for the interface. If BPF not included there
> >> will be no mutable hooks.
> >>
> >> Yes, your v1 got this right.
> > BPF LSM does provide mutable LSM hooks and it ends up being simpler
> > to implement/maintain when they are treated as such.
>
> If you want to put mutable hook handling in the infrastructure
> you need to make it general mutable hook handling as opposed to
> BPF hook handling. I don't know if that would be acceptable for
> all the reasons called out about dynamic module loading.
We can have generic mutable hook handling and if an LSM doesn't
provide a mutable security_hook_heads, it would not allow dynamic
hooks / dynamic module loading.
So, in practice it will just be the BPF LSM that allows mutable hooks
and the other existing LSMs won't. I guess it will be cleaner than
calling the BPF hooks directly from the LSM code (i.e in security.c)
- KP
>
> >
> > The other approaches which we have considered are:
> >
> > - Using macro magic to allocate static hook bodies which call eBPF
> > programs as implemented in v1. This entails maintaining a
> > separate list of LSM hooks in the BPF LSM which is evident from the
> > giant security/bpf/include/hooks.h in:
> >
> > https://lore.kernel.org/bpf/20191220154208.15895-7-kpsingh@chromium.org
>
> I haven't put much though into how you might make that cleaner,
> but I don't see how you can expect any approach to turn out
> smaller than or less ugly than security.c.
>
> >
> > - Another approach one can think of is to allocate all the trampoline
> > images (one page each) at __init and update these images to invoke
> > BPF programs when they are attached.
> >
> > Both these approaches seem to suffer from the downside of doing more
> > work when it's not really needed (i.e. doing prep work for hooks which
> > have no eBPF programs attached) and they appear to to mask the fact
> > that what the BPF LSM provides is actually mutable LSM hooks by
> > allocating static wrappers around mutable callbacks.
>
> That's a "feature" of the LSM infrastructure. If you're not using a hook
> you just don't provide one. It is a artifact of your intent of providing
> a general extension that requires you provide a hook which may do nothing
> for every interface.
>
> >
> > Are there other downsides apart from the fact we have an explicit call
> > to the mutable hooks in the LSM code? (Note that we want to have these
> > mutable hooks run after all the static LSM hooks so ordering
> > would still end up being LSM_ORDER_LAST)
>
> My intention when I suggested using LSM_ORDER_LAST was to
> remove the explicit calls to BPF in the infrastructure.
>
> >
> > It would be great to hear the maintainers' perspective based on the
> > trade-offs involved with the different approaches discussed.
>
> Please bear in mind that the maintainer (James Morris) didn't
> develop the hook list scheme.
>
> > We are happy to adapt our approach based on the consensus we reach
> > here.
> >
> > - KP
> >
> >>> - KP
> >>>
> >>>>> } while (0)
> >>>>>
> >>>>> -#define call_int_hook(FUNC, IRC, ...) ({ \
> >>>>> - int RC = IRC; \
> >>>>> - do { \
> >>>>> - struct security_hook_list *P; \
> >>>>> - \
> >>>>> +#define call_int_hook(FUNC, IRC, ...) ({ \
> >>>>> + int RC = IRC; \
> >>>>> + do { \
> >>>>> + struct security_hook_list *P; \
> >>>>> hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> >>>>> - RC = P->hook.FUNC(__VA_ARGS__); \
> >>>>> - if (RC != 0) \
> >>>>> - break; \
> >>>>> - } \
> >>>>> - } while (0); \
> >>>>> - RC; \
> >>>>> + RC = P->hook.FUNC(__VA_ARGS__); \
> >>>>> + if (RC != 0) \
> >>>>> + break; \
> >>>>> + } \
> >>>>> + if (RC == 0) \
> >>>>> + RC = CALL_BPF_LSM_INT_HOOKS(FUNC, __VA_ARGS__); \
> >>>>> + } while (0); \
> >>>>> + RC; \
> >>>>> })
> >>>>>
> >>>>> /* Security operations */
>
^ permalink raw reply
* [PATCH 0/1] keys: seq_file .next functions should increase position index
From: Vasily Averin @ 2020-01-24 6:25 UTC (permalink / raw)
To: keyrings, linux-security-module
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn
In Aug 2018 NeilBrown noticed
commit 1f4aace60b0e ("fs/seq_file.c: simplify seq_file iteration code and interface")
"Some ->next functions do not increment *pos when they return NULL...
Note that such ->next functions are buggy and should be fixed.
A simple demonstration is
dd if=/proc/swaps bs=1000 skip=1
Choose any block size larger than the size of /proc/swaps. This will
always show the whole last line of /proc/swaps"
Described problem is still actual. If you make lseek into middle of last output line
following read will output end of last line and whole last line once again.
$ dd if=/proc/swaps bs=1 # usual output
Filename Type Size Used Priority
/dev/dm-0 partition 4194812 97536 -2
104+0 records in
104+0 records out
104 bytes copied
$ dd if=/proc/swaps bs=40 skip=1 # last line was generated twice
dd: /proc/swaps: cannot skip to specified offset
v/dm-0 partition 4194812 97536 -2
/dev/dm-0 partition 4194812 97536 -2
3+1 records in
3+1 records out
131 bytes copied
There are lot of other affected files, I've found 30+ including
/proc/net/ip_tables_matches and /proc/sysvipc/*
Following patch fixes the problem in keys-related file.
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Vasily Averin (1):
proc_keys_next should increase position index
security/keys/proc.c | 2 ++
1 file changed, 2 insertions(+)
--
1.8.3.1
^ permalink raw reply
* [PATCH 1/1] proc_keys_next should increase position index
From: Vasily Averin @ 2020-01-24 6:25 UTC (permalink / raw)
To: keyrings, linux-security-module
Cc: David Howells, Jarkko Sakkinen, James Morris, Serge E. Hallyn
if seq_file .next fuction does not change position index,
read after some lseek can generate unexpected output.
https://bugzilla.kernel.org/show_bug.cgi?id=206283
Signed-off-by: Vasily Averin <vvs@virtuozzo.com>
---
security/keys/proc.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/security/keys/proc.c b/security/keys/proc.c
index 415f3f1..d0cde66 100644
--- a/security/keys/proc.c
+++ b/security/keys/proc.c
@@ -139,6 +139,8 @@ static void *proc_keys_next(struct seq_file *p, void *v, loff_t *_pos)
n = key_serial_next(p, v);
if (n)
*_pos = key_node_serial(n);
+ else
+ (*_pos)++;
return n;
}
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH bpf-next v3 01/10] bpf: btf: Add btf_type_by_name_kind
From: KP Singh @ 2020-01-24 14:12 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <CAEf4BzbBAM1+E3j6pBaLjBwnvOVKV=oWrnANONEm8SCoGj=ZbQ@mail.gmail.com>
On 23-Jan 12:06, Andrii Nakryiko wrote:
> On Thu, Jan 23, 2020 at 7:25 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > - The LSM code does the combination of btf_find_by_name_kind and
> > btf_type_by_id a couple of times to figure out the BTF type for
> > security_hook_heads and security_list_options.
> > - Add an extern for btf_vmlinux in btf.h
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > Reviewed-by: Brendan Jackman <jackmanb@google.com>
> > Reviewed-by: Florent Revest <revest@google.com>
> > Reviewed-by: Thomas Garnier <thgarnie@google.com>
> > ---
> > include/linux/btf.h | 3 +++
> > kernel/bpf/btf.c | 12 ++++++++++++
> > 2 files changed, 15 insertions(+)
> >
> > diff --git a/include/linux/btf.h b/include/linux/btf.h
> > index 5c1ea99b480f..d4e859f90a39 100644
> > --- a/include/linux/btf.h
> > +++ b/include/linux/btf.h
> > @@ -15,6 +15,7 @@ struct btf_type;
> > union bpf_attr;
> >
> > extern const struct file_operations btf_fops;
> > +extern struct btf *btf_vmlinux;
> >
> > void btf_put(struct btf *btf);
> > int btf_new_fd(const union bpf_attr *attr);
> > @@ -66,6 +67,8 @@ const struct btf_type *
> > btf_resolve_size(const struct btf *btf, const struct btf_type *type,
> > u32 *type_size, const struct btf_type **elem_type,
> > u32 *total_nelems);
> > +const struct btf_type *btf_type_by_name_kind(
> > + struct btf *btf, const char *name, u8 kind);
> >
> > #define for_each_member(i, struct_type, member) \
> > for (i = 0, member = btf_type_member(struct_type); \
> > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > index 32963b6d5a9c..ea53c16802cb 100644
> > --- a/kernel/bpf/btf.c
> > +++ b/kernel/bpf/btf.c
> > @@ -441,6 +441,18 @@ const struct btf_type *btf_type_resolve_func_ptr(const struct btf *btf,
> > return NULL;
> > }
> >
> > +const struct btf_type *btf_type_by_name_kind(
> > + struct btf *btf, const char *name, u8 kind)
> > +{
> > + s32 type_id;
> > +
> > + type_id = btf_find_by_name_kind(btf, name, kind);
> > + if (type_id < 0)
> > + return ERR_PTR(type_id);
> > +
> > + return btf_type_by_id(btf, type_id);
> > +}
> > +
>
> is it worth having this as a separate global function? If
> btf_find_by_name_kind returns valid ID, then you don't really need to
> check btf_type_by_id result, it is always going to be valid. So the
> pattern becomes:
Yeah you're right. We went from using this a few times in separate
functions to 2 times in the same function (based on the changes in
v2 -> v3)
I will drop this from the next revision.
> type_id = btf_find_by_name_kind(btf, name, kind);
> if (type_id < 0)
> goto handle_error;
> t = btf_type_by_id(btf, type_id);
> /* now just use t */
>
> which is not much more verbose than:
>
> t = btf_type_by_name_kind(btf, name, kind);
> if (IS_ERR(t))
> goto handle_error
> /* now use t */
>
Agreed.
- KP
>
> > /* Types that act only as a source, not sink or intermediate
> > * type when resolving.
> > */
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH bpf-next v3 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-24 14:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Brendan Jackman,
Florent Revest, Thomas Garnier, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <CAEf4BzZ7gmCTzxw4f=fp=j2_buBQ3rV8m3qWH8s-ySY6sGVPzw@mail.gmail.com>
> On Thu, Jan 23, 2020 at 7:25 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > attr->lsm_hook_idx
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > Reviewed-by: Brendan Jackman <jackmanb@google.com>
> > Reviewed-by: Florent Revest <revest@google.com>
> > Reviewed-by: Thomas Garnier <thgarnie@google.com>
> > ---
>
> Looks good, but see few nits below.
>
> Acked-by: Andrii Nakryiko <andriin@fb.com>
Thanks!
>
> > tools/lib/bpf/bpf.c | 6 ++-
> > tools/lib/bpf/bpf.h | 1 +
> > tools/lib/bpf/libbpf.c | 104 +++++++++++++++++++++++++++++++++++++--
> > tools/lib/bpf/libbpf.h | 4 ++
> > tools/lib/bpf/libbpf.map | 3 ++
> > 5 files changed, 114 insertions(+), 4 deletions(-)
> >
>
> [...]
>
> > @@ -5084,6 +5099,8 @@ __bpf_object__open(const char *path, const void *obj_buf, size_t obj_buf_sz,
> > if (prog->type != BPF_PROG_TYPE_UNSPEC)
> > continue;
> >
> > +
> > +
>
> why these extra lines?
Ah this might have crept in my latest rebase. Will remove these.
>
> > err = libbpf_prog_type_by_name(prog->section_name, &prog_type,
> > &attach_type);
> > if (err == -ESRCH)
> > @@ -6160,6 +6177,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog) \
> > } \
> >
> > BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
> > +BPF_PROG_TYPE_FNS(lsm, BPF_PROG_TYPE_LSM);
> > BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
> > BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
> > BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
> > @@ -6226,6 +6244,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> > struct bpf_program *prog);
> > static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > struct bpf_program *prog);
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > + struct bpf_program *prog);
> >
> > struct bpf_sec_def {
> > const char *sec;
> > @@ -6272,6 +6292,9 @@ static const struct bpf_sec_def section_defs[] = {
> > SEC_DEF("freplace/", EXT,
> > .is_attach_btf = true,
> > .attach_fn = attach_trace),
> > + SEC_DEF("lsm/", LSM,
> > + .expected_attach_type = BPF_LSM_MAC,
>
> curious, will there be non-MAC LSM programs? if yes, how they are
> going to be different and which prefix will we use then?
One can think BPF_LSM_AUDIT programs which will only be used to log
information from the LSM hooks and not enforce a policy. Currently,
one can sort of do that by disabling CONFIG_SECURITY_BPF_ENFORCE but
that's an all or none hammer.
>
> > + .attach_fn = attach_lsm),
> > BPF_PROG_SEC("xdp", BPF_PROG_TYPE_XDP),
> > BPF_PROG_SEC("perf_event", BPF_PROG_TYPE_PERF_EVENT),
> > BPF_PROG_SEC("lwt_in", BPF_PROG_TYPE_LWT_IN),
> > @@ -6533,6 +6556,44 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> > return -EINVAL;
> > }
> >
> > +static __s32 find_lsm_hook_idx(struct bpf_program *prog)
>
> nit: I'd stick to int for return result, we barely ever use __s32 in libbpf.c
Sure. Changed to int.
- KP
>
> [...]
^ permalink raw reply
* Re: [PATCH v14 06/23] Use lsmblob in security_secctx_to_secid
From: Stephen Smalley @ 2020-01-24 14:29 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-7-casey@schaufler-ca.com>
On 1/23/20 7:22 PM, Casey Schaufler wrote:
> Change security_secctx_to_secid() to fill in a lsmblob instead
> of a u32 secid. Multiple LSMs may be able to interpret the
> string, and this allows for setting whichever secid is
> appropriate. In some cases there is scaffolding where other
> interfaces have yet to be converted.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
I acked v13 of this patch and don't see any real change here, so:
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/linux/security.h | 5 +++--
> kernel/cred.c | 4 +---
> net/netfilter/nft_meta.c | 12 +++++++-----
> net/netfilter/xt_SECMARK.c | 5 ++++-
> net/netlabel/netlabel_unlabeled.c | 14 ++++++++------
> security/security.c | 18 +++++++++++++++---
> 6 files changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index e53ff7eebcc5..5ad7cc8abd55 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -496,7 +496,8 @@ int security_setprocattr(const char *lsm, const char *name, void *value,
> int security_netlink_send(struct sock *sk, struct sk_buff *skb);
> int security_ismaclabel(const char *name);
> int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen);
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid);
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob);
> void security_release_secctx(char *secdata, u32 seclen);
> void security_inode_invalidate_secctx(struct inode *inode);
> int security_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen);
> @@ -1302,7 +1303,7 @@ static inline int security_secid_to_secctx(u32 secid, char **secdata, u32 *secle
>
> static inline int security_secctx_to_secid(const char *secdata,
> u32 seclen,
> - u32 *secid)
> + struct lsmblob *blob)
> {
> return -EOPNOTSUPP;
> }
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 490d72825aa5..b41c2bbd357f 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -756,14 +756,12 @@ EXPORT_SYMBOL(set_security_override);
> int set_security_override_from_ctx(struct cred *new, const char *secctx)
> {
> struct lsmblob blob;
> - u32 secid;
> int ret;
>
> - ret = security_secctx_to_secid(secctx, strlen(secctx), &secid);
> + ret = security_secctx_to_secid(secctx, strlen(secctx), &blob);
> if (ret < 0)
> return ret;
>
> - lsmblob_init(&blob, secid);
> return set_security_override(new, &blob);
> }
> EXPORT_SYMBOL(set_security_override_from_ctx);
> diff --git a/net/netfilter/nft_meta.c b/net/netfilter/nft_meta.c
> index 9740b554fdb3..259c78d2f371 100644
> --- a/net/netfilter/nft_meta.c
> +++ b/net/netfilter/nft_meta.c
> @@ -625,21 +625,23 @@ static const struct nla_policy nft_secmark_policy[NFTA_SECMARK_MAX + 1] = {
>
> static int nft_secmark_compute_secid(struct nft_secmark *priv)
> {
> - u32 tmp_secid = 0;
> + struct lsmblob blob;
> int err;
>
> - err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &tmp_secid);
> + err = security_secctx_to_secid(priv->ctx, strlen(priv->ctx), &blob);
> if (err)
> return err;
>
> - if (!tmp_secid)
> + if (!lsmblob_is_set(&blob))
> return -ENOENT;
>
> - err = security_secmark_relabel_packet(tmp_secid);
> + /* Using le[0] is scaffolding */
> + err = security_secmark_relabel_packet(blob.secid[0]);
> if (err)
> return err;
>
> - priv->secid = tmp_secid;
> + /* Using le[0] is scaffolding */
> + priv->secid = blob.secid[0];
> return 0;
> }
>
> diff --git a/net/netfilter/xt_SECMARK.c b/net/netfilter/xt_SECMARK.c
> index 2317721f3ecb..2d68416b4552 100644
> --- a/net/netfilter/xt_SECMARK.c
> +++ b/net/netfilter/xt_SECMARK.c
> @@ -45,13 +45,14 @@ secmark_tg(struct sk_buff *skb, const struct xt_action_param *par)
>
> static int checkentry_lsm(struct xt_secmark_target_info *info)
> {
> + struct lsmblob blob;
> int err;
>
> info->secctx[SECMARK_SECCTX_MAX - 1] = '\0';
> info->secid = 0;
>
> err = security_secctx_to_secid(info->secctx, strlen(info->secctx),
> - &info->secid);
> + &blob);
> if (err) {
> if (err == -EINVAL)
> pr_info_ratelimited("invalid security context \'%s\'\n",
> @@ -59,6 +60,8 @@ static int checkentry_lsm(struct xt_secmark_target_info *info)
> return err;
> }
>
> + /* scaffolding during the transition */
> + info->secid = blob.secid[0];
> if (!info->secid) {
> pr_info_ratelimited("unable to map security context \'%s\'\n",
> info->secctx);
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index d2e4ab8d1cb1..7a5a87f15736 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -881,7 +881,7 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> void *addr;
> void *mask;
> u32 addr_len;
> - u32 secid;
> + struct lsmblob blob;
> struct netlbl_audit audit_info;
>
> /* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -905,12 +905,13 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> ret_val = security_secctx_to_secid(
> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> - &secid);
> + &blob);
> if (ret_val != 0)
> return ret_val;
>
> + /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, secid,
> + dev_name, addr, mask, addr_len, blob.secid[0],
> &audit_info);
> }
>
> @@ -932,7 +933,7 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> void *addr;
> void *mask;
> u32 addr_len;
> - u32 secid;
> + struct lsmblob blob;
> struct netlbl_audit audit_info;
>
> /* Don't allow users to add both IPv4 and IPv6 addresses for a
> @@ -954,12 +955,13 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> ret_val = security_secctx_to_secid(
> nla_data(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> nla_len(info->attrs[NLBL_UNLABEL_A_SECCTX]),
> - &secid);
> + &blob);
> if (ret_val != 0)
> return ret_val;
>
> + /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, secid,
> + NULL, addr, mask, addr_len, blob.secid[0],
> &audit_info);
> }
>
> diff --git a/security/security.c b/security/security.c
> index b7404bfc8938..c722cd495517 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1970,10 +1970,22 @@ int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen)
> }
> EXPORT_SYMBOL(security_secid_to_secctx);
>
> -int security_secctx_to_secid(const char *secdata, u32 seclen, u32 *secid)
> +int security_secctx_to_secid(const char *secdata, u32 seclen,
> + struct lsmblob *blob)
> {
> - *secid = 0;
> - return call_int_hook(secctx_to_secid, 0, secdata, seclen, secid);
> + struct security_hook_list *hp;
> + int rc;
> +
> + lsmblob_init(blob, 0);
> + hlist_for_each_entry(hp, &security_hook_heads.secctx_to_secid, list) {
> + if (WARN_ON(hp->lsmid->slot < 0 || hp->lsmid->slot >= lsm_slot))
> + continue;
> + rc = hp->hook.secctx_to_secid(secdata, seclen,
> + &blob->secid[hp->lsmid->slot]);
> + if (rc != 0)
> + return rc;
> + }
> + return 0;
> }
> EXPORT_SYMBOL(security_secctx_to_secid);
>
>
^ permalink raw reply
* Re: [PATCH v14 02/23] LSM: Create and manage the lsmblob data structure.
From: Stephen Smalley @ 2020-01-24 14:21 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-3-casey@schaufler-ca.com>
On 1/23/20 7:22 PM, Casey Schaufler wrote:
> When more than one security module is exporting data to
> audit and networking sub-systems a single 32 bit integer
> is no longer sufficient to represent the data. Add a
> structure to be used instead.
>
> The lsmblob structure is currently an array of
> u32 "secids". There is an entry for each of the
> security modules built into the system that would
> use secids if active. The system assigns the module
> a "slot" when it registers hooks. If modules are
> compiled in but not registered there will be unused
> slots.
>
> A new lsm_id structure, which contains the name
> of the LSM and its slot number, is created. There
> is an instance for each LSM, which assigns the name
> and passes it to the infrastructure to set the slot.
>
> The audit rules data is expanded to use an array of
> security module data rather than a single instance.
> Because IMA uses the audit rule functions it is
> affected as well.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> ---
> diff --git a/security/lockdown/lockdown.c b/security/lockdown/lockdown.c
> index b2f87015d6e9..91662325e450 100644
> --- a/security/lockdown/lockdown.c
> +++ b/security/lockdown/lockdown.c
> @@ -102,6 +102,11 @@ static struct security_hook_list lockdown_hooks[] __lsm_ro_after_init = {
> LSM_HOOK_INIT(locked_down, lockdown_is_locked_down),
> };
>
> +static struct lsm_id lockdown_lsmid __lsm_ro_after_init = {
> + .lsm = "landlock",
> + .slot = LSMBLOB_NOT_NEEDED
> +};
> +
> static int __init lockdown_lsm_init(void)
> {
> #if defined(CONFIG_LOCK_DOWN_KERNEL_FORCE_INTEGRITY)
You really want landlock merged, don't you?
^ permalink raw reply
* Re: [PATCH v14 18/23] NET: Store LSM netlabel data in a lsmblob
From: Stephen Smalley @ 2020-01-24 14:36 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-19-casey@schaufler-ca.com>
On 1/23/20 7:23 PM, Casey Schaufler wrote:
> Netlabel uses LSM interfaces requiring an lsmblob and
> the internal storage is used to pass information between
> these interfaces, so change the internal data from a secid
> to a lsmblob. Update the netlabel interfaces and their
> callers to accommodate the change. This requires that the
> modules using netlabel use the lsm_id.slot to access the
> correct secid when using netlabel.
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Reviewed-by: John Johansen <john.johansen@canonical.com>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
Similarly, I acked v13 of this patch and didn't see any real change here.
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> include/net/netlabel.h | 8 ++--
> net/ipv4/cipso_ipv4.c | 23 +++++++-----
> net/netlabel/netlabel_kapi.c | 6 +--
> net/netlabel/netlabel_unlabeled.c | 57 +++++++++++------------------
> net/netlabel/netlabel_unlabeled.h | 2 +-
> security/selinux/hooks.c | 2 +-
> security/selinux/include/security.h | 1 +
> security/selinux/netlabel.c | 2 +-
> security/selinux/ss/services.c | 4 +-
> security/smack/smack.h | 1 +
> security/smack/smack_lsm.c | 5 ++-
> security/smack/smackfs.c | 10 +++--
> 12 files changed, 59 insertions(+), 62 deletions(-)
>
> diff --git a/include/net/netlabel.h b/include/net/netlabel.h
> index 43ae50337685..73fc25b4042b 100644
> --- a/include/net/netlabel.h
> +++ b/include/net/netlabel.h
> @@ -166,7 +166,7 @@ struct netlbl_lsm_catmap {
> * @attr.mls: MLS sensitivity label
> * @attr.mls.cat: MLS category bitmap
> * @attr.mls.lvl: MLS sensitivity level
> - * @attr.secid: LSM specific secid token
> + * @attr.lsmblob: LSM specific data
> *
> * Description:
> * This structure is used to pass security attributes between NetLabel and the
> @@ -201,7 +201,7 @@ struct netlbl_lsm_secattr {
> struct netlbl_lsm_catmap *cat;
> u32 lvl;
> } mls;
> - u32 secid;
> + struct lsmblob lsmblob;
> } attr;
> };
>
> @@ -415,7 +415,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_cfg_unlbl_static_del(struct net *net,
> const char *dev_name,
> @@ -523,7 +523,7 @@ static inline int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> return -ENOSYS;
> diff --git a/net/ipv4/cipso_ipv4.c b/net/ipv4/cipso_ipv4.c
> index 376882215919..adb9dffc3952 100644
> --- a/net/ipv4/cipso_ipv4.c
> +++ b/net/ipv4/cipso_ipv4.c
> @@ -106,15 +106,17 @@ int cipso_v4_rbm_strictvalid = 1;
> /* Base length of the local tag (non-standard tag).
> * Tag definition (may change between kernel versions)
> *
> - * 0 8 16 24 32
> - * +----------+----------+----------+----------+
> - * | 10000000 | 00000110 | 32-bit secid value |
> - * +----------+----------+----------+----------+
> - * | in (host byte order)|
> - * +----------+----------+
> - *
> + * 0 8 16 16 + sizeof(struct lsmblob)
> + * +----------+----------+---------------------+
> + * | 10000000 | 00000110 | LSM blob data |
> + * +----------+----------+---------------------+
> + *
> + * All secid and flag fields are in host byte order.
> + * The lsmblob structure size varies depending on which
> + * Linux security modules are built in the kernel.
> + * The data is opaque.
> */
> -#define CIPSO_V4_TAG_LOC_BLEN 6
> +#define CIPSO_V4_TAG_LOC_BLEN (2 + sizeof(struct lsmblob))
>
> /*
> * Helper Functions
> @@ -1467,7 +1469,8 @@ static int cipso_v4_gentag_loc(const struct cipso_v4_doi *doi_def,
>
> buffer[0] = CIPSO_V4_TAG_LOCAL;
> buffer[1] = CIPSO_V4_TAG_LOC_BLEN;
> - *(u32 *)&buffer[2] = secattr->attr.secid;
> + memcpy(&buffer[2], &secattr->attr.lsmblob,
> + sizeof(secattr->attr.lsmblob));
>
> return CIPSO_V4_TAG_LOC_BLEN;
> }
> @@ -1487,7 +1490,7 @@ static int cipso_v4_parsetag_loc(const struct cipso_v4_doi *doi_def,
> const unsigned char *tag,
> struct netlbl_lsm_secattr *secattr)
> {
> - secattr->attr.secid = *(u32 *)&tag[2];
> + memcpy(&secattr->attr.lsmblob, &tag[2], sizeof(secattr->attr.lsmblob));
> secattr->flags |= NETLBL_SECATTR_SECID;
>
> return 0;
> diff --git a/net/netlabel/netlabel_kapi.c b/net/netlabel/netlabel_kapi.c
> index 409a3ae47ce2..f2ebd43a7992 100644
> --- a/net/netlabel/netlabel_kapi.c
> +++ b/net/netlabel/netlabel_kapi.c
> @@ -196,7 +196,7 @@ int netlbl_cfg_unlbl_map_add(const char *domain,
> * @addr: IP address in network byte order (struct in[6]_addr)
> * @mask: address mask in network byte order (struct in[6]_addr)
> * @family: address family
> - * @secid: LSM secid value for the entry
> + * @lsmblob: LSM data value for the entry
> * @audit_info: NetLabel audit information
> *
> * Description:
> @@ -210,7 +210,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
> const void *addr,
> const void *mask,
> u16 family,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> u32 addr_len;
> @@ -230,7 +230,7 @@ int netlbl_cfg_unlbl_static_add(struct net *net,
>
> return netlbl_unlhsh_add(net,
> dev_name, addr, mask, addr_len,
> - secid, audit_info);
> + lsmblob, audit_info);
> }
>
> /**
> diff --git a/net/netlabel/netlabel_unlabeled.c b/net/netlabel/netlabel_unlabeled.c
> index c03fe9a4f7b9..3b0f07b59436 100644
> --- a/net/netlabel/netlabel_unlabeled.c
> +++ b/net/netlabel/netlabel_unlabeled.c
> @@ -66,7 +66,7 @@ struct netlbl_unlhsh_tbl {
> #define netlbl_unlhsh_addr4_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr4, list)
> struct netlbl_unlhsh_addr4 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af4list list;
> struct rcu_head rcu;
> @@ -74,7 +74,7 @@ struct netlbl_unlhsh_addr4 {
> #define netlbl_unlhsh_addr6_entry(iter) \
> container_of(iter, struct netlbl_unlhsh_addr6, list)
> struct netlbl_unlhsh_addr6 {
> - u32 secid;
> + struct lsmblob lsmblob;
>
> struct netlbl_af6list list;
> struct rcu_head rcu;
> @@ -219,7 +219,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> * @iface: the associated interface entry
> * @addr: IPv4 address in network byte order
> * @mask: IPv4 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -230,7 +230,7 @@ static struct netlbl_unlhsh_iface *netlbl_unlhsh_search_iface(int ifindex)
> static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> const struct in_addr *addr,
> const struct in_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr4 *entry;
> @@ -242,7 +242,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> entry->list.addr = addr->s_addr & mask->s_addr;
> entry->list.mask = mask->s_addr;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af4list_add(&entry->list, &iface->addr4_list);
> @@ -259,7 +259,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> * @iface: the associated interface entry
> * @addr: IPv6 address in network byte order
> * @mask: IPv6 address mask in network byte order
> - * @secid: LSM secid value for entry
> + * @lsmblob: LSM data value for entry
> *
> * Description:
> * Add a new address entry into the unlabeled connection hash table using the
> @@ -270,7 +270,7 @@ static int netlbl_unlhsh_add_addr4(struct netlbl_unlhsh_iface *iface,
> static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> const struct in6_addr *addr,
> const struct in6_addr *mask,
> - u32 secid)
> + struct lsmblob *lsmblob)
> {
> int ret_val;
> struct netlbl_unlhsh_addr6 *entry;
> @@ -286,7 +286,7 @@ static int netlbl_unlhsh_add_addr6(struct netlbl_unlhsh_iface *iface,
> entry->list.addr.s6_addr32[3] &= mask->s6_addr32[3];
> entry->list.mask = *mask;
> entry->list.valid = 1;
> - entry->secid = secid;
> + entry->lsmblob = *lsmblob;
>
> spin_lock(&netlbl_unlhsh_lock);
> ret_val = netlbl_af6list_add(&entry->list, &iface->addr6_list);
> @@ -365,7 +365,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info)
> {
> int ret_val;
> @@ -374,7 +374,6 @@ int netlbl_unlhsh_add(struct net *net,
> struct netlbl_unlhsh_iface *iface;
> struct audit_buffer *audit_buf = NULL;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> if (addr_len != sizeof(struct in_addr) &&
> addr_len != sizeof(struct in6_addr))
> @@ -407,7 +406,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in_addr *addr4 = addr;
> const struct in_addr *mask4 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, secid);
> + ret_val = netlbl_unlhsh_add_addr4(iface, addr4, mask4, lsmblob);
> if (audit_buf != NULL)
> netlbl_af4list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -420,7 +419,7 @@ int netlbl_unlhsh_add(struct net *net,
> const struct in6_addr *addr6 = addr;
> const struct in6_addr *mask6 = mask;
>
> - ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, secid);
> + ret_val = netlbl_unlhsh_add_addr6(iface, addr6, mask6, lsmblob);
> if (audit_buf != NULL)
> netlbl_af6list_audit_addr(audit_buf, 1,
> dev_name,
> @@ -437,8 +436,7 @@ int netlbl_unlhsh_add(struct net *net,
> unlhsh_add_return:
> rcu_read_unlock();
> if (audit_buf != NULL) {
> - lsmblob_init(&blob, secid);
> - if (security_secid_to_secctx(&blob, &context) == 0) {
> + if (security_secid_to_secctx(lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -473,7 +471,6 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af4list_remove(addr->s_addr, mask->s_addr,
> @@ -493,10 +490,8 @@ static int netlbl_unlhsh_remove_addr4(struct net *net,
> addr->s_addr, mask->s_addr);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -537,7 +532,6 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> struct audit_buffer *audit_buf;
> struct net_device *dev;
> struct lsmcontext context;
> - struct lsmblob blob;
>
> spin_lock(&netlbl_unlhsh_lock);
> list_entry = netlbl_af6list_remove(addr, mask, &iface->addr6_list);
> @@ -556,10 +550,8 @@ static int netlbl_unlhsh_remove_addr6(struct net *net,
> addr, mask);
> if (dev != NULL)
> dev_put(dev);
> - if (entry != NULL)
> - lsmblob_init(&blob, entry->secid);
> if (entry != NULL &&
> - security_secid_to_secctx(&blob, &context) == 0) {
> + security_secid_to_secctx(&entry->lsmblob, &context) == 0) {
> audit_log_format(audit_buf, " sec_obj=%s",
> context.context);
> security_release_secctx(&context);
> @@ -913,9 +905,8 @@ static int netlbl_unlabel_staticadd(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - dev_name, addr, mask, addr_len, blob.secid[0],
> + dev_name, addr, mask, addr_len, &blob,
> &audit_info);
> }
>
> @@ -963,10 +954,8 @@ static int netlbl_unlabel_staticadddef(struct sk_buff *skb,
> if (ret_val != 0)
> return ret_val;
>
> - /* scaffolding with the [0] */
> return netlbl_unlhsh_add(&init_net,
> - NULL, addr, mask, addr_len, blob.secid[0],
> - &audit_info);
> + NULL, addr, mask, addr_len, &blob, &audit_info);
> }
>
> /**
> @@ -1078,8 +1067,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> struct net_device *dev;
> struct lsmcontext context;
> void *data;
> - u32 secid;
> - struct lsmblob blob;
> + struct lsmblob *lsmb;
>
> data = genlmsg_put(cb_arg->skb, NETLINK_CB(cb_arg->nl_cb->skb).portid,
> cb_arg->seq, &netlbl_unlabel_gnl_family,
> @@ -1117,7 +1105,7 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr4->secid;
> + lsmb = (struct lsmblob *)&addr4->lsmblob;
> } else {
> ret_val = nla_put_in6_addr(cb_arg->skb,
> NLBL_UNLABEL_A_IPV6ADDR,
> @@ -1131,11 +1119,10 @@ static int netlbl_unlabel_staticlist_gen(u32 cmd,
> if (ret_val != 0)
> goto list_cb_failure;
>
> - secid = addr6->secid;
> + lsmb = (struct lsmblob *)&addr6->lsmblob;
> }
>
> - lsmblob_init(&blob, secid);
> - ret_val = security_secid_to_secctx(&blob, &context);
> + ret_val = security_secid_to_secctx(lsmb, &context);
> if (ret_val != 0)
> goto list_cb_failure;
> ret_val = nla_put(cb_arg->skb,
> @@ -1487,7 +1474,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr4_list);
> if (addr4 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr4_entry(addr4)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr4_entry(addr4)->lsmblob;
> break;
> }
> #if IS_ENABLED(CONFIG_IPV6)
> @@ -1500,7 +1487,7 @@ int netlbl_unlabel_getattr(const struct sk_buff *skb,
> &iface->addr6_list);
> if (addr6 == NULL)
> goto unlabel_getattr_nolabel;
> - secattr->attr.secid = netlbl_unlhsh_addr6_entry(addr6)->secid;
> + secattr->attr.lsmblob = netlbl_unlhsh_addr6_entry(addr6)->lsmblob;
> break;
> }
> #endif /* IPv6 */
> diff --git a/net/netlabel/netlabel_unlabeled.h b/net/netlabel/netlabel_unlabeled.h
> index 058e3a285d56..168920780994 100644
> --- a/net/netlabel/netlabel_unlabeled.h
> +++ b/net/netlabel/netlabel_unlabeled.h
> @@ -211,7 +211,7 @@ int netlbl_unlhsh_add(struct net *net,
> const void *addr,
> const void *mask,
> u32 addr_len,
> - u32 secid,
> + struct lsmblob *lsmblob,
> struct netlbl_audit *audit_info);
> int netlbl_unlhsh_remove(struct net *net,
> const char *dev_name,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index b8501ca3c8f3..cd4743331800 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -6871,7 +6871,7 @@ static int selinux_perf_event_write(struct perf_event *event)
> }
> #endif
>
> -static struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> +struct lsm_id selinux_lsmid __lsm_ro_after_init = {
> .lsm = "selinux",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
> index ae840634e3c7..741ba0e6dd83 100644
> --- a/security/selinux/include/security.h
> +++ b/security/selinux/include/security.h
> @@ -70,6 +70,7 @@
> struct netlbl_lsm_secattr;
>
> extern int selinux_enabled;
> +extern struct lsm_id selinux_lsmid;
>
> /* Policy capabilities */
> enum {
> diff --git a/security/selinux/netlabel.c b/security/selinux/netlabel.c
> index 6a94b31b5472..d8d7603ab14e 100644
> --- a/security/selinux/netlabel.c
> +++ b/security/selinux/netlabel.c
> @@ -108,7 +108,7 @@ static struct netlbl_lsm_secattr *selinux_netlbl_sock_getattr(
> return NULL;
>
> if ((secattr->flags & NETLBL_SECATTR_SECID) &&
> - (secattr->attr.secid == sid))
> + (secattr->attr.lsmblob.secid[selinux_lsmid.slot] == sid))
> return secattr;
>
> return NULL;
> diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> index a5813c7629c1..2b7680903b6b 100644
> --- a/security/selinux/ss/services.c
> +++ b/security/selinux/ss/services.c
> @@ -3599,7 +3599,7 @@ int security_netlbl_secattr_to_sid(struct selinux_state *state,
> if (secattr->flags & NETLBL_SECATTR_CACHE)
> *sid = *(u32 *)secattr->cache->data;
> else if (secattr->flags & NETLBL_SECATTR_SECID)
> - *sid = secattr->attr.secid;
> + *sid = secattr->attr.lsmblob.secid[selinux_lsmid.slot];
> else if (secattr->flags & NETLBL_SECATTR_MLS_LVL) {
> rc = -EIDRM;
> ctx = sidtab_search(sidtab, SECINITSID_NETMSG);
> @@ -3672,7 +3672,7 @@ int security_netlbl_sid_to_secattr(struct selinux_state *state,
> if (secattr->domain == NULL)
> goto out;
>
> - secattr->attr.secid = sid;
> + secattr->attr.lsmblob.secid[selinux_lsmid.slot] = sid;
> secattr->flags |= NETLBL_SECATTR_DOMAIN_CPY | NETLBL_SECATTR_SECID;
> mls_export_netlbl_lvl(policydb, ctx, secattr);
> rc = mls_export_netlbl_cat(policydb, ctx, secattr);
> diff --git a/security/smack/smack.h b/security/smack/smack.h
> index 2836540f9577..6e76b6b33063 100644
> --- a/security/smack/smack.h
> +++ b/security/smack/smack.h
> @@ -316,6 +316,7 @@ void smk_destroy_label_list(struct list_head *list);
> * Shared data.
> */
> extern int smack_enabled;
> +extern struct lsm_id smack_lsmid;
> extern int smack_cipso_direct;
> extern int smack_cipso_mapped;
> extern struct smack_known *smack_net_ambient;
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 9737ead06b39..9ce67e03ac49 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3775,7 +3775,8 @@ static struct smack_known *smack_from_secattr(struct netlbl_lsm_secattr *sap,
> /*
> * Looks like a fallback, which gives us a secid.
> */
> - return smack_from_secid(sap->attr.secid);
> + return smack_from_secid(
> + sap->attr.lsmblob.secid[smack_lsmid.slot]);
> /*
> * Without guidance regarding the smack value
> * for the packet fall back on the network
> @@ -4592,7 +4593,7 @@ struct lsm_blob_sizes smack_blob_sizes __lsm_ro_after_init = {
> .lbs_sock = sizeof(struct socket_smack),
> };
>
> -static struct lsm_id smack_lsmid __lsm_ro_after_init = {
> +struct lsm_id smack_lsmid __lsm_ro_after_init = {
> .lsm = "smack",
> .slot = LSMBLOB_NEEDED
> };
> diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
> index e3e05c04dbd1..d10e9c96717e 100644
> --- a/security/smack/smackfs.c
> +++ b/security/smack/smackfs.c
> @@ -1122,6 +1122,7 @@ static void smk_net4addr_insert(struct smk_net4addr *new)
> static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> {
> + struct lsmblob lsmblob;
> struct smk_net4addr *snp;
> struct sockaddr_in newname;
> char *smack;
> @@ -1253,10 +1254,13 @@ static ssize_t smk_write_net4addr(struct file *file, const char __user *buf,
> * this host so that incoming packets get labeled.
> * but only if we didn't get the special CIPSO option
> */
> - if (rc == 0 && skp != NULL)
> + if (rc == 0 && skp != NULL) {
> + lsmblob_init(&lsmblob, 0);
> + lsmblob.secid[smack_lsmid.slot] = snp->smk_label->smk_secid;
> rc = netlbl_cfg_unlbl_static_add(&init_net, NULL,
> - &snp->smk_host, &snp->smk_mask, PF_INET,
> - snp->smk_label->smk_secid, &audit_info);
> + &snp->smk_host, &snp->smk_mask, PF_INET, &lsmblob,
> + &audit_info);
> + }
>
> if (rc == 0)
> rc = count;
>
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-01-24 14:42 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-23-casey@schaufler-ca.com>
On 1/23/20 7:23 PM, Casey Schaufler wrote:
> Add an entry /proc/.../attr/context which displays the full
> process security "context" in compound format:'
> lsm1\0value\0lsm2\0value\0...
> This entry is not writable.
>
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> Cc: linux-api@vger.kernel.org
As previously discussed, there are issues with AppArmor's implementation
of getprocattr() particularly around the trailing newline that
dbus-daemon and perhaps others would like to see go away in any new
interface. Hence, I don't think we should implement this new API using
the existing getprocattr() hook lest it also be locked into the current
behavior forever.
> ---
> Documentation/security/lsm.rst | 14 ++++++++
> fs/proc/base.c | 1 +
> security/security.c | 63 ++++++++++++++++++++++++++++++++++
> 3 files changed, 78 insertions(+)
>
> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
> index aadf47c808c0..a4979060f5d3 100644
> --- a/Documentation/security/lsm.rst
> +++ b/Documentation/security/lsm.rst
> @@ -199,3 +199,17 @@ capability-related fields:
> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
>
> - ``fs/proc/array.c``::c:func:`task_cap()`
> +
> +LSM External Interfaces
> +=======================
> +
> +The LSM infrastructure does not generally provide external interfaces.
> +The individual security modules provide what external interfaces they
> +require. The infrastructure does provide an interface for the special
> +case where multiple security modules provide a process context. This
> +is provided in compound context format.
> +
> +- `lsm1\0value\0lsm2\0value\0`
> +
> +The special file ``/proc/pid/attr/context`` provides the security
> +context of the identified process.
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 950c200cb9ad..d13c2cf50e4b 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
> ATTR(NULL, "keycreate", 0666),
> ATTR(NULL, "sockcreate", 0666),
> ATTR(NULL, "display", 0666),
> + ATTR(NULL, "context", 0666),
> #ifdef CONFIG_SECURITY_SMACK
> DIR("smack", 0555,
> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
> diff --git a/security/security.c b/security/security.c
> index 6a77c8b2ffbc..fdd0c85df89e 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task)
> panic("%s: Early task alloc failed.\n", __func__);
> }
>
> +/**
> + * append_ctx - append a lsm/context pair to a compound context
> + * @ctx: the existing compound context
> + * @ctxlen: size of the old context, including terminating nul byte
> + * @lsm: new lsm name, nul terminated
> + * @new: new context, possibly nul terminated
> + * @newlen: maximum size of @new
> + *
> + * replace @ctx with a new compound context, appending @newlsm and @new
> + * to @ctx. On exit the new data replaces the old, which is freed.
> + * @ctxlen is set to the new size, which includes a trailing nul byte.
> + *
> + * Returns 0 on success, -ENOMEM if no memory is available.
> + */
> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
> + int newlen)
> +{
> + char *final;
> + int llen;
> +
> + llen = strlen(lsm) + 1;
> + newlen = strnlen(new, newlen) + 1;
> +
> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
> + if (final == NULL)
> + return -ENOMEM;
> + if (*ctxlen)
> + memcpy(final, *ctx, *ctxlen);
> + memcpy(final + *ctxlen, lsm, llen);
> + memcpy(final + *ctxlen + llen, new, newlen);
> + kfree(*ctx);
> + *ctx = final;
> + *ctxlen = *ctxlen + llen + newlen;
> + return 0;
> +}
> +
> /*
> * Hook list operation macros.
> *
> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> char **value)
> {
> struct security_hook_list *hp;
> + char *final = NULL;
> + char *cp;
> + int rc = 0;
> + int finallen = 0;
> int display = lsm_task_display(current);
> int slot = 0;
>
> @@ -2068,6 +2108,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
> return -ENOMEM;
> }
>
> + if (!strcmp(name, "context")) {
> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
> + list) {
> + rc = hp->hook.getprocattr(p, "current", &cp);
> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
> + continue;
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
> + cp, rc);
> + if (rc < 0) {
> + kfree(final);
> + return rc;
> + }
> + }
> + if (final == NULL)
> + return -EINVAL;
> + *value = final;
> + return finallen;
> + }
> +
> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
> continue;
>
^ permalink raw reply
* Re: [PATCH v2] ima: export the measurement list when needed
From: david.safford @ 2020-01-24 14:46 UTC (permalink / raw)
To: Janne Karhunen, linux-integrity, linux-security-module, zohar
Cc: kgold, Wiseman, Monty (GE Global Research, US), Mimi Zohar
In-Reply-To: <20200108111743.23393-1-janne.karhunen@gmail.com>
On Wed, 2020-01-08 at 13:17 +0200, Janne Karhunen wrote:
> Some systems can end up carrying lots of entries in the ima
> measurement list. Since every entry is using a bit of kernel
> memory, allow the sysadmin to export the measurement list to
> the filesystem to free up some memory.
>
> Signed-off-by: Janne Karhunen <janne.karhunen@gmail.com>
I like this approach, as it will work easily for measurement lists in
any format, and it will work for user or kernel triggering.
I'm getting an OOPS, though, whenever I write a filename to the
securityfs file (e.g. echo /var/log/ima.log > list_name).
Here's the relevant from syslog:
BUG: unable to handle page fault for address: 00005650a0e7fe30
#PF: supervisor read access in kernel mode
#PF: error_code(0x0001) - permissions violation
Oops: 0001 [#1] SMP NOPTI
RIP: 0010:ima_write_list_name+0x35/0x114
I haven't had time to debug this. Any suggestions?
Also, one embedded comment follows...
> ---
> security/integrity/ima/ima.h | 7 +-
> security/integrity/ima/ima_fs.c | 171 +++++++++++++++++++++++++++++
> security/integrity/ima/ima_queue.c | 2 +-
> 3 files changed, 175 insertions(+), 5 deletions(-)
>
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 19769bf5f6ab..78f0b706848d 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -151,20 +151,19 @@ int template_desc_init_fields(const char *template_fmt,
> struct ima_template_desc *ima_template_desc_current(void);
> struct ima_template_desc *lookup_template_desc(const char *name);
> bool ima_template_has_modsig(const struct ima_template_desc *ima_template);
> +void ima_free_template_entry(struct ima_template_entry *entry);
> int ima_restore_measurement_entry(struct ima_template_entry *entry);
> int ima_restore_measurement_list(loff_t bufsize, void *buf);
> int ima_measurements_show(struct seq_file *m, void *v);
> unsigned long ima_get_binary_runtime_size(void);
> int ima_init_template(void);
> void ima_init_template_list(void);
> +int ima_export_list(const char *from);
> int __init ima_init_digests(void);
> int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> void *lsm_data);
>
> -/*
> - * used to protect h_table and sha_table
> - */
> -extern spinlock_t ima_queue_lock;
> +extern struct mutex ima_extend_list_mutex;
>
> struct ima_h_table {
> atomic_long_t len; /* number of stored measurements in the list */
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 2000e8df0301..b60a241b0d8b 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -22,10 +22,17 @@
> #include <linux/rcupdate.h>
> #include <linux/parser.h>
> #include <linux/vmalloc.h>
> +#include <linux/fs_struct.h>
> +#include <linux/syscalls.h>
>
> #include "ima.h"
>
> +#define secfs_mnt "/sys/kernel/security"
> +#define am_filename "/integrity/ima/ascii_runtime_measurements"
You probably really want to export the binary data, as that's
what's needed for attestation. (Or both, but that's trickier.)
dave
> +
> static DEFINE_MUTEX(ima_write_mutex);
> +static DEFINE_MUTEX(ima_list_mutex);
> +static char *ima_msmt_list_name;
>
>
^ permalink raw reply
* Re: Perf Data on LSM in v5.3
From: Stephen Smalley @ 2020-01-24 14:57 UTC (permalink / raw)
To: Wenhui Zhang
Cc: John Johansen, Casey Schaufler, Casey Schaufler, James Morris,
linux-security-module, SELinux, Kees Cook, penguin-kernel,
Paul Moore
In-Reply-To: <c98000ea-df0e-1ab7-a0e2-b47d913f50c8@tycho.nsa.gov>
On 1/15/20 11:04 AM, Stephen Smalley wrote:
> On 1/15/20 10:59 AM, Wenhui Zhang wrote:
>>
>>
>> On Wed, Jan 15, 2020 at 10:41 AM Stephen Smalley <sds@tycho.nsa.gov
>> <mailto:sds@tycho.nsa.gov>> wrote:
>>
>> On 1/15/20 10:34 AM, Stephen Smalley wrote:
>> > On 1/15/20 10:21 AM, Wenhui Zhang wrote:
>> >>
>> >> On Wed, Jan 15, 2020 at 9:08 AM Stephen Smalley
>> <sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>
>> >> <mailto:sds@tycho.nsa.gov <mailto:sds@tycho.nsa.gov>>> wrote:
>> >>
>> >> On 1/15/20 8:40 AM, Stephen Smalley wrote:
>> >> > On 1/14/20 8:00 PM, Wenhui Zhang wrote:
>> >> >> Hi, John:
>> >> >>
>> >> >> It seems like, the MAC hooks are default to*return 0 or
>> empty
>> >> void
>> >> >> hooks* if CONFIG_SECURITY, CONFIG_SECURITY_NETWORK ,
>> >> >> CONFIG_PAGE_TABLE_ISOLATION, CONFIG_SECURITY_INFINIBAND,
>> >> >> CONFIG_SECURITY_PATH, CONFIG_INTEL_TXT,
>> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR,
>> >> >>
>> CONFIG_HARDENED_USERCOPY, CONFIG_HARDENED_USERCOPY_FALLBACK *are
>> >> NOT
>> >> >> set*.
>> >> >>
>> >> >> If HOOKs are "return 0 or empty void hooks", MAC is not
>> enabled.
>> >> >> In runtime of fs-benchmarks,
>> if CONFIG_DEFAULT_SECURITY_DAC=y,
>> >> then
>> >> >> capability is enabled.
>> >> >>
>> >> >> Please correct me if I am wrong.
>> >> >>
>> >> >> For the first test, wo-sec is tested with:
>> >> >> # CONFIG_SECURITY_DMESG_RESTRICT is not set
>> >> >> # CONFIG_SECURITY is not set
>> >> >> # CONFIG_SECURITYFS is not set
>> >> >> # CONFIG_PAGE_TABLE_ISOLATION is not set
>> >> >> # CONFIG_INTEL_TXT is not set
>> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR=y
>> >> >> # CONFIG_HARDENED_USERCOPY is not set
>> >> >> CONFIG_FORTIFY_SOURCE=y
>> >> >> # CONFIG_STATIC_USERMODEHELPER is not set
>> >> >> CONFIG_DEFAULT_SECURITY_DAC=y
>> >> >>
>> >> >>
>> >> >> For the second test, w-sec is tested with:
>> >> >> # CONFIG_SECURITY_DMESG_RESTRICT is not set
>> >> >> CONFIG_SECURITY=y
>> >> >> CONFIG_SECURITYFS=y
>> >> >> # CONFIG_SECURITY_NETWORK is not set
>> >> >> CONFIG_PAGE_TABLE_ISOLATION=y
>> >> >> CONFIG_SECURITY_INFINIBAND=y
>> >> >> CONFIG_SECURITY_PATH=y
>> >> >> CONFIG_INTEL_TXT=y
>> >> >> CONFIG_HAVE_HARDENED_USERCOPY_ALLOCATOR=y
>> >> >> CONFIG_HARDENED_USERCOPY=y
>> >> >> CONFIG_HARDENED_USERCOPY_FALLBACK=y
>> >> >> # CONFIG_HARDENED_USERCOPY_PAGESPAN is not set
>> >> >> CONFIG_FORTIFY_SOURCE=y
>> >> >> # CONFIG_STATIC_USERMODEHELPER is not set
>> >> >> # CONFIG_SECURITY_SMACK is not set
>> >> >> # CONFIG_SECURITY_TOMOYO is not set
>> >> >> # CONFIG_SECURITY_APPARMOR is not set
>> >> >> # CONFIG_SECURITY_LOADPIN is not set
>> >> >> # CONFIG_SECURITY_YAMA is not set
>> >> >> # CONFIG_SECURITY_SAFESETID is not set
>> >> >> # CONFIG_INTEGRITY is not set
>> >> >> CONFIG_DEFAULT_SECURITY_DAC=y
>> >> >> #
>> >> >>
>> >>
>> >>
>>
>> CONFIG_LSM="yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo"
>>
>>
>> >>
>> >>
>> >> >>
>> >> >
>> >> > Your configs should only differ with respect to
>> CONFIG_SECURITY*
>> >> if you
>> >> > want to evaluate LSM, SELinux, etc overheads.
>> >> PAGE_TABLE_ISOLATION,
>> >> > INTEL_TXT, and HARDENED_USERCOPY are not relevant to LSM
>> itself.
>> >> >
>> >> > Also, what benchmarks are you using? Your own home-grown
>> ones, a
>> >> set of
>> >> > open source standard benchmarks (if so, which ones?).
>> You should
>> >> > include both micro and macro benchmarks in your suite.
>> >> >
>> >> > How stable are your results? What kind of variance /
>> standard
>> >> deviation
>> >> > are you seeing?
>> >> >
>> >> > It is hard to get meaningful, reliable performance
>> measurements
>> >> so going
>> >> > down this road is not to be done lightly.
>> >>
>> >> Also, I note that you don't enable CONFIG_SECURITY_NETWORK
>> above.
>> >> That
>> >> means you aren't including the base LSM overhead for the
>> networking
>> >> security hooks. So if you then compare that against SELinux
>> (which
>> >> requires CONFIG_SECURITY_NETWORK), you are going to end up
>> >> attributing
>> >> the cost of both the LSM overhead and SELinux overhead all to
>> >> SELinux.
>> >> If you truly want to isolate the base LSM overhead, you
>> need to
>> >> enable
>> >> all the hooks.
>> >>
>> >> I will give it a try for enabling CONFIG_SECURITY_NETWORK later
>> this
>> >> week, however I wonder if this would affect the test results
>> that much.
>> >> I am testing with LMBench 2.5 , with focusing on filesystem unit
>> >> tests, however not network stack at this time.
>> >> My understanding of why this result is so different from previous
>> >> paper 20 years ago, is that the Bottleneck changes.
>> >> As Chris was testing with 4 cores , each 700MHz CPU, and 128MB
>> memory,
>> >> with HDD (latency is about 20,000,000 ns for sequential read).
>> >> The Bottleneck of accessing files w/ MAC are mostly on I/O.
>> >> However hardware setup is different now, we have much larger and
>> >> faster memory (better prefetching as well), with SSD (latency is
>> about
>> >> 49,000 ns for sequential read). , while CPU speed is not
>> increasing as
>> >> much as that of I/O.
>> >> The Bottleneck of accessing files w/ MAC are mostly on CPU now.
>> >
>> > Don't know if lmbench is still a good benchmark and I recall
>> struggling
>> > with it even back then to get stable results.
>> >
>> > Could be bottleneck changes, could be the fact that your kernel
>> config
>> > changes aren't limited to CONFIG_SECURITY* (e.g. PTI introduces
>> > non-trivial overheads), could be changes to LSM since that time
>> (e.g.
>> > stacking support, moving security_ calls out-of-line, more hooks,
>> ...),
>> > could be that running SELinux w/o policy is flooding the system
>> logs
>> > with warnings or other messages since it wasn't really designed
>> to be
>> > used that way past initialization. Lots of options, can't tell
>> without
>> > more info on your details.
>>
>> I'd think that these days one would leverage perf and/or lkp for
>> Linux
>> kernel performance measurements, not lmbench.
>>
>>
>> Thanks so much, I will give it a try for lkp and let you know how it
>> goes.
>> Maybe later next week or this weekend, we should have the results.
>
> Ok, please make sure your kernel configs are truly comparable (i.e. no
> differences other than the right set of CONFIG_SECURITY* options), that
> all of the same LSM hooks are enabled for comparing LSM-only versus
> SELinux (i.e. CONFIG_SECURITY and CONFIG_SECURITY_NETWORK both enabled),
> and consider using a distribution that actually supports SELinux out of
> the box (e.g. Fedora) so that you can properly test SELinux with a
> policy loaded in enforcing mode. Similarly if you want to do the same
> for AppArmor, except for it you'll need to enable CONFIG_SECURITY_PATH
> as well for the pathname-based hooks and you'll want to use Ubuntu or
> latest Debian to get a working policy.
One last point that I should have mentioned: you should likely run your
benchmarks both under an unconfined profile/domain and under a confined
profile/domain (the latter could be one that you define specifically for
your benchmark that just allows it everything) at least for AppArmor.
The reason is that AppArmor has an intrinsic concept of unconfined since
it was designed for targeted enforcement and its hook functions directly
test for the unconfined label early and skip permission checking in that
case, so if you only collect data while running benchmarks unconfined,
you won't see the real overheads imposed on a confined profile/domain.
In contrast, SELinux has no intrinsic concept of unconfined since it was
designed for full system confinement (as applied in "strict"
configurations and in Android); if there is an unconfined domain, it is
merely defined through policy (i.e. there must be explicit allow rules
allowing it everything) and the hook functions invoke the same
permission checking for both unconfined and confined domains alike.
^ permalink raw reply
* Re: [PATCH v14 00/23] LSM: Module stacking for AppArmor
From: Stephen Smalley @ 2020-01-24 15:05 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-1-casey@schaufler-ca.com>
On 1/23/20 7:22 PM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
>
> v14: Rebase to 5.5-rc5
> Incorporate feedback from v13
> - Use an array of audit rules (patch 0002)
> - Significant change, removed Acks (patch 0002)
> - Remove unneeded include (patch 0013)
> - Use context.len correctly (patch 0015)
> - Reorder code to be more sensible (patch 0016)
> - Drop SO_PEERCONTEXT as it's not needed yet (patch 0023)
Looks like you also dropped "LSM: Use lsmcontext in
security_dentry_init_security" entirely (was patch 16 in v13).
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-01-24 16:20 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <1de8338a-9c1c-c13b-16f0-e47ebec0e7ea@tycho.nsa.gov>
On 1/24/20 9:42 AM, Stephen Smalley wrote:
> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>> Add an entry /proc/.../attr/context which displays the full
>> process security "context" in compound format:'
>> lsm1\0value\0lsm2\0value\0...
>> This entry is not writable.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> Cc: linux-api@vger.kernel.org
>
> As previously discussed, there are issues with AppArmor's implementation
> of getprocattr() particularly around the trailing newline that
> dbus-daemon and perhaps others would like to see go away in any new
> interface. Hence, I don't think we should implement this new API using
> the existing getprocattr() hook lest it also be locked into the current
> behavior forever.
Also, it would be good if whatever hook is introduced to support
/proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT
implementation in the future so that we are guaranteed a consistent
result between the two interfaces, unlike the current situation for
/proc/self/attr/current versus SO_PEERSEC.
>
>> ---
>> Documentation/security/lsm.rst | 14 ++++++++
>> fs/proc/base.c | 1 +
>> security/security.c | 63 ++++++++++++++++++++++++++++++++++
>> 3 files changed, 78 insertions(+)
>>
>> diff --git a/Documentation/security/lsm.rst
>> b/Documentation/security/lsm.rst
>> index aadf47c808c0..a4979060f5d3 100644
>> --- a/Documentation/security/lsm.rst
>> +++ b/Documentation/security/lsm.rst
>> @@ -199,3 +199,17 @@ capability-related fields:
>> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
>> - ``fs/proc/array.c``::c:func:`task_cap()`
>> +
>> +LSM External Interfaces
>> +=======================
>> +
>> +The LSM infrastructure does not generally provide external interfaces.
>> +The individual security modules provide what external interfaces they
>> +require. The infrastructure does provide an interface for the special
>> +case where multiple security modules provide a process context. This
>> +is provided in compound context format.
>> +
>> +- `lsm1\0value\0lsm2\0value\0`
>> +
>> +The special file ``/proc/pid/attr/context`` provides the security
>> +context of the identified process.
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 950c200cb9ad..d13c2cf50e4b 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>> ATTR(NULL, "keycreate", 0666),
>> ATTR(NULL, "sockcreate", 0666),
>> ATTR(NULL, "display", 0666),
>> + ATTR(NULL, "context", 0666),
>> #ifdef CONFIG_SECURITY_SMACK
>> DIR("smack", 0555,
>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>> diff --git a/security/security.c b/security/security.c
>> index 6a77c8b2ffbc..fdd0c85df89e 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct
>> task_struct *task)
>> panic("%s: Early task alloc failed.\n", __func__);
>> }
>> +/**
>> + * append_ctx - append a lsm/context pair to a compound context
>> + * @ctx: the existing compound context
>> + * @ctxlen: size of the old context, including terminating nul byte
>> + * @lsm: new lsm name, nul terminated
>> + * @new: new context, possibly nul terminated
>> + * @newlen: maximum size of @new
>> + *
>> + * replace @ctx with a new compound context, appending @newlsm and @new
>> + * to @ctx. On exit the new data replaces the old, which is freed.
>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>> + *
>> + * Returns 0 on success, -ENOMEM if no memory is available.
>> + */
>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char
>> *new,
>> + int newlen)
>> +{
>> + char *final;
>> + int llen;
>> +
>> + llen = strlen(lsm) + 1;
>> + newlen = strnlen(new, newlen) + 1;
>> +
>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>> + if (final == NULL)
>> + return -ENOMEM;
>> + if (*ctxlen)
>> + memcpy(final, *ctx, *ctxlen);
>> + memcpy(final + *ctxlen, lsm, llen);
>> + memcpy(final + *ctxlen + llen, new, newlen);
>> + kfree(*ctx);
>> + *ctx = final;
>> + *ctxlen = *ctxlen + llen + newlen;
>> + return 0;
>> +}
>> +
>> /*
>> * Hook list operation macros.
>> *
>> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p,
>> const char *lsm, char *name,
>> char **value)
>> {
>> struct security_hook_list *hp;
>> + char *final = NULL;
>> + char *cp;
>> + int rc = 0;
>> + int finallen = 0;
>> int display = lsm_task_display(current);
>> int slot = 0;
>> @@ -2068,6 +2108,29 @@ int security_getprocattr(struct task_struct *p,
>> const char *lsm, char *name,
>> return -ENOMEM;
>> }
>> + if (!strcmp(name, "context")) {
>> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>> + list) {
>> + rc = hp->hook.getprocattr(p, "current", &cp);
>> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
>> + continue;
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>> + cp, rc);
>> + if (rc < 0) {
>> + kfree(final);
>> + return rc;
>> + }
>> + }
>> + if (final == NULL)
>> + return -EINVAL;
>> + *value = final;
>> + return finallen;
>> + }
>> +
>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>> continue;
>>
>
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Casey Schaufler @ 2020-01-24 19:28 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <f3dea066-1f6d-4b92-1a5b-dac25b58aae7@tycho.nsa.gov>
On 1/24/2020 8:20 AM, Stephen Smalley wrote:
> On 1/24/20 9:42 AM, Stephen Smalley wrote:
>> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>>> Add an entry /proc/.../attr/context which displays the full
>>> process security "context" in compound format:'
>>> lsm1\0value\0lsm2\0value\0...
>>> This entry is not writable.
>>>
>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>> Cc: linux-api@vger.kernel.org
>>
>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever.
>
> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC.
I don't believe that a new hook is necessary, and that introducing one
just to deal with a '\n' would be pedantic. We really have two rational
options. AppArmor could drop the '\n' from their "context". Or, we can
simply document that the /proc/pid/attr/context interface will trim any
trailing whitespace. I understand that this would be a break from the
notion that the LSM infrastructure does not constrain what a module uses
for its own data. On the other hand, we have been saying that "context"s
are strings, and ignoring trailing whitespace is usual behavior for
strings.
>
>>
>>> ---
>>> Documentation/security/lsm.rst | 14 ++++++++
>>> fs/proc/base.c | 1 +
>>> security/security.c | 63 ++++++++++++++++++++++++++++++++++
>>> 3 files changed, 78 insertions(+)
>>>
>>> diff --git a/Documentation/security/lsm.rst b/Documentation/security/lsm.rst
>>> index aadf47c808c0..a4979060f5d3 100644
>>> --- a/Documentation/security/lsm.rst
>>> +++ b/Documentation/security/lsm.rst
>>> @@ -199,3 +199,17 @@ capability-related fields:
>>> - ``fs/nfsd/auth.c``::c:func:`nfsd_setuser()`
>>> - ``fs/proc/array.c``::c:func:`task_cap()`
>>> +
>>> +LSM External Interfaces
>>> +=======================
>>> +
>>> +The LSM infrastructure does not generally provide external interfaces.
>>> +The individual security modules provide what external interfaces they
>>> +require. The infrastructure does provide an interface for the special
>>> +case where multiple security modules provide a process context. This
>>> +is provided in compound context format.
>>> +
>>> +- `lsm1\0value\0lsm2\0value\0`
>>> +
>>> +The special file ``/proc/pid/attr/context`` provides the security
>>> +context of the identified process.
>>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>>> index 950c200cb9ad..d13c2cf50e4b 100644
>>> --- a/fs/proc/base.c
>>> +++ b/fs/proc/base.c
>>> @@ -2653,6 +2653,7 @@ static const struct pid_entry attr_dir_stuff[] = {
>>> ATTR(NULL, "keycreate", 0666),
>>> ATTR(NULL, "sockcreate", 0666),
>>> ATTR(NULL, "display", 0666),
>>> + ATTR(NULL, "context", 0666),
>>> #ifdef CONFIG_SECURITY_SMACK
>>> DIR("smack", 0555,
>>> proc_smack_attr_dir_inode_ops, proc_smack_attr_dir_ops),
>>> diff --git a/security/security.c b/security/security.c
>>> index 6a77c8b2ffbc..fdd0c85df89e 100644
>>> --- a/security/security.c
>>> +++ b/security/security.c
>>> @@ -722,6 +722,42 @@ static void __init lsm_early_task(struct task_struct *task)
>>> panic("%s: Early task alloc failed.\n", __func__);
>>> }
>>> +/**
>>> + * append_ctx - append a lsm/context pair to a compound context
>>> + * @ctx: the existing compound context
>>> + * @ctxlen: size of the old context, including terminating nul byte
>>> + * @lsm: new lsm name, nul terminated
>>> + * @new: new context, possibly nul terminated
>>> + * @newlen: maximum size of @new
>>> + *
>>> + * replace @ctx with a new compound context, appending @newlsm and @new
>>> + * to @ctx. On exit the new data replaces the old, which is freed.
>>> + * @ctxlen is set to the new size, which includes a trailing nul byte.
>>> + *
>>> + * Returns 0 on success, -ENOMEM if no memory is available.
>>> + */
>>> +static int append_ctx(char **ctx, int *ctxlen, const char *lsm, char *new,
>>> + int newlen)
>>> +{
>>> + char *final;
>>> + int llen;
>>> +
>>> + llen = strlen(lsm) + 1;
>>> + newlen = strnlen(new, newlen) + 1;
>>> +
>>> + final = kzalloc(*ctxlen + llen + newlen, GFP_KERNEL);
>>> + if (final == NULL)
>>> + return -ENOMEM;
>>> + if (*ctxlen)
>>> + memcpy(final, *ctx, *ctxlen);
>>> + memcpy(final + *ctxlen, lsm, llen);
>>> + memcpy(final + *ctxlen + llen, new, newlen);
>>> + kfree(*ctx);
>>> + *ctx = final;
>>> + *ctxlen = *ctxlen + llen + newlen;
>>> + return 0;
>>> +}
>>> +
>>> /*
>>> * Hook list operation macros.
>>> *
>>> @@ -2041,6 +2077,10 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>> char **value)
>>> {
>>> struct security_hook_list *hp;
>>> + char *final = NULL;
>>> + char *cp;
>>> + int rc = 0;
>>> + int finallen = 0;
>>> int display = lsm_task_display(current);
>>> int slot = 0;
>>> @@ -2068,6 +2108,29 @@ int security_getprocattr(struct task_struct *p, const char *lsm, char *name,
>>> return -ENOMEM;
>>> }
>>> + if (!strcmp(name, "context")) {
>>> + hlist_for_each_entry(hp, &security_hook_heads.getprocattr,
>>> + list) {
>>> + rc = hp->hook.getprocattr(p, "current", &cp);
>>> + if (rc == -EINVAL || rc == -ENOPROTOOPT)
>>> + continue;
>>> + if (rc < 0) {
>>> + kfree(final);
>>> + return rc;
>>> + }
>>> + rc = append_ctx(&final, &finallen, hp->lsmid->lsm,
>>> + cp, rc);
>>> + if (rc < 0) {
>>> + kfree(final);
>>> + return rc;
>>> + }
>>> + }
>>> + if (final == NULL)
>>> + return -EINVAL;
>>> + *value = final;
>>> + return finallen;
>>> + }
>>> +
>>> hlist_for_each_entry(hp, &security_hook_heads.getprocattr, list) {
>>> if (lsm != NULL && strcmp(lsm, hp->lsmid->lsm))
>>> continue;
>>>
>>
>
^ permalink raw reply
* Re: [PATCH v14 22/23] LSM: Add /proc attr entry for full LSM context
From: Stephen Smalley @ 2020-01-24 20:16 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <9afb8d9d-a590-0e13-bf46-53a347ea15dd@schaufler-ca.com>
On 1/24/20 2:28 PM, Casey Schaufler wrote:
> On 1/24/2020 8:20 AM, Stephen Smalley wrote:
>> On 1/24/20 9:42 AM, Stephen Smalley wrote:
>>> On 1/23/20 7:23 PM, Casey Schaufler wrote:
>>>> Add an entry /proc/.../attr/context which displays the full
>>>> process security "context" in compound format:'
>>>> lsm1\0value\0lsm2\0value\0...
>>>> This entry is not writable.
>>>>
>>>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>>>> Cc: linux-api@vger.kernel.org
>>>
>>> As previously discussed, there are issues with AppArmor's implementation of getprocattr() particularly around the trailing newline that dbus-daemon and perhaps others would like to see go away in any new interface. Hence, I don't think we should implement this new API using the existing getprocattr() hook lest it also be locked into the current behavior forever.
>>
>> Also, it would be good if whatever hook is introduced to support /proc/pid/attr/context could also be leveraged by the SO_PEERCONTEXT implementation in the future so that we are guaranteed a consistent result between the two interfaces, unlike the current situation for /proc/self/attr/current versus SO_PEERSEC.
>
> I don't believe that a new hook is necessary, and that introducing one
> just to deal with a '\n' would be pedantic. We really have two rational
> options. AppArmor could drop the '\n' from their "context". Or, we can
> simply document that the /proc/pid/attr/context interface will trim any
> trailing whitespace. I understand that this would be a break from the
> notion that the LSM infrastructure does not constrain what a module uses
> for its own data. On the other hand, we have been saying that "context"s
> are strings, and ignoring trailing whitespace is usual behavior for
> strings.
Well, you can either introduce a new common underlying hook for use by
/proc/pid/attr/context and SO_PEERCONTEXT to produce the string that is
to be returned to userspace (in order to guarantee consistency in format
and allowing them to be directly compared, which I think is what the
dbus maintainers wanted), or you can modify every security module to
provide that guarantee in its existing getprocattr and getpeersec* hook
functions (SELinux already provides this guarantee; Smack and AppArmor
produce slightly different results with respect to \0 and/or \n), or you
can have the framework trim the values it gets from the security modules
before composing them. But you need to do one of those things before
this interface gets merged upstream.
Aside from the trailing newline and \0 issues, AppArmor also has a
whitespace-separated (mode) field that may or may not be present in the
contexts it presently returns, ala "/usr/sbin/cupsd (enforce)". Not
sure what they want for the new interfaces.
^ permalink raw reply
* Re: [PATCH v14 00/23] LSM: Module stacking for AppArmor
From: Stephen Smalley @ 2020-01-24 21:04 UTC (permalink / raw)
To: Casey Schaufler, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul
In-Reply-To: <20200124002306.3552-1-casey@schaufler-ca.com>
On 1/23/20 7:22 PM, Casey Schaufler wrote:
> This patchset provides the changes required for
> the AppArmor security module to stack safely with any other.
>
> v14: Rebase to 5.5-rc5
> Incorporate feedback from v13
> - Use an array of audit rules (patch 0002)
> - Significant change, removed Acks (patch 0002)
> - Remove unneeded include (patch 0013)
> - Use context.len correctly (patch 0015)
> - Reorder code to be more sensible (patch 0016)
> - Drop SO_PEERCONTEXT as it's not needed yet (patch 0023)
I don't know for sure if this is your bug, but it happens every time I
boot with your patches applied and not at all on stock v5.5-rc5 so here
it is. Will try to bisect as time permits but not until next week.
Trigger seems to be loading the tun driver.
[ 67.726834] tun: Universal TUN/TAP device driver, 1.6
[ 67.736657]
==================================================================
[ 67.741335] BUG: KASAN: slab-out-of-bounds in sock_init_data+0x14a/0x5a0
[ 67.745037] Read of size 4 at addr ffff88870afe8928 by task libvirtd/1238
[ 67.751861] CPU: 4 PID: 1238 Comm: libvirtd Tainted: G
T 5.5.0-rc5+ #54
[ 67.756250] Call Trace:
[ 67.759510] dump_stack+0xb8/0x110
[ 67.761604] print_address_description.constprop.0+0x1f/0x280
[ 67.763768] __kasan_report.cold+0x75/0x8f
[ 67.765895] ? sock_init_data+0x14a/0x5a0
[ 67.768282] kasan_report+0xe/0x20
[ 67.770397] sock_init_data+0x14a/0x5a0
[ 67.772511] tun_chr_open+0x1de/0x280 [tun]
[ 67.774644] misc_open+0x1cb/0x210
[ 67.776820] chrdev_open+0x15b/0x350
[ 67.778917] ? cdev_put.part.0+0x30/0x30
[ 67.781030] do_dentry_open+0x2cb/0x800
[ 67.783135] ? cdev_put.part.0+0x30/0x30
[ 67.785225] ? devcgroup_check_permission+0x11a/0x260
[ 67.787321] ? __x64_sys_fchdir+0xf0/0xf0
[ 67.789418] ? security_inode_permission+0x5b/0x70
[ 67.791513] path_openat+0x858/0x14a0
[ 67.793589] ? path_mountpoint+0x5e0/0x5e0
[ 67.795719] ? mark_lock+0xb8/0xb00
[ 67.797786] do_filp_open+0x11e/0x1b0
[ 67.799840] ? may_open_dev+0x60/0x60
[ 67.801871] ? match_held_lock+0x1b/0x240
[ 67.803968] ? lock_downgrade+0x360/0x360
[ 67.805997] ? do_raw_spin_lock+0x119/0x1d0
[ 67.808041] ? rwlock_bug.part.0+0x60/0x60
[ 67.810099] ? do_raw_spin_unlock+0xa3/0x130
[ 67.812244] ? _raw_spin_unlock+0x1f/0x30
[ 67.814287] ? __alloc_fd+0x143/0x2f0
[ 67.816324] do_sys_open+0x1f0/0x2d0
[ 67.818358] ? filp_open+0x50/0x50
[ 67.820404] ? trace_hardirqs_on_thunk+0x1a/0x1c
[ 67.822447] ? lockdep_hardirqs_off+0xbe/0x100
[ 67.824473] ? mark_held_locks+0x24/0x90
[ 67.826484] do_syscall_64+0x74/0xd0
[ 67.828480] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 67.830478] RIP: 0033:0x7f1a2cce6074
[ 67.832495] Code: 24 20 eb 8f 66 90 44 89 54 24 0c e8 86 f4 ff ff 44
8b 54 24 0c 44 89 e2 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00 00 0f
05 <48> 3d 00 f0 ff ff 77 32 44 89 c7 89 44 24 0c e8 b8 f4 ff ff 8b 44
[ 67.834760] RSP: 002b:00007f19e4af46d0 EFLAGS: 00000293 ORIG_RAX:
0000000000000101
[ 67.837032] RAX: ffffffffffffffda RBX: 0000000000000000 RCX:
00007f1a2cce6074
[ 67.839318] RDX: 0000000000000002 RSI: 00007f1a2d0bfb67 RDI:
00000000ffffff9c
[ 67.841598] RBP: 00007f1a2d0bfb67 R08: 0000000000000000 R09:
00007f19e4af4914
[ 67.843941] R10: 0000000000000000 R11: 0000000000000293 R12:
0000000000000002
[ 67.846283] R13: 000000000000000d R14: 00007f19e4af4920 R15:
00007f1a2d0bfb67
[ 67.850936] Allocated by task 1238:
[ 67.853241] save_stack+0x1b/0x80
[ 67.855533] __kasan_kmalloc.constprop.0+0xc2/0xd0
[ 67.857935] sk_prot_alloc+0x115/0x170
[ 67.860235] sk_alloc+0x2f/0xa10
[ 67.862541] tun_chr_open+0x4d/0x280 [tun]
[ 67.864894] misc_open+0x1cb/0x210
[ 67.867164] chrdev_open+0x15b/0x350
[ 67.869448] do_dentry_open+0x2cb/0x800
[ 67.871768] path_openat+0x858/0x14a0
[ 67.874041] do_filp_open+0x11e/0x1b0
[ 67.876328] do_sys_open+0x1f0/0x2d0
[ 67.878592] do_syscall_64+0x74/0xd0
[ 67.880899] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 67.885431] Freed by task 726:
[ 67.887689] save_stack+0x1b/0x80
[ 67.889967] __kasan_slab_free+0x12c/0x170
[ 67.892197] kfree+0xff/0x430
[ 67.894444] uevent_show+0x176/0x1b0
[ 67.896709] dev_attr_show+0x37/0x70
[ 67.898940] sysfs_kf_seq_show+0x119/0x210
[ 67.901159] seq_read+0x29d/0x720
[ 67.903367] vfs_read+0xf9/0x1f0
[ 67.905538] ksys_read+0xc9/0x160
[ 67.907736] do_syscall_64+0x74/0xd0
[ 67.909889] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 67.914100] The buggy address belongs to the object at ffff88870afe8000
which belongs to the cache kmalloc-4k of size 4096
[ 67.918357] The buggy address is located 2344 bytes inside of
4096-byte region [ffff88870afe8000, ffff88870afe9000)
[ 67.922562] The buggy address belongs to the page:
[ 67.924725] page:ffffea001c2bfa00 refcount:1 mapcount:0
mapping:ffff88881f00de00 index:0x0 compound_mapcount: 0
[ 67.926926] raw: 0017ffe000010200 ffffea001c167a00 0000000200000002
ffff88881f00de00
[ 67.929144] raw: 0000000000000000 0000000080040004 00000001ffffffff
0000000000000000
[ 67.931362] page dumped because: kasan: bad access detected
[ 67.936192] Memory state around the buggy address:
[ 67.938438] ffff88870afe8800: 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00
[ 67.941078] ffff88870afe8880: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 67.943393] >ffff88870afe8900: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 67.945709] ^
[ 67.948000] ffff88870afe8980: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 67.950311] ffff88870afe8a00: fc fc fc fc fc fc fc fc fc fc fc fc fc
fc fc fc
[ 67.952629]
==================================================================
^ permalink raw reply
* Re: [PATCH v14 00/23] LSM: Module stacking for AppArmor
From: Casey Schaufler @ 2020-01-24 21:49 UTC (permalink / raw)
To: Stephen Smalley, casey.schaufler, jmorris, linux-security-module,
selinux
Cc: keescook, john.johansen, penguin-kernel, paul, Casey Schaufler
In-Reply-To: <22585291-b7e0-5a22-6682-168611d902fa@tycho.nsa.gov>
On 1/24/2020 1:04 PM, Stephen Smalley wrote:
> On 1/23/20 7:22 PM, Casey Schaufler wrote:
>> This patchset provides the changes required for
>> the AppArmor security module to stack safely with any other.
>>
>> v14: Rebase to 5.5-rc5
>> Incorporate feedback from v13
>> - Use an array of audit rules (patch 0002)
>> - Significant change, removed Acks (patch 0002)
>> - Remove unneeded include (patch 0013)
>> - Use context.len correctly (patch 0015)
>> - Reorder code to be more sensible (patch 0016)
>> - Drop SO_PEERCONTEXT as it's not needed yet (patch 0023)
>
> I don't know for sure if this is your bug, but it happens every time I boot with your patches applied and not at all on stock v5.5-rc5 so here it is. Will try to bisect as time permits but not until next week. Trigger seems to be loading the tun driver.
Thanks. I will have a look as well.
>
> [ 67.726834] tun: Universal TUN/TAP device driver, 1.6
> [ 67.736657] ==================================================================
> [ 67.741335] BUG: KASAN: slab-out-of-bounds in sock_init_data+0x14a/0x5a0
> [ 67.745037] Read of size 4 at addr ffff88870afe8928 by task libvirtd/1238
>
> [ 67.751861] CPU: 4 PID: 1238 Comm: libvirtd Tainted: G T 5.5.0-rc5+ #54
> [ 67.756250] Call Trace:
> [ 67.759510] dump_stack+0xb8/0x110
> [ 67.761604] print_address_description.constprop.0+0x1f/0x280
> [ 67.763768] __kasan_report.cold+0x75/0x8f
> [ 67.765895] ? sock_init_data+0x14a/0x5a0
> [ 67.768282] kasan_report+0xe/0x20
> [ 67.770397] sock_init_data+0x14a/0x5a0
> [ 67.772511] tun_chr_open+0x1de/0x280 [tun]
> [ 67.774644] misc_open+0x1cb/0x210
> [ 67.776820] chrdev_open+0x15b/0x350
> [ 67.778917] ? cdev_put.part.0+0x30/0x30
> [ 67.781030] do_dentry_open+0x2cb/0x800
> [ 67.783135] ? cdev_put.part.0+0x30/0x30
> [ 67.785225] ? devcgroup_check_permission+0x11a/0x260
> [ 67.787321] ? __x64_sys_fchdir+0xf0/0xf0
> [ 67.789418] ? security_inode_permission+0x5b/0x70
> [ 67.791513] path_openat+0x858/0x14a0
> [ 67.793589] ? path_mountpoint+0x5e0/0x5e0
> [ 67.795719] ? mark_lock+0xb8/0xb00
> [ 67.797786] do_filp_open+0x11e/0x1b0
> [ 67.799840] ? may_open_dev+0x60/0x60
> [ 67.801871] ? match_held_lock+0x1b/0x240
> [ 67.803968] ? lock_downgrade+0x360/0x360
> [ 67.805997] ? do_raw_spin_lock+0x119/0x1d0
> [ 67.808041] ? rwlock_bug.part.0+0x60/0x60
> [ 67.810099] ? do_raw_spin_unlock+0xa3/0x130
> [ 67.812244] ? _raw_spin_unlock+0x1f/0x30
> [ 67.814287] ? __alloc_fd+0x143/0x2f0
> [ 67.816324] do_sys_open+0x1f0/0x2d0
> [ 67.818358] ? filp_open+0x50/0x50
> [ 67.820404] ? trace_hardirqs_on_thunk+0x1a/0x1c
> [ 67.822447] ? lockdep_hardirqs_off+0xbe/0x100
> [ 67.824473] ? mark_held_locks+0x24/0x90
> [ 67.826484] do_syscall_64+0x74/0xd0
> [ 67.828480] entry_SYSCALL_64_after_hwframe+0x49/0xbe
> [ 67.830478] RIP: 0033:0x7f1a2cce6074
> [ 67.832495] Code: 24 20 eb 8f 66 90 44 89 54 24 0c e8 86 f4 ff ff 44 8b 54 24 0c 44 89 e2 48 89 ee 41 89 c0 bf 9c ff ff ff b8 01 01 00 00 0f 05 <48> 3d 00 f0 ff ff 77 32 44 89 c7 89 44 24 0c e8 b8 f4 ff ff 8b 44
> [ 67.834760] RSP: 002b:00007f19e4af46d0 EFLAGS: 00000293 ORIG_RAX: 0000000000000101
> [ 67.837032] RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f1a2cce6074
> [ 67.839318] RDX: 0000000000000002 RSI: 00007f1a2d0bfb67 RDI: 00000000ffffff9c
> [ 67.841598] RBP: 00007f1a2d0bfb67 R08: 0000000000000000 R09: 00007f19e4af4914
> [ 67.843941] R10: 0000000000000000 R11: 0000000000000293 R12: 0000000000000002
> [ 67.846283] R13: 000000000000000d R14: 00007f19e4af4920 R15: 00007f1a2d0bfb67
>
> [ 67.850936] Allocated by task 1238:
> [ 67.853241] save_stack+0x1b/0x80
> [ 67.855533] __kasan_kmalloc.constprop.0+0xc2/0xd0
> [ 67.857935] sk_prot_alloc+0x115/0x170
> [ 67.860235] sk_alloc+0x2f/0xa10
> [ 67.862541] tun_chr_open+0x4d/0x280 [tun]
> [ 67.864894] misc_open+0x1cb/0x210
> [ 67.867164] chrdev_open+0x15b/0x350
> [ 67.869448] do_dentry_open+0x2cb/0x800
> [ 67.871768] path_openat+0x858/0x14a0
> [ 67.874041] do_filp_open+0x11e/0x1b0
> [ 67.876328] do_sys_open+0x1f0/0x2d0
> [ 67.878592] do_syscall_64+0x74/0xd0
> [ 67.880899] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> [ 67.885431] Freed by task 726:
> [ 67.887689] save_stack+0x1b/0x80
> [ 67.889967] __kasan_slab_free+0x12c/0x170
> [ 67.892197] kfree+0xff/0x430
> [ 67.894444] uevent_show+0x176/0x1b0
> [ 67.896709] dev_attr_show+0x37/0x70
> [ 67.898940] sysfs_kf_seq_show+0x119/0x210
> [ 67.901159] seq_read+0x29d/0x720
> [ 67.903367] vfs_read+0xf9/0x1f0
> [ 67.905538] ksys_read+0xc9/0x160
> [ 67.907736] do_syscall_64+0x74/0xd0
> [ 67.909889] entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> [ 67.914100] The buggy address belongs to the object at ffff88870afe8000
> which belongs to the cache kmalloc-4k of size 4096
> [ 67.918357] The buggy address is located 2344 bytes inside of
> 4096-byte region [ffff88870afe8000, ffff88870afe9000)
> [ 67.922562] The buggy address belongs to the page:
> [ 67.924725] page:ffffea001c2bfa00 refcount:1 mapcount:0 mapping:ffff88881f00de00 index:0x0 compound_mapcount: 0
> [ 67.926926] raw: 0017ffe000010200 ffffea001c167a00 0000000200000002 ffff88881f00de00
> [ 67.929144] raw: 0000000000000000 0000000080040004 00000001ffffffff 0000000000000000
> [ 67.931362] page dumped because: kasan: bad access detected
>
> [ 67.936192] Memory state around the buggy address:
> [ 67.938438] ffff88870afe8800: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> [ 67.941078] ffff88870afe8880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 67.943393] >ffff88870afe8900: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 67.945709] ^
> [ 67.948000] ffff88870afe8980: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 67.950311] ffff88870afe8a00: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
> [ 67.952629] ==================================================================
^ permalink raw reply
* Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: James Morris @ 2020-01-24 21:55 UTC (permalink / raw)
To: KP Singh; +Cc: Casey Schaufler, LKML, Linux Security Module list, bpf
In-Reply-To: <20200124012501.GA8709@chromium.org>
On Thu, 23 Jan 2020, KP Singh wrote:
>
> > If you want to put mutable hook handling in the infrastructure
> > you need to make it general mutable hook handling as opposed to
> > BPF hook handling. I don't know if that would be acceptable for
> > all the reasons called out about dynamic module loading.
>
> We can have generic mutable hook handling and if an LSM doesn't
--> provide a mutable security_hook_heads, it would not allow dynamic
> hooks / dynamic module loading.
>
> So, in practice it will just be the BPF LSM that allows mutable hooks
> and the other existing LSMs won't. I guess it will be cleaner than
> calling the BPF hooks directly from the LSM code (i.e in security.c)
I'm inclined to only have mutable hooks for KRSI, not for all LSMs. This
is a special case and we don't need to provide this for anyone else.
Btw, folks, PLEASE trim replies.
--
James Morris
<jmorris@namei.org>
^ permalink raw reply
* [PATCH v7 00/11] proc: modernize proc to support multiple private instances
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
Greetings!
Preface:
--------
This is patchset v7 to modernize procfs and make it able to support multiple
private instances per the same pid namespace.
This patchset can be applied on top of v5.4-rc7-49-g0e3f1ad80fc8
Procfs modernization:
---------------------
Historically procfs was always tied to pid namespaces, during pid
namespace creation we internally create a procfs mount for it. However,
this has the effect that all new procfs mounts are just a mirror of the
internal one, any change, any mount option update, any new future
introduction will propagate to all other procfs mounts that are in the
same pid namespace.
This may have solved several use cases in that time. However today we
face new requirements, and making procfs able to support new private
instances inside same pid namespace seems a major point. If we want to
to introduce new features and security mechanisms we have to make sure
first that we do not break existing usecases. Supporting private procfs
instances will allow to support new features and behaviour without
propagating it to all other procfs mounts.
Today procfs is more of a burden especially to some Embedded, IoT,
sandbox, container use cases. In user space we are over-mounting null
or inaccessible files on top to hide files and information. If we want
to hide pids we have to create PID namespaces otherwise mount options
propagate to all other proc mounts, changing a mount option value in one
mount will propagate to all other proc mounts. If we want to introduce
new features, then they will propagate to all other mounts too, resulting
either maybe new useful functionality or maybe breaking stuff. We have
also to note that userspace should not workaround procfs, the kernel
should just provide a sane simple interface.
In this regard several developers and maintainers pointed out that
there are problems with procfs and it has to be modernized:
"Here's another one: split up and modernize /proc." by Andy Lutomirski [1]
Discussion about kernel pointer leaks:
"And yes, as Kees and Daniel mentioned, it's definitely not just dmesg.
In fact, the primary things tend to be /proc and /sys, not dmesg
itself." By Linus Torvalds [2]
Lot of other areas in the kernel and filesystems have been updated to be
able to support private instances, devpts is one major example [3].
Which will be used for:
1) Embedded systems and IoT: usually we have one supervisor for
apps, we have some lightweight sandbox support, however if we create
pid namespaces we have to manage all the processes inside too,
where our goal is to be able to run a bunch of apps each one inside
its own mount namespace, maybe use network namespaces for vlans
setups, but right now we only want mount namespaces, without all the
other complexity. We want procfs to behave more like a real file system,
and block access to inodes that belong to other users. The 'hidepid=' will
not work since it is a shared mount option.
2) Containers, sandboxes and Private instances of file systems - devpts case
Historically, lot of file systems inside Linux kernel view when instantiated
were just a mirror of an already created and mounted filesystem. This was the
case of devpts filesystem, it seems at that time the requirements were to
optimize things and reuse the same memory, etc. This design used to work but not
anymore with today's containers, IoT, hostile environments and all the privacy
challenges that Linux faces.
In that regards, devpts was updated so that each new mounts is a total
independent file system by the following patches:
"devpts: Make each mount of devpts an independent filesystem" by
Eric W. Biederman [3] [4]
3) Linux Security Modules have multiple ptrace paths inside some
subsystems, however inside procfs, the implementation does not guarantee
that the ptrace() check which triggers the security_ptrace_check() hook
will always run. We have the 'hidepid' mount option that can be used to
force the ptrace_may_access() check inside has_pid_permissions() to run.
The problem is that 'hidepid' is per pid namespace and not attached to
the mount point, any remount or modification of 'hidepid' will propagate
to all other procfs mounts.
This also does not allow to support Yama LSM easily in desktop and user
sessions. Yama ptrace scope which restricts ptrace and some other
syscalls to be allowed only on inferiors, can be updated to have a
per-task context, where the context will be inherited during fork(),
clone() and preserved across execve(). If we support multiple private
procfs instances, then we may force the ptrace_may_access() on
/proc/<pids>/ to always run inside that new procfs instances. This will
allow to specifiy on user sessions if we should populate procfs with
pids that the user can ptrace or not.
By using Yama ptrace scope, some restricted users will only be able to see
inferiors inside /proc, they won't even be able to see their other
processes. Some software like Chromium, Firefox's crash handler, Wine
and others are already using Yama to restrict which processes can be
ptracable. With this change this will give the possibility to restrict
/proc/<pids>/ but more importantly this will give desktop users a
generic and usuable way to specifiy which users should see all processes
and which user can not.
Side notes:
* This covers the lack of seccomp where it is not able to parse
arguments, it is easy to install a seccomp filter on direct syscalls
that operate on pids, however /proc/<pid>/ is a Linux ABI using
filesystem syscalls. With this change all LSMs should be able to analyze
open/read/write/close... on /proc/<pid>/
4) This will allow to implement new features either in kernel or
userspace without having to worry about procfs.
In containers, sandboxes, etc we have workarounds to hide some /proc
inodes, this should be supported natively without doing extra complex
work, the kernel should be able to support sane options that work with
today and future Linux use cases.
5) Creation of new superblock with all procfs options for each procfs
mount will fix the ignoring of mount options. The problem is that the
second mount of procfs in the same pid namespace ignores the mount
options. The mount options are ignored without error until procfs is
remounted.
Before:
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
# strace -e mount mount -o hidepid=1 -t proc proc /tmp/proc
mount("proc", "/tmp/proc", "proc", 0, "hidepid=1") = 0
+++ exited with 0 +++
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
proc /tmp/proc proc rw,relatime,hidepid=2 0 0
# mount -o remount,hidepid=1 -t proc proc /tmp/proc
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=1 0 0
proc /tmp/proc proc rw,relatime,hidepid=1 0 0
After:
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
# mount -o hidepid=1 -t proc proc /tmp/proc
# grep ^proc /proc/mounts
proc /proc proc rw,relatime,hidepid=2 0 0
proc /tmp/proc proc rw,relatime,hidepid=1 0 0
Introduced changes:
-------------------
Each mount of procfs creates a separate procfs instance with its own
mount options.
This series adds few new mount options:
* New 'hidepid=4' mount option to show only ptraceable processes in the procfs.
This allows to support lightweight sandboxes in Embedded Linux, also
solves the case for LSM where now with this mount option, we make sure
that they have a ptrace path in procfs.
* 'subset=pidfs' that allows to hide non-pid inodes from procfs. It can be used
in containers and sandboxes, as these are already trying to hide and block
access to procfs inodes anyway.
ChangeLog:
----------
# v7:
* 'pidonly=1' renamed to 'subset=pidfs' as Alexey Dobriyan suggested.
* HIDEPID_* moved to uapi/ as they are user interface to mount().
Suggested-by Alexey Dobriyan <adobriyan@gmail.com>
# v6:
* 'hidepid=' and 'gid=' mount options are moved from pid namespace to superblock.
* 'newinstance' mount option removed as Eric W. Biederman suggested.
Mount of procfs always creates a new instance.
* 'limit_pids' renamed to 'hidepid=3'.
* I took into account the comment of Linus Torvalds [7].
* Documentation added.
# v5:
* Fixed a bug that caused a problem with the Fedora boot.
* The 'pidonly' option is visible among the mount options.
# v2:
* Renamed mount options to 'newinstance' and 'pids='
Suggested-by: Andy Lutomirski <luto@kernel.org>
* Fixed order of commit, Suggested-by: Andy Lutomirski <luto@kernel.org>
* Many bug fixes.
# v1:
* Removed 'unshared' mount option and replaced it with 'limit_pids'
which is attached to the current procfs mount.
Suggested-by Andy Lutomirski <luto@kernel.org>
* Do not fill dcache with pid entries that we can not ptrace.
* Many bug fixes.
References:
-----------
[1] https://lists.linuxfoundation.org/pipermail/ksummit-discuss/2017-January/004215.html
[2] http://www.openwall.com/lists/kernel-hardening/2017/10/05/5
[3] https://lwn.net/Articles/689539/
[4] http://lxr.free-electrons.com/source/Documentation/filesystems/devpts.txt?v=3.14
[5] https://lkml.org/lkml/2017/5/2/407
[6] https://lkml.org/lkml/2017/5/3/357
[7] https://lkml.org/lkml/2018/5/11/505
Alexey Gladkov (11):
proc: Rename struct proc_fs_info to proc_fs_opts
proc: add proc_fs_info struct to store proc information
proc: move /proc/{self|thread-self} dentries to proc_fs_info
proc: move hide_pid, pid_gid from pid_namespace to proc_fs_info
proc: add helpers to set and get proc hidepid and gid mount options
proc: support mounting procfs instances inside same pid namespace
proc: flush task dcache entries from all procfs instances
proc: instantiate only pids that we can ptrace on 'hidepid=4' mount
option
proc: add option to mount only a pids subset
docs: proc: add documentation for "hidepid=4" and "subset=pidfs"
options and new mount behavior
proc: Move hidepid values to uapi as they are user interface to mount
Documentation/filesystems/proc.txt | 53 ++++++++++++
fs/locks.c | 6 +-
fs/proc/base.c | 69 ++++++++++++----
fs/proc/generic.c | 9 ++
fs/proc/inode.c | 21 +++--
fs/proc/internal.h | 30 +++++++
fs/proc/root.c | 128 +++++++++++++++++++++++------
fs/proc/self.c | 4 +-
fs/proc/thread_self.c | 6 +-
fs/proc_namespace.c | 14 ++--
include/linux/pid_namespace.h | 54 +++++++++---
include/linux/proc_fs.h | 25 +++++-
include/uapi/linux/proc_fs.h | 13 +++
13 files changed, 355 insertions(+), 77 deletions(-)
create mode 100644 include/uapi/linux/proc_fs.h
--
2.24.1
^ permalink raw reply
* [PATCH v7 11/11] proc: Move hidepid values to uapi as they are user interface to mount
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
Suggested-by: Alexey Dobriyan <adobriyan@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
include/linux/proc_fs.h | 9 +--------
include/uapi/linux/proc_fs.h | 13 +++++++++++++
2 files changed, 14 insertions(+), 8 deletions(-)
create mode 100644 include/uapi/linux/proc_fs.h
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 3ad0a47c3556..f2b4a411d371 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -7,19 +7,12 @@
#include <linux/types.h>
#include <linux/fs.h>
+#include <uapi/linux/proc_fs.h>
struct proc_dir_entry;
struct seq_file;
struct seq_operations;
-/* definitions for hide_pid field */
-enum {
- HIDEPID_OFF = 0,
- HIDEPID_NO_ACCESS = 1,
- HIDEPID_INVISIBLE = 2,
- HIDEPID_NOT_PTRACABLE = 4, /* Limit pids to only ptracable pids */
-};
-
/* definitions for proc mount option pidonly */
enum {
PROC_PIDONLY_OFF = 0,
diff --git a/include/uapi/linux/proc_fs.h b/include/uapi/linux/proc_fs.h
new file mode 100644
index 000000000000..1e3374efffe2
--- /dev/null
+++ b/include/uapi/linux/proc_fs.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
+#ifndef _UAPI_PROC_FS_H
+#define _UAPI_PROC_FS_H
+
+/* definitions for hide_pid field */
+enum {
+ HIDEPID_OFF = 0,
+ HIDEPID_NO_ACCESS = 1,
+ HIDEPID_INVISIBLE = 2,
+ HIDEPID_NOT_PTRACABLE = 4,
+};
+
+#endif
--
2.24.1
^ permalink raw reply related
* [PATCH v7 10/11] docs: proc: add documentation for "hidepid=4" and "subset=pidfs" options and new mount behavior
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
Documentation/filesystems/proc.txt | 53 ++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/Documentation/filesystems/proc.txt b/Documentation/filesystems/proc.txt
index 99ca040e3f90..4741fd092f36 100644
--- a/Documentation/filesystems/proc.txt
+++ b/Documentation/filesystems/proc.txt
@@ -50,6 +50,8 @@ Table of Contents
4 Configuring procfs
4.1 Mount options
+ 5 Filesystem behavior
+
------------------------------------------------------------------------------
Preface
------------------------------------------------------------------------------
@@ -2021,6 +2023,7 @@ The following mount options are supported:
hidepid= Set /proc/<pid>/ access mode.
gid= Set the group authorized to learn processes information.
+ subset= Show only the specified subset of procfs.
hidepid=0 means classic mode - everybody may access all /proc/<pid>/ directories
(default).
@@ -2042,6 +2045,56 @@ information about running processes, whether some daemon runs with elevated
privileges, whether other user runs some sensitive program, whether other users
run any program at all, etc.
+hidepid=4 means that procfs should only contain /proc/<pid>/ directories
+that the caller can ptrace.
+
gid= defines a group authorized to learn processes information otherwise
prohibited by hidepid=. If you use some daemon like identd which needs to learn
information about processes information, just add identd to this group.
+
+subset=pidfs hides all top level files and directories in the procfs that
+are not related to tasks.
+
+------------------------------------------------------------------------------
+5 Filesystem behavior
+------------------------------------------------------------------------------
+
+Originally, before the advent of pid namepsace, procfs was a global file
+system. It means that there was only one procfs instance in the system.
+
+When pid namespace was added, a separate procfs instance was mounted in
+each pid namespace. So, procfs mount options are global among all
+mountpoints within the same namespace.
+
+# grep ^proc /proc/mounts
+proc /proc proc rw,relatime,hidepid=2 0 0
+
+# strace -e mount mount -o hidepid=1 -t proc proc /tmp/proc
+mount("proc", "/tmp/proc", "proc", 0, "hidepid=1") = 0
++++ exited with 0 +++
+
+# grep ^proc /proc/mounts
+proc /proc proc rw,relatime,hidepid=2 0 0
+proc /tmp/proc proc rw,relatime,hidepid=2 0 0
+
+and only after remounting procfs mount options will change at all
+mountpoints.
+
+# mount -o remount,hidepid=1 -t proc proc /tmp/proc
+
+# grep ^proc /proc/mounts
+proc /proc proc rw,relatime,hidepid=1 0 0
+proc /tmp/proc proc rw,relatime,hidepid=1 0 0
+
+This behavior is different from the behavior of other filesystems.
+
+The new procfs behavior is more like other filesystems. Each procfs mount
+creates a new procfs instance. Mount options affect own procfs instance.
+It means that it became possible to have several procfs instances
+displaying tasks with different filtering options in one pid namespace.
+
+# mount -o hidepid=2 -t proc proc /proc
+# mount -o hidepid=1 -t proc proc /tmp/proc
+# grep ^proc /proc/mounts
+proc /proc proc rw,relatime,hidepid=2 0 0
+proc /tmp/proc proc rw,relatime,hidepid=1 0 0
--
2.24.1
^ permalink raw reply related
* [PATCH v7 09/11] proc: add option to mount only a pids subset
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
This allows to hide all files and directories in the procfs that are not
related to tasks.
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/generic.c | 9 +++++++++
fs/proc/inode.c | 7 +++++++
fs/proc/internal.h | 10 ++++++++++
fs/proc/root.c | 36 ++++++++++++++++++++++++++++++++++++
include/linux/proc_fs.h | 7 +++++++
5 files changed, 69 insertions(+)
diff --git a/fs/proc/generic.c b/fs/proc/generic.c
index 64e9ee1b129e..6f6517d63053 100644
--- a/fs/proc/generic.c
+++ b/fs/proc/generic.c
@@ -267,6 +267,11 @@ struct dentry *proc_lookup_de(struct inode *dir, struct dentry *dentry,
struct dentry *proc_lookup(struct inode *dir, struct dentry *dentry,
unsigned int flags)
{
+ struct proc_fs_info *fs_info = proc_sb_info(dir->i_sb);
+
+ if (proc_fs_pidonly(fs_info) == PROC_PIDONLY_ON)
+ return ERR_PTR(-ENOENT);
+
return proc_lookup_de(dir, dentry, PDE(dir));
}
@@ -323,6 +328,10 @@ int proc_readdir_de(struct file *file, struct dir_context *ctx,
int proc_readdir(struct file *file, struct dir_context *ctx)
{
struct inode *inode = file_inode(file);
+ struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
+
+ if (proc_fs_pidonly(fs_info) == PROC_PIDONLY_ON)
+ return 1;
return proc_readdir_de(file, ctx, PDE(inode));
}
diff --git a/fs/proc/inode.c b/fs/proc/inode.c
index 70b722fb8811..f35eef117775 100644
--- a/fs/proc/inode.c
+++ b/fs/proc/inode.c
@@ -114,6 +114,9 @@ static int proc_show_options(struct seq_file *seq, struct dentry *root)
if (hidepid != HIDEPID_OFF)
seq_printf(seq, ",hidepid=%u", hidepid);
+ if (proc_fs_pidonly(fs_info) != PROC_PIDONLY_OFF)
+ seq_printf(seq, ",subset=pidfs");
+
return 0;
}
@@ -333,12 +336,16 @@ proc_reg_get_unmapped_area(struct file *file, unsigned long orig_addr,
static int proc_reg_open(struct inode *inode, struct file *file)
{
+ struct proc_fs_info *fs_info = proc_sb_info(inode->i_sb);
struct proc_dir_entry *pde = PDE(inode);
int rv = 0;
typeof_member(struct file_operations, open) open;
typeof_member(struct file_operations, release) release;
struct pde_opener *pdeo;
+ if (proc_fs_pidonly(fs_info) == PROC_PIDONLY_ON)
+ return -ENOENT;
+
/*
* Ensure that
* 1) PDE's ->release hook will be called no matter what
diff --git a/fs/proc/internal.h b/fs/proc/internal.h
index ff2f274b2e0d..e2c729267317 100644
--- a/fs/proc/internal.h
+++ b/fs/proc/internal.h
@@ -126,6 +126,11 @@ static inline void proc_fs_set_hide_pid(struct proc_fs_info *fs_info, int hide_p
fs_info->hide_pid = hide_pid;
}
+static inline void proc_fs_set_pidonly(struct proc_fs_info *fs_info, int value)
+{
+ fs_info->pidonly = value;
+}
+
static inline void proc_fs_set_pid_gid(struct proc_fs_info *fs_info, kgid_t gid)
{
fs_info->pid_gid = gid;
@@ -141,6 +146,11 @@ static inline kgid_t proc_fs_pid_gid(struct proc_fs_info *fs_info)
return fs_info->pid_gid;
}
+static inline int proc_fs_pidonly(struct proc_fs_info *fs_info)
+{
+ return fs_info->pidonly;
+}
+
void task_dump_owner(struct task_struct *task, umode_t mode,
kuid_t *ruid, kgid_t *rgid);
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 57276cb65528..8e8d5c930e32 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -34,16 +34,19 @@ struct proc_fs_context {
unsigned int mask;
int hidepid;
int gid;
+ int pidonly;
};
enum proc_param {
Opt_gid,
Opt_hidepid,
+ Opt_subset,
};
static const struct fs_parameter_spec proc_param_specs[] = {
fsparam_u32("gid", Opt_gid),
fsparam_u32("hidepid", Opt_hidepid),
+ fsparam_string("subset", Opt_subset),
{}
};
@@ -61,6 +64,30 @@ valid_hidepid(unsigned int value)
value == HIDEPID_NOT_PTRACABLE);
}
+static inline int
+proc_parse_subset_param(struct fs_context *fc, char *value)
+{
+ struct proc_fs_context *ctx = fc->fs_private;
+
+ while (value) {
+ char *ptr = strchr(value, ',');
+
+ if (ptr != NULL)
+ *ptr++ = '\0';
+
+ if (*value != '\0') {
+ if (!strcmp(value, "pidfs")) {
+ ctx->pidonly = PROC_PIDONLY_ON;
+ } else {
+ return invalf(fc, "proc: unsupported subset option - %s\n", value);
+ }
+ }
+ value = ptr;
+ }
+
+ return 0;
+}
+
static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
{
struct proc_fs_context *ctx = fc->fs_private;
@@ -82,6 +109,11 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
ctx->hidepid = result.uint_32;
break;
+ case Opt_subset:
+ if (proc_parse_subset_param(fc, param->string) < 0)
+ return -EINVAL;
+ break;
+
default:
return -EINVAL;
}
@@ -102,6 +134,7 @@ static void proc_apply_options(struct proc_fs_info *fs_info,
proc_fs_set_pid_gid(fs_info, proc_fs_pid_gid(pidns_fs_info));
proc_fs_set_hide_pid(fs_info, proc_fs_hide_pid(pidns_fs_info));
+ proc_fs_set_pidonly(fs_info, proc_fs_pidonly(pidns_fs_info));
}
if (ctx->mask & (1 << Opt_gid))
@@ -109,6 +142,9 @@ static void proc_apply_options(struct proc_fs_info *fs_info,
if (ctx->mask & (1 << Opt_hidepid))
proc_fs_set_hide_pid(fs_info, ctx->hidepid);
+
+ if (ctx->mask & (1 << Opt_subset))
+ proc_fs_set_pidonly(fs_info, ctx->pidonly);
}
static int proc_fill_super(struct super_block *s, struct fs_context *fc)
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index 6822548405a7..3ad0a47c3556 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -20,6 +20,12 @@ enum {
HIDEPID_NOT_PTRACABLE = 4, /* Limit pids to only ptracable pids */
};
+/* definitions for proc mount option pidonly */
+enum {
+ PROC_PIDONLY_OFF = 0,
+ PROC_PIDONLY_ON = 1,
+};
+
struct proc_fs_info {
struct list_head pidns_entry; /* Node in procfs_mounts of a pidns */
struct super_block *m_super;
@@ -28,6 +34,7 @@ struct proc_fs_info {
struct dentry *proc_thread_self; /* For /proc/thread-self */
kgid_t pid_gid;
int hide_pid;
+ int pidonly;
};
static inline struct proc_fs_info *proc_sb_info(struct super_block *sb)
--
2.24.1
^ permalink raw reply related
* [PATCH v7 08/11] proc: instantiate only pids that we can ptrace on 'hidepid=4' mount option
From: Alexey Gladkov @ 2020-01-25 13:05 UTC (permalink / raw)
To: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module
Cc: Akinobu Mita, Alexander Viro, Alexey Dobriyan, Alexey Gladkov,
Andrew Morton, Andy Lutomirski, Daniel Micay, Djalal Harouni,
Dmitry V . Levin, Eric W . Biederman, Greg Kroah-Hartman,
Ingo Molnar, J . Bruce Fields, Jeff Layton, Jonathan Corbet,
Kees Cook, Linus Torvalds, Oleg Nesterov, Solar Designer,
Stephen Rothwell
In-Reply-To: <20200125130541.450409-1-gladkov.alexey@gmail.com>
If "hidepid=4" mount option is set then do not instantiate pids that
we can not ptrace. "hidepid=4" means that procfs should only contain
pids that the caller can ptrace.
Cc: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <tixxdz@gmail.com>
Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
---
fs/proc/base.c | 15 +++++++++++++++
fs/proc/root.c | 14 +++++++++++---
include/linux/proc_fs.h | 1 +
3 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index f4f1bcb28603..b55d205a7f6e 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -699,6 +699,14 @@ static bool has_pid_permissions(struct proc_fs_info *fs_info,
struct task_struct *task,
int hide_pid_min)
{
+ /*
+ * If 'hidpid' mount option is set force a ptrace check,
+ * we indicate that we are using a filesystem syscall
+ * by passing PTRACE_MODE_READ_FSCREDS
+ */
+ if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE)
+ return ptrace_may_access(task, PTRACE_MODE_READ_FSCREDS);
+
if (proc_fs_hide_pid(fs_info) < hide_pid_min)
return true;
if (in_group_p(proc_fs_pid_gid(fs_info)))
@@ -3274,7 +3282,14 @@ struct dentry *proc_pid_lookup(struct dentry *dentry, unsigned int flags)
if (!task)
goto out;
+ /* Limit procfs to only ptracable tasks */
+ if (proc_fs_hide_pid(fs_info) == HIDEPID_NOT_PTRACABLE) {
+ if (!has_pid_permissions(fs_info, task, HIDEPID_NO_ACCESS))
+ goto out_put_task;
+ }
+
result = proc_pid_instantiate(dentry, task, NULL);
+out_put_task:
put_task_struct(task);
out:
return result;
diff --git a/fs/proc/root.c b/fs/proc/root.c
index 3bb8df360cf7..57276cb65528 100644
--- a/fs/proc/root.c
+++ b/fs/proc/root.c
@@ -52,6 +52,15 @@ static const struct fs_parameter_description proc_fs_parameters = {
.specs = proc_param_specs,
};
+static inline int
+valid_hidepid(unsigned int value)
+{
+ return (value == HIDEPID_OFF ||
+ value == HIDEPID_NO_ACCESS ||
+ value == HIDEPID_INVISIBLE ||
+ value == HIDEPID_NOT_PTRACABLE);
+}
+
static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
{
struct proc_fs_context *ctx = fc->fs_private;
@@ -68,10 +77,9 @@ static int proc_parse_param(struct fs_context *fc, struct fs_parameter *param)
break;
case Opt_hidepid:
+ if (!valid_hidepid(result.uint_32))
+ return invalf(fc, "proc: unknown value of hidepid.\n");
ctx->hidepid = result.uint_32;
- if (ctx->hidepid < HIDEPID_OFF ||
- ctx->hidepid > HIDEPID_INVISIBLE)
- return invalf(fc, "proc: hidepid value must be between 0 and 2.\n");
break;
default:
diff --git a/include/linux/proc_fs.h b/include/linux/proc_fs.h
index f307940f8311..6822548405a7 100644
--- a/include/linux/proc_fs.h
+++ b/include/linux/proc_fs.h
@@ -17,6 +17,7 @@ enum {
HIDEPID_OFF = 0,
HIDEPID_NO_ACCESS = 1,
HIDEPID_INVISIBLE = 2,
+ HIDEPID_NOT_PTRACABLE = 4, /* Limit pids to only ptracable pids */
};
struct proc_fs_info {
--
2.24.1
^ 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