* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Ondrej Mosnacek @ 2019-03-18 20:02 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, simo, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <3f02f7b56d2fba2918ff6fe90fcfa3ae558faff8.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
>
> Implement audit container identifier filtering using the AUDIT_CONTID
> field name to send an 8-character string representing a u64 since the
> value field is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
>
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
>
> See: https://github.com/linux-audit/audit-kernel/issues/91
> See: https://github.com/linux-audit/audit-userspace/issues/40
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 ++++-
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 6db5aba7cc01..fa19fa408931 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -77,6 +77,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> + u64 val64;
> kuid_t uid;
> kgid_t gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index a6383e28b2c8..741ab6f38294 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -265,6 +265,7 @@
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> #define AUDIT_FSTYPE 26 /* FileSystem Type */
> +#define AUDIT_CONTID 27 /* Container ID */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -345,6 +346,7 @@ enum {
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID 0x00000080
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -352,7 +354,8 @@ enum {
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 2a1a8b8a8019..3a40b608bf8d 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -230,6 +230,7 @@ static inline int audit_hash_ino(u32 ino)
>
> extern int audit_match_class(int class, unsigned syscall);
> extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index add360b46b38..516b8e58959e 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_CONTID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -582,6 +583,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> + case AUDIT_CONTID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -664,6 +673,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp, audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
How about just:
memcpy(bufp, &f->val64, sizeof(u64));
instead of the awkward for loop? It is simpler and also more in line
with the code in audit_pack_string().
Also, doesn't this loop interfere with the outer loop that also uses
'i' as the control variable?
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result = audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op, f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index aa5d13b4fbbb..2d74238e9638 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
--
Ondrej Mosnacek <omosnace at redhat dot com>
Associate Software Engineer, Security Technologies
Red Hat, Inc.
^ permalink raw reply
* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Neil Horman @ 2019-03-18 20:39 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <3f02f7b56d2fba2918ff6fe90fcfa3ae558faff8.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:56PM -0400, Richard Guy Briggs wrote:
> Implement audit container identifier filtering using the AUDIT_CONTID
> field name to send an 8-character string representing a u64 since the
> value field is only u32.
>
> Sending it as two u32 was considered, but gathering and comparing two
> fields was more complex.
>
> The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
>
> See: https://github.com/linux-audit/audit-kernel/issues/91
> See: https://github.com/linux-audit/audit-userspace/issues/40
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> Acked-by: Serge Hallyn <serge@hallyn.com>
> ---
> include/linux/audit.h | 1 +
> include/uapi/linux/audit.h | 5 ++++-
> kernel/audit.h | 1 +
> kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> kernel/auditsc.c | 3 +++
> 5 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 6db5aba7cc01..fa19fa408931 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -77,6 +77,7 @@ struct audit_field {
> u32 type;
> union {
> u32 val;
> + u64 val64;
> kuid_t uid;
> kgid_t gid;
> struct {
> diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> index a6383e28b2c8..741ab6f38294 100644
> --- a/include/uapi/linux/audit.h
> +++ b/include/uapi/linux/audit.h
> @@ -265,6 +265,7 @@
> #define AUDIT_LOGINUID_SET 24
> #define AUDIT_SESSIONID 25 /* Session ID */
> #define AUDIT_FSTYPE 26 /* FileSystem Type */
> +#define AUDIT_CONTID 27 /* Container ID */
>
> /* These are ONLY useful when checking
> * at syscall exit time (AUDIT_AT_EXIT). */
> @@ -345,6 +346,7 @@ enum {
> #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> +#define AUDIT_FEATURE_BITMAP_CONTAINERID 0x00000080
>
> #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> @@ -352,7 +354,8 @@ enum {
> AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> AUDIT_FEATURE_BITMAP_LOST_RESET | \
> - AUDIT_FEATURE_BITMAP_FILTER_FS)
> + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> + AUDIT_FEATURE_BITMAP_CONTAINERID)
>
> /* deprecated: AUDIT_VERSION_* */
> #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> diff --git a/kernel/audit.h b/kernel/audit.h
> index 2a1a8b8a8019..3a40b608bf8d 100644
> --- a/kernel/audit.h
> +++ b/kernel/audit.h
> @@ -230,6 +230,7 @@ static inline int audit_hash_ino(u32 ino)
>
> extern int audit_match_class(int class, unsigned syscall);
> extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> extern int parent_len(const char *path);
> diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> index add360b46b38..516b8e58959e 100644
> --- a/kernel/auditfilter.c
> +++ b/kernel/auditfilter.c
> @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> /* FALL THROUGH */
> case AUDIT_ARCH:
> case AUDIT_FSTYPE:
> + case AUDIT_CONTID:
> if (f->op != Audit_not_equal && f->op != Audit_equal)
> return -EINVAL;
> break;
> @@ -582,6 +583,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> }
> entry->rule.exe = audit_mark;
> break;
> + case AUDIT_CONTID:
> + if (f->val != sizeof(u64))
> + goto exit_free;
> + str = audit_unpack_string(&bufp, &remain, f->val);
> + if (IS_ERR(str))
> + goto exit_free;
> + f->val64 = ((u64 *)str)[0];
> + break;
> }
> }
>
> @@ -664,6 +673,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> data->buflen += data->values[i] =
> audit_pack_string(&bufp, audit_mark_path(krule->exe));
> break;
> + case AUDIT_CONTID:
> + data->buflen += data->values[i] = sizeof(u64);
> + for (i = 0; i < sizeof(u64); i++)
> + ((char *)bufp)[i] = ((char *)&f->val64)[i];
> + break;
> case AUDIT_LOGINUID_SET:
> if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> data->fields[i] = AUDIT_LOGINUID;
> @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> return 1;
> break;
> + case AUDIT_CONTID:
> + if (a->fields[i].val64 != b->fields[i].val64)
> + return 1;
> + break;
> default:
> if (a->fields[i].val != b->fields[i].val)
> return 1;
> @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> }
> }
>
> +int audit_comparator64(u64 left, u32 op, u64 right)
> +{
> + switch (op) {
> + case Audit_equal:
> + return (left == right);
> + case Audit_not_equal:
> + return (left != right);
> + case Audit_lt:
> + return (left < right);
> + case Audit_le:
> + return (left <= right);
> + case Audit_gt:
> + return (left > right);
> + case Audit_ge:
> + return (left >= right);
> + case Audit_bitmask:
> + return (left & right);
> + case Audit_bittest:
> + return ((left & right) == right);
> + default:
> + BUG();
> + return 0;
> + }
> +}
> +
> int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> {
> switch (op) {
> @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> result = audit_comparator(audit_loginuid_set(current),
> f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(current),
> + f->op, f->val64);
> + break;
> case AUDIT_MSGTYPE:
> result = audit_comparator(msgtype, f->op, f->val);
> break;
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index aa5d13b4fbbb..2d74238e9638 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> case AUDIT_LOGINUID_SET:
> result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> break;
> + case AUDIT_CONTID:
> + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> + break;
> case AUDIT_SUBJ_USER:
> case AUDIT_SUBJ_ROLE:
> case AUDIT_SUBJ_TYPE:
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ghak90 V5 09/10] audit: add support for containerid to network namespaces
From: Neil Horman @ 2019-03-18 20:56 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <27473c84a274c64871cfa8e3636deaf05603c978.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:57PM -0400, Richard Guy Briggs wrote:
> Audit events could happen in a network namespace outside of a task
> context due to packets received from the net that trigger an auditing
> rule prior to being associated with a running task. The network
> namespace could be in use by multiple containers by association to the
> tasks in that network namespace. We still want a way to attribute
> these events to any potential containers. Keep a list per network
> namespace to track these audit container identifiiers.
>
> Add/increment the audit container identifier on:
> - initial setting of the audit container identifier via /proc
> - clone/fork call that inherits an audit container identifier
> - unshare call that inherits an audit container identifier
> - setns call that inherits an audit container identifier
> Delete/decrement the audit container identifier on:
> - an inherited audit container identifier dropped when child set
> - process exit
> - unshare call that drops a net namespace
> - setns call that drops a net namespace
>
> See: https://github.com/linux-audit/audit-kernel/issues/92
> See: https://github.com/linux-audit/audit-testsuite/issues/64
> See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 19 ++++++++++++
> kernel/audit.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++++--
> kernel/nsproxy.c | 4 +++
> 3 files changed, 106 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index fa19fa408931..70255c2dfb9f 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -27,6 +27,7 @@
> #include <linux/ptrace.h>
> #include <linux/namei.h> /* LOOKUP_* */
> #include <uapi/linux/audit.h>
> +#include <linux/refcount.h>
>
> #define AUDIT_INO_UNSET ((unsigned long)-1)
> #define AUDIT_DEV_UNSET ((dev_t)-1)
> @@ -99,6 +100,13 @@ struct audit_task_info {
>
> extern struct audit_task_info init_struct_audit;
>
> +struct audit_contid {
> + struct list_head list;
> + u64 id;
> + refcount_t refcount;
> + struct rcu_head rcu;
> +};
> +
> extern int is_audit_feature_set(int which);
>
> extern int __init audit_register_class(int class, unsigned *list);
> @@ -202,6 +210,10 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> }
>
> extern void audit_log_contid(struct audit_context *context, u64 contid);
> +extern void audit_netns_contid_add(struct net *net, u64 contid);
> +extern void audit_netns_contid_del(struct net *net, u64 contid);
> +extern void audit_switch_task_namespaces(struct nsproxy *ns,
> + struct task_struct *p);
>
> extern u32 audit_enabled;
> #else /* CONFIG_AUDIT */
> @@ -271,6 +283,13 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
>
> static inline void audit_log_contid(struct audit_context *context, u64 contid)
> { }
> +static inline void audit_netns_contid_add(struct net *net, u64 contid)
> +{ }
> +static inline void audit_netns_contid_del(struct net *net, u64 contid)
> +{ }
> +static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> + struct task_struct *p)
> +{ }
>
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index cf448599ef34..7fa3194f5342 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -72,6 +72,7 @@
> #include <linux/freezer.h>
> #include <linux/pid_namespace.h>
> #include <net/netns/generic.h>
> +#include <net/net_namespace.h>
>
> #include "audit.h"
>
> @@ -99,9 +100,13 @@
> /**
> * struct audit_net - audit private network namespace data
> * @sk: communication socket
> + * @contid_list: audit container identifier list
> + * @contid_list_lock audit container identifier list lock
> */
> struct audit_net {
> struct sock *sk;
> + struct list_head contid_list;
> + spinlock_t contid_list_lock;
> };
>
> /**
> @@ -275,8 +280,11 @@ struct audit_task_info init_struct_audit = {
> void audit_free(struct task_struct *tsk)
> {
> struct audit_task_info *info = tsk->audit;
> + struct nsproxy *ns = tsk->nsproxy;
>
> audit_free_syscall(tsk);
> + if (ns)
> + audit_netns_contid_del(ns->net_ns, audit_get_contid(tsk));
> /* Freeing the audit_task_info struct must be performed after
> * audit_log_exit() due to need for loginuid and sessionid.
> */
> @@ -376,6 +384,73 @@ static struct sock *audit_get_sk(const struct net *net)
> return aunet->sk;
> }
>
> +void audit_netns_contid_add(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet = net_generic(net, audit_net_id);
> + struct list_head *contid_list = &aunet->contid_list;
> + struct audit_contid *cont;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + if (!aunet)
> + return;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + refcount_inc(&cont->refcount);
> + goto out;
> + }
> + cont = kmalloc(sizeof(struct audit_contid), GFP_ATOMIC);
> + if (cont) {
> + INIT_LIST_HEAD(&cont->list);
> + cont->id = contid;
> + refcount_set(&cont->refcount, 1);
> + list_add_rcu(&cont->list, contid_list);
> + }
> +out:
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_netns_contid_del(struct net *net, u64 contid)
> +{
> + struct audit_net *aunet;
> + struct list_head *contid_list;
> + struct audit_contid *cont = NULL;
> +
> + if (!net)
> + return;
> + if (!audit_contid_valid(contid))
> + return;
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + return;
> + contid_list = &aunet->contid_list;
> + spin_lock(&aunet->contid_list_lock);
> + if (!list_empty(contid_list))
> + list_for_each_entry_rcu(cont, contid_list, list)
> + if (cont->id == contid) {
> + if (refcount_dec_and_test(&cont->refcount)) {
> + list_del_rcu(&cont->list);
> + kfree_rcu(cont, rcu);
> + }
> + break;
> + }
> + spin_unlock(&aunet->contid_list_lock);
> +}
> +
> +void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> +{
> + u64 contid = audit_get_contid(p);
> + struct nsproxy *new = p->nsproxy;
> +
> + if (!audit_contid_valid(contid))
> + return;
> + audit_netns_contid_del(ns->net_ns, contid);
> + if (new)
> + audit_netns_contid_add(new->net_ns, contid);
> +}
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> @@ -1619,7 +1694,6 @@ static int __net_init audit_net_init(struct net *net)
> .flags = NL_CFG_F_NONROOT_RECV,
> .groups = AUDIT_NLGRP_MAX,
> };
> -
> struct audit_net *aunet = net_generic(net, audit_net_id);
>
> aunet->sk = netlink_kernel_create(net, NETLINK_AUDIT, &cfg);
> @@ -1628,7 +1702,8 @@ static int __net_init audit_net_init(struct net *net)
> return -ENOMEM;
> }
> aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
> -
> + INIT_LIST_HEAD(&aunet->contid_list);
> + spin_lock_init(&aunet->contid_list_lock);
> return 0;
> }
>
> @@ -2380,6 +2455,7 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> uid_t uid;
> struct tty_struct *tty;
> char comm[sizeof(current->comm)];
> + struct net *net = task->nsproxy->net_ns;
>
> task_lock(task);
> /* Can't set if audit disabled */
> @@ -2401,8 +2477,12 @@ int audit_set_contid(struct task_struct *task, u64 contid)
> else if (!(thread_group_leader(task) && thread_group_empty(task)))
> rc = -EALREADY;
> read_unlock(&tasklist_lock);
> - if (!rc)
> + if (!rc) {
> + if (audit_contid_valid(oldcontid))
> + audit_netns_contid_del(net, oldcontid);
> task->audit->contid = contid;
> + audit_netns_contid_add(net, contid);
> + }
> task_unlock(task);
>
> if (!audit_enabled)
> diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
> index f6c5d330059a..718b1201ae70 100644
> --- a/kernel/nsproxy.c
> +++ b/kernel/nsproxy.c
> @@ -27,6 +27,7 @@
> #include <linux/syscalls.h>
> #include <linux/cgroup.h>
> #include <linux/perf_event.h>
> +#include <linux/audit.h>
>
> static struct kmem_cache *nsproxy_cachep;
>
> @@ -140,6 +141,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> struct nsproxy *old_ns = tsk->nsproxy;
> struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
> struct nsproxy *new_ns;
> + u64 contid = audit_get_contid(tsk);
>
> if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
> CLONE_NEWPID | CLONE_NEWNET |
> @@ -167,6 +169,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
> return PTR_ERR(new_ns);
>
> tsk->nsproxy = new_ns;
> + audit_netns_contid_add(new_ns->net_ns, contid);
> return 0;
> }
>
> @@ -224,6 +227,7 @@ void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
> ns = p->nsproxy;
> p->nsproxy = new;
> task_unlock(p);
> + audit_switch_task_namespaces(ns, p);
>
> if (ns && atomic_dec_and_test(&ns->count))
> free_nsproxy(ns);
> --
> 1.8.3.1
>
>
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [PATCH ghak90 V5 10/10] audit: NETFILTER_PKT: record each container ID associated with a netNS
From: Neil Horman @ 2019-03-18 20:58 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, sgrubb, omosnace,
dhowells, simo, eparis, serge, ebiederm
In-Reply-To: <56127b2a5b82f15cb0d0f040502c2e3bb6945f30.1552665316.git.rgb@redhat.com>
On Fri, Mar 15, 2019 at 02:29:58PM -0400, Richard Guy Briggs wrote:
> Add audit container identifier auxiliary record(s) to NETFILTER_PKT
> event standalone records. Iterate through all potential audit container
> identifiers associated with a network namespace.
>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
> include/linux/audit.h | 5 +++++
> kernel/audit.c | 41 +++++++++++++++++++++++++++++++++++++++++
> net/netfilter/nft_log.c | 11 +++++++++--
> net/netfilter/xt_AUDIT.c | 11 +++++++++--
> 4 files changed, 64 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/audit.h b/include/linux/audit.h
> index 70255c2dfb9f..723e2d020228 100644
> --- a/include/linux/audit.h
> +++ b/include/linux/audit.h
> @@ -214,6 +214,8 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
> extern void audit_netns_contid_del(struct net *net, u64 contid);
> extern void audit_switch_task_namespaces(struct nsproxy *ns,
> struct task_struct *p);
> +extern void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context);
>
> extern u32 audit_enabled;
> #else /* CONFIG_AUDIT */
> @@ -290,6 +292,9 @@ static inline void audit_netns_contid_del(struct net *net, u64 contid)
> static inline void audit_switch_task_namespaces(struct nsproxy *ns,
> struct task_struct *p)
> { }
> +static inline void audit_log_netns_contid_list(struct net *net,
> + struct audit_context *context)
> +{ }
>
> #define audit_enabled AUDIT_OFF
> #endif /* CONFIG_AUDIT */
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 7fa3194f5342..80ed323feeb5 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -451,6 +451,47 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
> audit_netns_contid_add(new->net_ns, contid);
> }
>
> +/**
> + * audit_log_netns_contid_list - List contids for the given network namespace
> + * @net: the network namespace of interest
> + * @context: the audit context to use
> + *
> + * Description:
> + * Issues a CONTAINER_ID record with a CSV list of contids associated
> + * with a network namespace to accompany a NETFILTER_PKT record.
> + */
> +void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
> +{
> + struct audit_buffer *ab = NULL;
> + struct audit_contid *cont;
> + bool first = true;
> + struct audit_net *aunet;
> +
> + /* Generate AUDIT_CONTAINER_ID record with container ID CSV list */
> + rcu_read_lock();
> + aunet = net_generic(net, audit_net_id);
> + if (!aunet)
> + goto out;
> + list_for_each_entry_rcu(cont, &aunet->contid_list, list) {
> + if (first) {
> + ab = audit_log_start(context, GFP_ATOMIC,
> + AUDIT_CONTAINER_ID);
> + if (!ab) {
> + audit_log_lost("out of memory in audit_log_netns_contid_list");
> + goto out;
> + }
> + audit_log_format(ab, "contid=");
> + } else
> + audit_log_format(ab, ",");
> + audit_log_format(ab, "%llu", cont->id);
> + first = false;
> + }
> + audit_log_end(ab);
> +out:
> + rcu_read_unlock();
> +}
> +EXPORT_SYMBOL(audit_log_netns_contid_list);
> +
> void audit_panic(const char *message)
> {
> switch (audit_failure) {
> diff --git a/net/netfilter/nft_log.c b/net/netfilter/nft_log.c
> index 655187bed5d8..bdb1ec2368a7 100644
> --- a/net/netfilter/nft_log.c
> +++ b/net/netfilter/nft_log.c
> @@ -69,13 +69,16 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> struct sk_buff *skb = pkt->skb;
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (!audit_enabled)
> return;
>
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (!ab)
> - return;
> + goto errout;
>
> audit_log_format(ab, "mark=%#x", skb->mark);
>
> @@ -102,6 +105,10 @@ static void nft_log_eval_audit(const struct nft_pktinfo *pkt)
> audit_log_format(ab, " saddr=? daddr=? proto=-1");
>
> audit_log_end(ab);
> + net = xt_net(&pkt->xt);
> + audit_log_netns_contid_list(net, context);
> +errout:
> + audit_free_context(context);
> }
>
> static void nft_log_eval(const struct nft_expr *expr,
> diff --git a/net/netfilter/xt_AUDIT.c b/net/netfilter/xt_AUDIT.c
> index af883f1b64f9..a3e547435f13 100644
> --- a/net/netfilter/xt_AUDIT.c
> +++ b/net/netfilter/xt_AUDIT.c
> @@ -71,10 +71,13 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
> {
> struct audit_buffer *ab;
> int fam = -1;
> + struct audit_context *context;
> + struct net *net;
>
> if (audit_enabled == AUDIT_OFF)
> - goto errout;
> - ab = audit_log_start(NULL, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> + goto out;
> + context = audit_alloc_local(GFP_ATOMIC);
> + ab = audit_log_start(context, GFP_ATOMIC, AUDIT_NETFILTER_PKT);
> if (ab == NULL)
> goto errout;
>
> @@ -104,7 +107,11 @@ static bool audit_ip6(struct audit_buffer *ab, struct sk_buff *skb)
>
> audit_log_end(ab);
>
> + net = xt_net(par);
> + audit_log_netns_contid_list(net, context);
> errout:
> + audit_free_context(context);
> +out:
> return XT_CONTINUE;
> }
>
> --
> 1.8.3.1
>
>
minus the whitespace fix
Acked-by: Neil Horman <nhorman@tuxdriver.com>
^ permalink raw reply
* Re: [RFC PATCH v3 07/18] fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY ioctl
From: Eric Biggers @ 2019-03-18 23:08 UTC (permalink / raw)
To: Richard Weinberger
Cc: linux-ext4, open list:ABI/API, linux-f2fs-devel, linux-fscrypt,
keyrings, linux-mtd, linux-crypto, linux-fsdevel, Satya Tangirala,
Paul Crowley
In-Reply-To: <20190221184203.GB140206@gmail.com>
Hi Richard,
On Thu, Feb 21, 2019 at 10:42:03AM -0800, Eric Biggers wrote:
> On Thu, Feb 21, 2019 at 10:33:12AM +0100, Richard Weinberger wrote:
> > Eric,
> >
> > Am Donnerstag, 21. Februar 2019, 06:49:39 CET schrieb Eric Biggers:
> > > Hi Richard,
> > >
> > > On Thu, Feb 21, 2019 at 12:52:38AM +0100, Richard Weinberger wrote:
> > > > On Wed, Feb 20, 2019 at 7:55 AM Eric Biggers <ebiggers@kernel.org> wrote:
> > > > > +#define FSCRYPT_FS_KEYRING_DESCRIPTION_SIZE \
> > > > > + (CONST_STRLEN("fscrypt-") + FIELD_SIZEOF(struct super_block, s_id))
> > > > > +
> > > > > +#define FSCRYPT_MK_DESCRIPTION_SIZE (2 * FSCRYPT_KEY_DESCRIPTOR_SIZE + 1)
> > > > > +
> > > > > +static void format_fs_keyring_description(
> > > > > + char description[FSCRYPT_FS_KEYRING_DESCRIPTION_SIZE],
> > > > > + const struct super_block *sb)
> > > > > +{
> > > > > + sprintf(description, "fscrypt-%s", sb->s_id);
> > > > > +}
> > > >
> > > > I fear ->s_id is not the right thing.
> > > > For filesystems such as ext4 ->s_id is the name of the backing block device,
> > > > so it is per filesysem instance unique.
> > > > But this is not guaranteed. For UBIFS ->s_id is just "ubifs", always.
> > > > So the names will clash.
> > > >
> > >
> > > What name do you suggest using for UBIFS filesystems? The keyring name could be
> > > set by the filesystem via a fscrypt_operations callback if needed.
> >
> > IMHO the BDI name should be used.
I tried using sb->s_bdi->name, but it's still "ubifs" for all UBIFS filesystems.
Perhaps there's a way you can make ->s_id for UBIFS unique? There are already
existing places that log ->s_id, so perhaps you should do it anyway regardless
of this patchset?
> >
> > > Note that the keyring name isn't particularly important, since the ioctls will
> > > work regardless. But we might as well choose something logical, since the
> > > keyring name will still show up in /proc/keys.
> >
> > I'm not done with reviewing your patches, but will it be possible to use keyctl?
> > For the a unique name is helpful. :)
> >
>
> Not for adding keys, removing keys, or getting a key's status -- those are what
> the ioctls are for.
>
> See e.g. the discussion in patch 7 ("fscrypt: add FS_IOC_ADD_ENCRYPTION_KEY
> ioctl") for why the keyrings syscalls are a poor fit for fscrypt.
>
Anyway, perhaps I should reconsider whether fscrypt should even use the keyrings
subsystem at all, even just "internally", as its quirks still leak out a bit.
I'd prefer a nice clean API without any quirks like having to name the keyrings
and assign SELinux labels to the keys just to make the keyrings subsystem happy.
- Eric
^ permalink raw reply
* Re: [PATCH ghak90 V5 08/10] audit: add containerid filtering
From: Richard Guy Briggs @ 2019-03-18 23:47 UTC (permalink / raw)
To: Ondrej Mosnacek
Cc: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel, Paul Moore, Steve Grubb,
David Howells, simo, Eric Paris, Serge E. Hallyn,
Eric W . Biederman, nhorman
In-Reply-To: <CAFqZXNt0O8b54isxuXv1fbtv+UKRw0Lnyut2n0FGFEMme-SdkQ@mail.gmail.com>
On 2019-03-18 21:02, Ondrej Mosnacek wrote:
> On Fri, Mar 15, 2019 at 7:35 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> >
> > Implement audit container identifier filtering using the AUDIT_CONTID
> > field name to send an 8-character string representing a u64 since the
> > value field is only u32.
> >
> > Sending it as two u32 was considered, but gathering and comparing two
> > fields was more complex.
> >
> > The feature indicator is AUDIT_FEATURE_BITMAP_CONTAINERID.
> >
> > See: https://github.com/linux-audit/audit-kernel/issues/91
> > See: https://github.com/linux-audit/audit-userspace/issues/40
> > See: https://github.com/linux-audit/audit-testsuite/issues/64
> > See: https://github.com/linux-audit/audit-kernel/wiki/RFE-Audit-Container-ID
> > Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> > Acked-by: Serge Hallyn <serge@hallyn.com>
> > ---
> > include/linux/audit.h | 1 +
> > include/uapi/linux/audit.h | 5 ++++-
> > kernel/audit.h | 1 +
> > kernel/auditfilter.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++
> > kernel/auditsc.c | 3 +++
> > 5 files changed, 56 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/audit.h b/include/linux/audit.h
> > index 6db5aba7cc01..fa19fa408931 100644
> > --- a/include/linux/audit.h
> > +++ b/include/linux/audit.h
> > @@ -77,6 +77,7 @@ struct audit_field {
> > u32 type;
> > union {
> > u32 val;
> > + u64 val64;
> > kuid_t uid;
> > kgid_t gid;
> > struct {
> > diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
> > index a6383e28b2c8..741ab6f38294 100644
> > --- a/include/uapi/linux/audit.h
> > +++ b/include/uapi/linux/audit.h
> > @@ -265,6 +265,7 @@
> > #define AUDIT_LOGINUID_SET 24
> > #define AUDIT_SESSIONID 25 /* Session ID */
> > #define AUDIT_FSTYPE 26 /* FileSystem Type */
> > +#define AUDIT_CONTID 27 /* Container ID */
> >
> > /* These are ONLY useful when checking
> > * at syscall exit time (AUDIT_AT_EXIT). */
> > @@ -345,6 +346,7 @@ enum {
> > #define AUDIT_FEATURE_BITMAP_SESSIONID_FILTER 0x00000010
> > #define AUDIT_FEATURE_BITMAP_LOST_RESET 0x00000020
> > #define AUDIT_FEATURE_BITMAP_FILTER_FS 0x00000040
> > +#define AUDIT_FEATURE_BITMAP_CONTAINERID 0x00000080
> >
> > #define AUDIT_FEATURE_BITMAP_ALL (AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT | \
> > AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME | \
> > @@ -352,7 +354,8 @@ enum {
> > AUDIT_FEATURE_BITMAP_EXCLUDE_EXTEND | \
> > AUDIT_FEATURE_BITMAP_SESSIONID_FILTER | \
> > AUDIT_FEATURE_BITMAP_LOST_RESET | \
> > - AUDIT_FEATURE_BITMAP_FILTER_FS)
> > + AUDIT_FEATURE_BITMAP_FILTER_FS | \
> > + AUDIT_FEATURE_BITMAP_CONTAINERID)
> >
> > /* deprecated: AUDIT_VERSION_* */
> > #define AUDIT_VERSION_LATEST AUDIT_FEATURE_BITMAP_ALL
> > diff --git a/kernel/audit.h b/kernel/audit.h
> > index 2a1a8b8a8019..3a40b608bf8d 100644
> > --- a/kernel/audit.h
> > +++ b/kernel/audit.h
> > @@ -230,6 +230,7 @@ static inline int audit_hash_ino(u32 ino)
> >
> > extern int audit_match_class(int class, unsigned syscall);
> > extern int audit_comparator(const u32 left, const u32 op, const u32 right);
> > +extern int audit_comparator64(const u64 left, const u32 op, const u64 right);
> > extern int audit_uid_comparator(kuid_t left, u32 op, kuid_t right);
> > extern int audit_gid_comparator(kgid_t left, u32 op, kgid_t right);
> > extern int parent_len(const char *path);
> > diff --git a/kernel/auditfilter.c b/kernel/auditfilter.c
> > index add360b46b38..516b8e58959e 100644
> > --- a/kernel/auditfilter.c
> > +++ b/kernel/auditfilter.c
> > @@ -410,6 +410,7 @@ static int audit_field_valid(struct audit_entry *entry, struct audit_field *f)
> > /* FALL THROUGH */
> > case AUDIT_ARCH:
> > case AUDIT_FSTYPE:
> > + case AUDIT_CONTID:
> > if (f->op != Audit_not_equal && f->op != Audit_equal)
> > return -EINVAL;
> > break;
> > @@ -582,6 +583,14 @@ static struct audit_entry *audit_data_to_entry(struct audit_rule_data *data,
> > }
> > entry->rule.exe = audit_mark;
> > break;
> > + case AUDIT_CONTID:
> > + if (f->val != sizeof(u64))
> > + goto exit_free;
> > + str = audit_unpack_string(&bufp, &remain, f->val);
> > + if (IS_ERR(str))
> > + goto exit_free;
> > + f->val64 = ((u64 *)str)[0];
> > + break;
> > }
> > }
> >
> > @@ -664,6 +673,11 @@ static struct audit_rule_data *audit_krule_to_data(struct audit_krule *krule)
> > data->buflen += data->values[i] =
> > audit_pack_string(&bufp, audit_mark_path(krule->exe));
> > break;
> > + case AUDIT_CONTID:
> > + data->buflen += data->values[i] = sizeof(u64);
> > + for (i = 0; i < sizeof(u64); i++)
> > + ((char *)bufp)[i] = ((char *)&f->val64)[i];
>
> How about just:
>
> memcpy(bufp, &f->val64, sizeof(u64));
>
> instead of the awkward for loop? It is simpler and also more in line
> with the code in audit_pack_string().
I'll grant you that. I'll use that instead.
> Also, doesn't this loop interfere with the outer loop that also uses
> 'i' as the control variable?
Yes, it does. Thank you for catching that. Doing a auditctl -l reveals
this.
> > + break;
> > case AUDIT_LOGINUID_SET:
> > if (krule->pflags & AUDIT_LOGINUID_LEGACY && !f->val) {
> > data->fields[i] = AUDIT_LOGINUID;
> > @@ -750,6 +764,10 @@ static int audit_compare_rule(struct audit_krule *a, struct audit_krule *b)
> > if (!gid_eq(a->fields[i].gid, b->fields[i].gid))
> > return 1;
> > break;
> > + case AUDIT_CONTID:
> > + if (a->fields[i].val64 != b->fields[i].val64)
> > + return 1;
> > + break;
> > default:
> > if (a->fields[i].val != b->fields[i].val)
> > return 1;
> > @@ -1206,6 +1224,31 @@ int audit_comparator(u32 left, u32 op, u32 right)
> > }
> > }
> >
> > +int audit_comparator64(u64 left, u32 op, u64 right)
> > +{
> > + switch (op) {
> > + case Audit_equal:
> > + return (left == right);
> > + case Audit_not_equal:
> > + return (left != right);
> > + case Audit_lt:
> > + return (left < right);
> > + case Audit_le:
> > + return (left <= right);
> > + case Audit_gt:
> > + return (left > right);
> > + case Audit_ge:
> > + return (left >= right);
> > + case Audit_bitmask:
> > + return (left & right);
> > + case Audit_bittest:
> > + return ((left & right) == right);
> > + default:
> > + BUG();
> > + return 0;
> > + }
> > +}
> > +
> > int audit_uid_comparator(kuid_t left, u32 op, kuid_t right)
> > {
> > switch (op) {
> > @@ -1344,6 +1387,10 @@ int audit_filter(int msgtype, unsigned int listtype)
> > result = audit_comparator(audit_loginuid_set(current),
> > f->op, f->val);
> > break;
> > + case AUDIT_CONTID:
> > + result = audit_comparator64(audit_get_contid(current),
> > + f->op, f->val64);
> > + break;
> > case AUDIT_MSGTYPE:
> > result = audit_comparator(msgtype, f->op, f->val);
> > break;
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index aa5d13b4fbbb..2d74238e9638 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -616,6 +616,9 @@ static int audit_filter_rules(struct task_struct *tsk,
> > case AUDIT_LOGINUID_SET:
> > result = audit_comparator(audit_loginuid_set(tsk), f->op, f->val);
> > break;
> > + case AUDIT_CONTID:
> > + result = audit_comparator64(audit_get_contid(tsk), f->op, f->val64);
> > + break;
> > case AUDIT_SUBJ_USER:
> > case AUDIT_SUBJ_ROLE:
> > case AUDIT_SUBJ_TYPE:
> > --
> > 1.8.3.1
> >
>
>
> --
> Ondrej Mosnacek <omosnace at redhat dot com>
> Associate Software Engineer, Security Technologies
> Red Hat, Inc.
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* [PATCH v2 0/1] userfaultfd: allow to forbid unprivileged users
From: Peter Xu @ 2019-03-19 3:07 UTC (permalink / raw)
To: linux-kernel
Cc: Paolo Bonzini, Hugh Dickins, Luis Chamberlain, Maxime Coquelin,
Maya Gokhale, Jerome Glisse, Pavel Emelyanov, Johannes Weiner,
peterx, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Mike Kravetz, Andrea Arcangeli, Mike Rapoport,
Kees Cook, Mel Gorman, Kirill A . Shutemov, linux-api,
linux-fsdevel, Dr
Hi,
This is the second version of the work. V1 was here:
https://lkml.org/lkml/2019/3/11/207
I removed CC to kvm list since not necessary any more, but added
linux-api to the list as suggested by Kirill.
This one greatly simplifies the previous version, dropped the kvm
special entry and mimic the sysctl_unprivileged_bpf_disabled knob for
userfaultfd as suggested by many. The major differences comparing to
the BPF flag are: (1) use PTRACE instead of ADMIN capability, and (2)
allow to switch the flag back and forth (BPF does not allow to switch
back to "enabled" if "disabled" once).
So the main idea of this simpler version is that we still keep the old
way as is by default but we only provide a way for admins when they
really want to turn userfaultfd off for unprivileged users.
About procfs vs sysfs: I still used the procfs way because admins can
still leverage sysctl.conf with that and also since no one yet
explicitly asked for sysfs for a better reason yet (And I just noticed
BPF just added another bpf_stats_enabled into sysctl a few weeks ago).
Please have a look, thanks.
Peter Xu (1):
userfaultfd/sysctl: add vm.unprivileged_userfaultfd
Documentation/sysctl/vm.txt | 12 ++++++++++++
fs/userfaultfd.c | 5 +++++
include/linux/userfaultfd_k.h | 2 ++
kernel/sysctl.c | 12 ++++++++++++
4 files changed, 31 insertions(+)
--
2.17.1
^ permalink raw reply
* [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Peter Xu @ 2019-03-19 3:07 UTC (permalink / raw)
To: linux-kernel
Cc: Paolo Bonzini, Hugh Dickins, Luis Chamberlain, Maxime Coquelin,
Maya Gokhale, Jerome Glisse, Pavel Emelyanov, Johannes Weiner,
peterx, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Mike Kravetz, Andrea Arcangeli, Mike Rapoport,
Kees Cook, Mel Gorman, Kirill A . Shutemov, linux-api,
linux-fsdevel, Dr
In-Reply-To: <20190319030722.12441-1-peterx@redhat.com>
Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
whether userfaultfd is allowed by unprivileged users. When this is
set to zero, only privileged users (root user, or users with the
CAP_SYS_PTRACE capability) will be able to use the userfaultfd
syscalls.
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Suggested-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
Documentation/sysctl/vm.txt | 12 ++++++++++++
fs/userfaultfd.c | 5 +++++
include/linux/userfaultfd_k.h | 2 ++
kernel/sysctl.c | 12 ++++++++++++
4 files changed, 31 insertions(+)
diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
index 187ce4f599a2..f146712f67bb 100644
--- a/Documentation/sysctl/vm.txt
+++ b/Documentation/sysctl/vm.txt
@@ -61,6 +61,7 @@ Currently, these files are in /proc/sys/vm:
- stat_refresh
- numa_stat
- swappiness
+- unprivileged_userfaultfd
- user_reserve_kbytes
- vfs_cache_pressure
- watermark_boost_factor
@@ -818,6 +819,17 @@ The default value is 60.
==============================================================
+unprivileged_userfaultfd
+
+This flag controls whether unprivileged users can use the userfaultfd
+syscalls. Set this to 1 to allow unprivileged users to use the
+userfaultfd syscalls, or set this to 0 to restrict userfaultfd to only
+privileged users (with SYS_CAP_PTRACE capability).
+
+The default value is 1.
+
+==============================================================
+
- user_reserve_kbytes
When overcommit_memory is set to 2, "never overcommit" mode, reserve
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 89800fc7dc9d..7e856a25cc2f 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -30,6 +30,8 @@
#include <linux/security.h>
#include <linux/hugetlb.h>
+int sysctl_unprivileged_userfaultfd __read_mostly = 1;
+
static struct kmem_cache *userfaultfd_ctx_cachep __read_mostly;
enum userfaultfd_state {
@@ -1921,6 +1923,9 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
struct userfaultfd_ctx *ctx;
int fd;
+ if (!sysctl_unprivileged_userfaultfd && !capable(CAP_SYS_PTRACE))
+ return -EPERM;
+
BUG_ON(!current->mm);
/* Check the UFFD_* constants for consistency. */
diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
index 37c9eba75c98..ac9d71e24b81 100644
--- a/include/linux/userfaultfd_k.h
+++ b/include/linux/userfaultfd_k.h
@@ -28,6 +28,8 @@
#define UFFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
#define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
+extern int sysctl_unprivileged_userfaultfd;
+
extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason);
extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start,
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 7578e21a711b..9b8ff1881df9 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -66,6 +66,7 @@
#include <linux/kexec.h>
#include <linux/bpf.h>
#include <linux/mount.h>
+#include <linux/userfaultfd_k.h>
#include <linux/uaccess.h>
#include <asm/processor.h>
@@ -1704,6 +1705,17 @@ static struct ctl_table vm_table[] = {
.extra1 = (void *)&mmap_rnd_compat_bits_min,
.extra2 = (void *)&mmap_rnd_compat_bits_max,
},
+#endif
+#ifdef CONFIG_USERFAULTFD
+ {
+ .procname = "unprivileged_userfaultfd",
+ .data = &sysctl_unprivileged_userfaultfd,
+ .maxlen = sizeof(sysctl_unprivileged_userfaultfd),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &zero,
+ .extra2 = &one,
+ },
#endif
{ }
};
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Mike Rapoport @ 2019-03-19 7:11 UTC (permalink / raw)
To: Peter Xu
Cc: linux-kernel, Paolo Bonzini, Hugh Dickins, Luis Chamberlain,
Maxime Coquelin, Maya Gokhale, Jerome Glisse, Pavel Emelyanov,
Johannes Weiner, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Mike Kravetz, Andrea Arcangeli, Mike Rapoport,
Kees Cook, Mel Gorman, Kirill A . Shutemov, linux-api,
linux-fsdevel
In-Reply-To: <20190319030722.12441-2-peterx@redhat.com>
Hi Peter,
On Tue, Mar 19, 2019 at 11:07:22AM +0800, Peter Xu wrote:
> Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
> whether userfaultfd is allowed by unprivileged users. When this is
> set to zero, only privileged users (root user, or users with the
> CAP_SYS_PTRACE capability) will be able to use the userfaultfd
> syscalls.
>
> Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
> Suggested-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
Just one minor note below
> ---
> Documentation/sysctl/vm.txt | 12 ++++++++++++
> fs/userfaultfd.c | 5 +++++
> include/linux/userfaultfd_k.h | 2 ++
> kernel/sysctl.c | 12 ++++++++++++
> 4 files changed, 31 insertions(+)
>
> diff --git a/Documentation/sysctl/vm.txt b/Documentation/sysctl/vm.txt
> index 187ce4f599a2..f146712f67bb 100644
> --- a/Documentation/sysctl/vm.txt
> +++ b/Documentation/sysctl/vm.txt
> @@ -61,6 +61,7 @@ Currently, these files are in /proc/sys/vm:
> - stat_refresh
> - numa_stat
> - swappiness
> +- unprivileged_userfaultfd
> - user_reserve_kbytes
> - vfs_cache_pressure
> - watermark_boost_factor
> @@ -818,6 +819,17 @@ The default value is 60.
>
> ==============================================================
>
> +unprivileged_userfaultfd
> +
> +This flag controls whether unprivileged users can use the userfaultfd
> +syscalls. Set this to 1 to allow unprivileged users to use the
> +userfaultfd syscalls, or set this to 0 to restrict userfaultfd to only
> +privileged users (with SYS_CAP_PTRACE capability).
Can you please fully spell "system call"?
> +
> +The default value is 1.
> +
> +==============================================================
> +
> - user_reserve_kbytes
>
> When overcommit_memory is set to 2, "never overcommit" mode, reserve
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index 89800fc7dc9d..7e856a25cc2f 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -30,6 +30,8 @@
> #include <linux/security.h>
> #include <linux/hugetlb.h>
>
> +int sysctl_unprivileged_userfaultfd __read_mostly = 1;
> +
> static struct kmem_cache *userfaultfd_ctx_cachep __read_mostly;
>
> enum userfaultfd_state {
> @@ -1921,6 +1923,9 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
> struct userfaultfd_ctx *ctx;
> int fd;
>
> + if (!sysctl_unprivileged_userfaultfd && !capable(CAP_SYS_PTRACE))
> + return -EPERM;
> +
> BUG_ON(!current->mm);
>
> /* Check the UFFD_* constants for consistency. */
> diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> index 37c9eba75c98..ac9d71e24b81 100644
> --- a/include/linux/userfaultfd_k.h
> +++ b/include/linux/userfaultfd_k.h
> @@ -28,6 +28,8 @@
> #define UFFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
> #define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
>
> +extern int sysctl_unprivileged_userfaultfd;
> +
> extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason);
>
> extern ssize_t mcopy_atomic(struct mm_struct *dst_mm, unsigned long dst_start,
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 7578e21a711b..9b8ff1881df9 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -66,6 +66,7 @@
> #include <linux/kexec.h>
> #include <linux/bpf.h>
> #include <linux/mount.h>
> +#include <linux/userfaultfd_k.h>
>
> #include <linux/uaccess.h>
> #include <asm/processor.h>
> @@ -1704,6 +1705,17 @@ static struct ctl_table vm_table[] = {
> .extra1 = (void *)&mmap_rnd_compat_bits_min,
> .extra2 = (void *)&mmap_rnd_compat_bits_max,
> },
> +#endif
> +#ifdef CONFIG_USERFAULTFD
> + {
> + .procname = "unprivileged_userfaultfd",
> + .data = &sysctl_unprivileged_userfaultfd,
> + .maxlen = sizeof(sysctl_unprivileged_userfaultfd),
> + .mode = 0644,
> + .proc_handler = proc_dointvec_minmax,
> + .extra1 = &zero,
> + .extra2 = &one,
> + },
> #endif
> { }
> };
> --
> 2.17.1
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v7 11/15] sched/core: uclamp: Extend CPU's cgroup controller
From: Patrick Bellasi @ 2019-03-19 10:00 UTC (permalink / raw)
To: Tejun Heo
Cc: linux-kernel, linux-pm, linux-api, Ingo Molnar, Peter Zijlstra,
Rafael J . Wysocki, Vincent Guittot, Viresh Kumar, Paul Turner,
Quentin Perret, Dietmar Eggemann, Morten Rasmussen, Juri Lelli,
Todd Kjos, Joel Fernandes, Steve Muckle, Suren Baghdasaryan
In-Reply-To: <20190214154817.GN50184@devbig004.ftw2.facebook.com>
On 14-Feb 07:48, Tejun Heo wrote:
> Hello,
Hi Tejun,
> On Fri, Feb 08, 2019 at 10:05:50AM +0000, Patrick Bellasi wrote:
> > a) are available only for non-root nodes, both on default and legacy
> > hierarchies, while system wide clamps are defined by a generic
> > interface which does not depends on cgroups
> >
> > b) do not enforce any constraints and/or dependencies between the parent
> > and its child nodes, thus relying:
> > - on permission settings defined by the system management software,
> > to define if subgroups can configure their clamp values
> > - on the delegation model, to ensure that effective clamps are
> > updated to consider both subgroup requests and parent group
> > constraints
>
> I'm not sure about this hierarchical behavior.
Yes, the above paragraph is misleading and it fails in properly
document what the code really does.
I'll update it on v8 to be more clear.
>
> > c) have higher priority than task-specific clamps, defined via
> > sched_setattr(), thus allowing to control and restrict task requests
>
> and I have some other concerns about the interface, but let's discuss
> them once the !cgroup portion is settled.
Sure, looking forward to get some more feedbacks from you on that
part.
For the time being I'll keep adding the cgroups bits on top of
the core ones just to follow the principle of "share early, share
often" and to give interested people a more complete picture of our
final goal.
I'm sure Peter will let you know when it's worth for you to be
more actively involved with the review ;)
> Thanks.
Cheers,
Patrick
--
#include <best/regards.h>
Patrick Bellasi
^ permalink raw reply
* [PATCH] uapi: avoid namespace conflict in linux/posix_types.h
From: Arnd Bergmann @ 2019-03-19 16:51 UTC (permalink / raw)
To: linux-api
Cc: linux-arch, netdev, Laura Abbott, Arnd Bergmann, Florian Weimer,
Paul Burton, Deepa Dinamani, linux-kernel
Florian Weimer points out an issue with including linux/posix_types.h
from arbitrary other headers: if an application has defined a macro
named 'fds_bits', it may stop compiling after a change in the kernel
headers. Since fds_bits is a non-reserved identifier in C, that is
considered a bug in the kernel headers.
The structure definition does not really seem to be helpful here,
as the kernel no longer provides macros to manipulate it.
The only user of the structure that we could find was in libsanitize.
This usage is actually broken, as discussed in the email thread,
but this means we cannot just remove the definition from the exported
headers, but we can use the __kernel_* namespace for it, to avoid
the namespace conflict. The kernel itself just uses bit operations
on the typedef without accessing the field by name.
This change should also help with the many other kernel headers that
include linux/posix_types.h directly or through linux/types.h.
Similarly, the definition of __kernel_fsid_t uses a structure with
field identifier 'val' that may have the same problem. Again, user
space should not rely on the specific field name but instead treat
an fsid_t as an opaque identifier. I'm using an #ifdef __KERNEL__
guard here to save us from having to change all kernel code accessing
the field.
Glibc has changed this from 'val' to '__val' back in 1996/97 when
they moved away from using the kernel types directly, it it
is likely that nothing uses __kernel_fsid_t any more. MIPS still
has another copy of __kernel_fsid_t, but that is in the process
of being removed as well.
Link: https://lore.kernel.org/lkml/87a7hvded7.fsf@mid.deneb.enyo.de/
Link: https://lore.kernel.org/lkml/20190314173900.25454-1-paul.burton@mips.com/
Cc: Florian Weimer <fw@deneb.enyo.de>
Cc: Paul Burton <pburton@wavecomp.com>
Fixes: a623a7a1a567 ("y2038: fix socket.h header inclusion")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
There was another bug report for the same change just now, so
I wouldn't apply this patch yet before we have fully understood
the other issue. Sending this for review now since the two
problems are most likely independent.
---
include/uapi/asm-generic/posix_types.h | 4 ++++
include/uapi/linux/posix_types.h | 2 +-
2 files changed, 5 insertions(+), 1 deletion(-)
diff --git a/include/uapi/asm-generic/posix_types.h b/include/uapi/asm-generic/posix_types.h
index f0733a26ebfc..2a8c68ac88ca 100644
--- a/include/uapi/asm-generic/posix_types.h
+++ b/include/uapi/asm-generic/posix_types.h
@@ -77,7 +77,11 @@ typedef __kernel_long_t __kernel_ptrdiff_t;
#ifndef __kernel_fsid_t
typedef struct {
+#ifdef __KERNEL__
int val[2];
+#else
+ int __kernel_val[2];
+#endif
} __kernel_fsid_t;
#endif
diff --git a/include/uapi/linux/posix_types.h b/include/uapi/linux/posix_types.h
index 9a7a740b35a2..a5a5cfc38bbf 100644
--- a/include/uapi/linux/posix_types.h
+++ b/include/uapi/linux/posix_types.h
@@ -23,7 +23,7 @@
#define __FD_SETSIZE 1024
typedef struct {
- unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
+ unsigned long __kernel_fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
} __kernel_fd_set;
/* Type of a signal handler. */
--
2.20.0
^ permalink raw reply related
* Re: [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Andrew Morton @ 2019-03-19 18:02 UTC (permalink / raw)
To: Peter Xu
Cc: linux-kernel, Paolo Bonzini, Hugh Dickins, Luis Chamberlain,
Maxime Coquelin, Maya Gokhale, Jerome Glisse, Pavel Emelyanov,
Johannes Weiner, Martin Cracauer, Denis Plotnikov, linux-mm,
Marty McFadden, Mike Kravetz, Andrea Arcangeli, Mike Rapoport,
Kees Cook, Mel Gorman, Kirill A . Shutemov, linux-api,
linux-fsdevel
In-Reply-To: <20190319030722.12441-2-peterx@redhat.com>
On Tue, 19 Mar 2019 11:07:22 +0800 Peter Xu <peterx@redhat.com> wrote:
> Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
> whether userfaultfd is allowed by unprivileged users. When this is
> set to zero, only privileged users (root user, or users with the
> CAP_SYS_PTRACE capability) will be able to use the userfaultfd
> syscalls.
Please send along a full description of why you believe Linux needs
this feature, for me to add to the changelog. What is the benefit to
our users? How will it be used?
etcetera. As it was presented I'm seeing no justification for adding
the patch!
^ permalink raw reply
* Re: [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Andrea Arcangeli @ 2019-03-19 18:07 UTC (permalink / raw)
To: Mike Rapoport
Cc: Peter Xu, linux-kernel, Paolo Bonzini, Hugh Dickins,
Luis Chamberlain, Maxime Coquelin, Maya Gokhale, Jerome Glisse,
Pavel Emelyanov, Johannes Weiner, Martin Cracauer,
Denis Plotnikov, linux-mm, Marty McFadden, Mike Kravetz,
Mike Rapoport, Kees Cook, Mel Gorman, Kirill A . Shutemov,
linux-api, linux-fsdevel, Dr
In-Reply-To: <20190319071104.GA6392@rapoport-lnx>
Hello,
On Tue, Mar 19, 2019 at 09:11:04AM +0200, Mike Rapoport wrote:
> Hi Peter,
>
> On Tue, Mar 19, 2019 at 11:07:22AM +0800, Peter Xu wrote:
> > Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
> > whether userfaultfd is allowed by unprivileged users. When this is
> > set to zero, only privileged users (root user, or users with the
> > CAP_SYS_PTRACE capability) will be able to use the userfaultfd
> > syscalls.
> >
> > Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
> > Suggested-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
>
> Reviewed-by: Mike Rapoport <rppt@linux.ibm.com>
>
> Just one minor note below
This looks fine with me too.
> > + if (!sysctl_unprivileged_userfaultfd && !capable(CAP_SYS_PTRACE))
> > + return -EPERM;
The only difference between the bpf sysctl and the userfaultfd sysctl
this way is that the bpf sysctl adds the CAP_SYS_ADMIN capability
requirement, while userfaultfd adds the CAP_SYS_PTRACE requirement,
because the userfaultfd monitor is more likely to need CAP_SYS_PTRACE
already if it's doing other kind of tracking on processes runtime, in
addition of userfaultfd. In other words both syscalls works only for
root, when the two sysctl are opt-in set to 1.
Reviewed-by: Andrea Arcangeli <aarcange@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Dr. David Alan Gilbert @ 2019-03-19 18:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Peter Xu, linux-kernel, Paolo Bonzini, Hugh Dickins,
Luis Chamberlain, Maxime Coquelin, Maya Gokhale, Jerome Glisse,
Pavel Emelyanov, Johannes Weiner, Martin Cracauer,
Denis Plotnikov, linux-mm, Marty McFadden, Mike Kravetz,
Andrea Arcangeli, Mike Rapoport, Kees Cook, Mel Gorman,
Kirill A . Shutemov, linux-api
In-Reply-To: <20190319110236.b6169d6b469a587a852c7e09@linux-foundation.org>
* Andrew Morton (akpm@linux-foundation.org) wrote:
> On Tue, 19 Mar 2019 11:07:22 +0800 Peter Xu <peterx@redhat.com> wrote:
>
> > Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
> > whether userfaultfd is allowed by unprivileged users. When this is
> > set to zero, only privileged users (root user, or users with the
> > CAP_SYS_PTRACE capability) will be able to use the userfaultfd
> > syscalls.
>
> Please send along a full description of why you believe Linux needs
> this feature, for me to add to the changelog. What is the benefit to
> our users? How will it be used?
>
> etcetera. As it was presented I'm seeing no justification for adding
> the patch!
How about:
---
Userfaultfd can be misued to make it easier to exploit existing use-after-free
(and similar) bugs that might otherwise only make a short window
or race condition available. By using userfaultfd to stall a kernel
thread, a malicious program can keep some state, that it wrote, stable
for an extended period, which it can then access using an existing
exploit. While it doesn't cause the exploit itself, and while it's not
the only thing that can stall a kernel thread when accessing a memory location,
it's one of the few that never needs priviledge.
Add a flag, allowing userfaultfd to be restricted, so that in general
it won't be useable by arbitrary user programs, but in environments that
require userfaultfd it can be turned back on.
---
Dave
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply
* Re: [PATCH] uapi: avoid namespace conflict in linux/posix_types.h
From: David Miller @ 2019-03-19 21:46 UTC (permalink / raw)
To: arnd
Cc: linux-api, linux-arch, netdev, labbott, fw, pburton, deepa.kernel,
linux-kernel
In-Reply-To: <20190319165123.3967889-1-arnd@arndb.de>
From: Arnd Bergmann <arnd@arndb.de>
Date: Tue, 19 Mar 2019 17:51:09 +0100
> Florian Weimer points out an issue with including linux/posix_types.h
> from arbitrary other headers: if an application has defined a macro
> named 'fds_bits', it may stop compiling after a change in the kernel
> headers. Since fds_bits is a non-reserved identifier in C, that is
> considered a bug in the kernel headers.
>
> The structure definition does not really seem to be helpful here,
> as the kernel no longer provides macros to manipulate it.
...
> Fixes: a623a7a1a567 ("y2038: fix socket.h header inclusion")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH] uapi: avoid namespace conflict in linux/posix_types.h
From: Florian Weimer @ 2019-03-19 21:55 UTC (permalink / raw)
To: Arnd Bergmann
Cc: linux-api, linux-arch, netdev, Laura Abbott, Paul Burton,
Deepa Dinamani, linux-kernel
In-Reply-To: <20190319165123.3967889-1-arnd@arndb.de>
* Arnd Bergmann:
> diff --git a/include/uapi/asm-generic/posix_types.h b/include/uapi/asm-generic/posix_types.h
> index f0733a26ebfc..2a8c68ac88ca 100644
> --- a/include/uapi/asm-generic/posix_types.h
> +++ b/include/uapi/asm-generic/posix_types.h
> @@ -77,7 +77,11 @@ typedef __kernel_long_t __kernel_ptrdiff_t;
>
> #ifndef __kernel_fsid_t
> typedef struct {
> +#ifdef __KERNEL__
> int val[2];
> +#else
> + int __kernel_val[2];
> +#endif
> } __kernel_fsid_t;
> #endif
>
> diff --git a/include/uapi/linux/posix_types.h b/include/uapi/linux/posix_types.h
> index 9a7a740b35a2..a5a5cfc38bbf 100644
> --- a/include/uapi/linux/posix_types.h
> +++ b/include/uapi/linux/posix_types.h
> @@ -23,7 +23,7 @@
> #define __FD_SETSIZE 1024
>
> typedef struct {
> - unsigned long fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> + unsigned long __kernel_fds_bits[__FD_SETSIZE / (8 * sizeof(long))];
> } __kernel_fd_set;
>
> /* Type of a signal handler. */
Both changes look reasonably to me, but I have not tested them.
^ permalink raw reply
* Re: [PATCH v2 1/1] userfaultfd/sysctl: add vm.unprivileged_userfaultfd
From: Peter Xu @ 2019-03-20 0:20 UTC (permalink / raw)
To: Dr. David Alan Gilbert
Cc: Andrew Morton, linux-kernel, Paolo Bonzini, Hugh Dickins,
Luis Chamberlain, Maxime Coquelin, Maya Gokhale, Jerome Glisse,
Pavel Emelyanov, Johannes Weiner, Martin Cracauer,
Denis Plotnikov, linux-mm, Marty McFadden, Mike Kravetz,
Andrea Arcangeli, Mike Rapoport, Kees Cook, Mel Gorman,
Kirill A . Shutemov, linux-ap
In-Reply-To: <20190319182822.GK2727@work-vm>
On Tue, Mar 19, 2019 at 06:28:23PM +0000, Dr. David Alan Gilbert wrote:
> * Andrew Morton (akpm@linux-foundation.org) wrote:
> > On Tue, 19 Mar 2019 11:07:22 +0800 Peter Xu <peterx@redhat.com> wrote:
> >
> > > Add a global sysctl knob "vm.unprivileged_userfaultfd" to control
> > > whether userfaultfd is allowed by unprivileged users. When this is
> > > set to zero, only privileged users (root user, or users with the
> > > CAP_SYS_PTRACE capability) will be able to use the userfaultfd
> > > syscalls.
> >
> > Please send along a full description of why you believe Linux needs
> > this feature, for me to add to the changelog. What is the benefit to
> > our users? How will it be used?
> >
> > etcetera. As it was presented I'm seeing no justification for adding
> > the patch!
>
> How about:
>
> ---
> Userfaultfd can be misued to make it easier to exploit existing use-after-free
> (and similar) bugs that might otherwise only make a short window
> or race condition available. By using userfaultfd to stall a kernel
> thread, a malicious program can keep some state, that it wrote, stable
> for an extended period, which it can then access using an existing
> exploit. While it doesn't cause the exploit itself, and while it's not
> the only thing that can stall a kernel thread when accessing a memory location,
> it's one of the few that never needs priviledge.
>
> Add a flag, allowing userfaultfd to be restricted, so that in general
> it won't be useable by arbitrary user programs, but in environments that
> require userfaultfd it can be turned back on.
Thanks for the quick write up, Dave! I definitely should have some
justification in the cover letter and carry it until the last version.
Sorry to be unclear at the first glance.
--
Peter Xu
^ permalink raw reply
* Re: [RESEND PATCH v1] moduleparam: Save information about built-in modules in separate file
From: Masahiro Yamada @ 2019-03-20 8:54 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Michal Marek, Linux Kernel Mailing List,
Linux Kbuild mailing list, linux-api, Kirill A . Shutemov,
Gleb Fotengauer-Malinovskiy, Dmitry V. Levin
In-Reply-To: <20190315101013.GN8455@Legion-PC.fortress>
On Fri, Mar 15, 2019 at 7:10 PM Alexey Gladkov <gladkov.alexey@gmail.com> wrote:
>
> Problem:
>
> When a kernel module is compiled as a separate module, some important
> information about the kernel module is available via .modinfo section of
> the module. In contrast, when the kernel module is compiled into the
> kernel, that information is not available.
>
> Information about built-in modules is necessary in the following cases:
>
> 1. When it is necessary to find out what additional parameters can be
> passed to the kernel at boot time.
>
> 2. When you need to know which module names and their aliases are in
> the kernel. This is very useful for creating an initrd image.
>
> Proposal:
>
> The proposed patch does not remove .modinfo section with module
> information from the vmlinux at the build time and saves it into a
> separate file after kernel linking. So, the kernel does not increase in
> size and no additional information remains in it. Information is stored
> in the same format as in the separate modules (null-terminated string
> array). Because the .modinfo section is already exported with a separate
> modules, we are not creating a new API.
>
> It can be easily read in the userspace:
>
> $ tr '\0' '\n' < kernel.builtin.modinfo
> ext4.softdep=pre: crc32c
> ext4.license=GPL
> ext4.description=Fourth Extended Filesystem
> ext4.author=Remy Card, Stephen Tweedie, Andrew Morton, Andreas Dilger, Theodore Ts'o and others
> ext4.alias=fs-ext4
> ext4.alias=ext3
> ext4.alias=fs-ext3
> ext4.alias=ext2
> ext4.alias=fs-ext2
> md_mod.alias=block-major-9-*
> md_mod.alias=md
> md_mod.description=MD RAID framework
> md_mod.license=GPL
> md_mod.parmtype=create_on_open:bool
> md_mod.parmtype=start_dirty_degraded:int
> ...
>
> Co-Developed-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> Signed-off-by: Gleb Fotengauer-Malinovskiy <glebfm@altlinux.org>
> Signed-off-by: Alexey Gladkov <gladkov.alexey@gmail.com>
> ---
I just started to take a look.
I was able to compile it for x86,
but got tons of warnings for powerpc.
Anyway, you do not need to send v2 soon.
Please give me some more time for the through review.
The following is what I got by
"make -j8 ARCH=powerpc CROSS_COMPILE=powerpc-linux- defconfig all"
...
CC [M] drivers/scsi/st.o
AR drivers/scsi/built-in.a
AR drivers/built-in.a
GEN .version
CHK include/generated/compile.h
UPD include/generated/compile.h
CC init/version.o
AR init/built-in.a
LD vmlinux.o
MODPOST vmlinux.o
powerpc-linux-ld: warning: orphan section `.gnu.hash' from `linker
stubs' being placed in section `.gnu.hash'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`arch/powerpc/platforms/pseries/pseries_energy.o' being placed in
section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`arch/powerpc/platforms/pseries/cmm.o' being placed in section
`.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`arch/powerpc/platforms/pasemi/gpio_mdio.o' being placed in section
`.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/workqueue.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/printk/printk.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/irq/spurious.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/rcu/update.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/rcu/srcutree.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/rcu/tree.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/module.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`kernel/configs.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/binfmt_script.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/binfmt_elf.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/compat_binfmt_elf.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/mbcache.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs_common/nfsacl.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs_common/grace.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/ext4/super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/ext2/super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/jbd2/journal.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/fat/inode.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/fat/namei_msdos.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/isofs/inode.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/dir.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/inode.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/nfs2super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/nfs3super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nfs/nfs4super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/exportfs/expfs.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/lockd/svc.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nls/nls_base.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nls/nls_cp437.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nls/nls_ascii.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nls/nls_iso8859-1.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/nls/nls_utf8.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/xfs/xfs_super.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/debugfs/inode.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`fs/tracefs/inode.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/api.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/crypto_wq.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/algapi.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/aead.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/blkcipher.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/skcipher.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/ahash.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/shash.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/akcipher.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/kpp.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/acompress.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/scompress.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/algboss.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/testmgr.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/hmac.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/crypto_null.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/sha256_generic.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/aes_generic.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/crc32c_generic.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`crypto/rng.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`block/bsg.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`block/mq-deadline.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`block/kyber-iosched.o' being placed in section `.modinfo'.
powerpc-linux-ld: warning: orphan section `.modinfo' from
`lib/btree.o' being placed in section `.modinfo'.
...
> Makefile | 1 +
> include/linux/moduleparam.h | 12 +++++-------
> scripts/link-vmlinux.sh | 8 ++++++++
> 3 files changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index d5713e7b1e50..971102194c92 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1288,6 +1288,7 @@ _modinst_:
> fi
> @cp -f $(objtree)/modules.order $(MODLIB)/
> @cp -f $(objtree)/modules.builtin $(MODLIB)/
> + @cp -f $(objtree)/kernel.builtin.modinfo $(MODLIB)/
> $(Q)$(MAKE) -f $(srctree)/scripts/Makefile.modinst
>
> # This depmod is only for convenience to give the initial
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index ba36506db4fb..5ba250d9172a 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -10,23 +10,21 @@
> module name. */
> #ifdef MODULE
> #define MODULE_PARAM_PREFIX /* empty */
> +#define __MODULE_INFO_PREFIX /* empty */
> #else
> #define MODULE_PARAM_PREFIX KBUILD_MODNAME "."
> +/* We cannot use MODULE_PARAM_PREFIX because some modules override it. */
> +#define __MODULE_INFO_PREFIX KBUILD_MODNAME "."
> #endif
>
> /* Chosen so that structs with an unsigned long line up. */
> #define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
>
> -#ifdef MODULE
> #define __MODULE_INFO(tag, name, info) \
> static const char __UNIQUE_ID(name)[] \
> __used __attribute__((section(".modinfo"), unused, aligned(1))) \
> - = __stringify(tag) "=" info
> -#else /* !MODULE */
> -/* This struct is here for syntactic coherency, it is not used */
> -#define __MODULE_INFO(tag, name, info) \
> - struct __UNIQUE_ID(name) {}
> -#endif
> + = __MODULE_INFO_PREFIX __stringify(tag) "=" info
> +
> #define __MODULE_PARM_TYPE(name, _type) \
> __MODULE_INFO(parmtype, name##type, #name ":" _type)
>
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index c8cf45362bd6..399d7e4d11ec 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -258,10 +258,12 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
>
> # step 1
> vmlinux_link "" .tmp_vmlinux1
> + "${OBJCOPY}" -R .modinfo .tmp_vmlinux1
> kallsyms .tmp_vmlinux1 .tmp_kallsyms1.o
>
> # step 2
> vmlinux_link .tmp_kallsyms1.o .tmp_vmlinux2
> + "${OBJCOPY}" -R .modinfo .tmp_vmlinux2
> kallsyms .tmp_vmlinux2 .tmp_kallsyms2.o
>
> # step 3
> @@ -273,6 +275,7 @@ if [ -n "${CONFIG_KALLSYMS}" ]; then
> kallsyms_vmlinux=.tmp_vmlinux3
>
> vmlinux_link .tmp_kallsyms2.o .tmp_vmlinux3
> + "${OBJCOPY}" -R .modinfo .tmp_vmlinux3
>
> kallsyms .tmp_vmlinux3 .tmp_kallsyms3.o
> fi
> @@ -281,6 +284,11 @@ fi
> info LD vmlinux
> vmlinux_link "${kallsymso}" vmlinux
>
> +info MODINFO kernel.builtin.modinfo
> +"${OBJCOPY}" -j .modinfo -O binary vmlinux kernel.builtin.modinfo
> +chmod 444 kernel.builtin.modinfo
> +"${OBJCOPY}" -R .modinfo vmlinux
> +
> if [ -n "${CONFIG_BUILDTIME_EXTABLE_SORT}" ]; then
> info SORTEX vmlinux
> sortextable vmlinux
> --
> 2.19.2
>
--
Best Regards
Masahiro Yamada
^ permalink raw reply
* [PATCH RESEND v5 0/5] namei: vfs flags to restrict path resolution
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Jann Horn,
Christian Brauner, David Drysdale, Tycho Andersen, Kees Cook,
containers, linux-fsdevel, linux-api, Andrew Morton,
Alexei Starovoitov, Chanho Min, Oleg Nesterov, Aleksa Sarai,
Linus Torvalds, linux-kernel, linux-arch
Now that the holiday break is over, it's time to re-send this patch
series (with a few additions, due to new information we got from
CVE-2019-5736 -- which this patchset mostly protected against but had
some holes with regards to #!-style scripts).
Patch changelog:
v5:
* In response to CVE-2019-5736 (one of the vectors showed that
open(2)+fexec(3) cannot be used to scope binfmt_script's implicit
open_exec()), AT_* flags have been re-added and are now piped
through to binfmt_script (and other binfmt_* that use open_exec)
but are only supported for execveat(2) for now.
v4:
* Remove AT_* flag reservations, as they require more discussion.
* Switch to path_is_under() over __d_path() for breakout checking.
* Make O_XDEV no longer block openat("/tmp", "/", O_XDEV) -- dirfd
is now ignored for absolute paths to match other flags.
* Improve the dirfd_path_init() refactor and move it to a separate
commit.
* Remove reference to Linux-capsicum.
* Switch "proclink" name to "magic link".
v3: [resend]
v2:
* Made ".." resolution with AT_THIS_ROOT and AT_BENEATH safe(r) with
some semi-aggressive __d_path checking (see patch 3).
* Disallowed "proclinks" with AT_THIS_ROOT and AT_BENEATH, in the
hopes they can be re-enabled once safe.
* Removed the selftests as they will be reimplemented as xfstests.
* Removed stat(2) support, since you can already get it through
O_PATH and fstatat(2).
The need for some sort of control over VFS's path resolution (to avoid
malicious paths resulting in inadvertent breakouts) has been a very
long-standing desire of many userspace applications. This patchset is a
revival of Al Viro's old AT_NO_JUMPS[1,2] patchset (which was a variant
of David Drysdale's O_BENEATH patchset[3] which was a spin-off of the
Capsicum project[4]) with a few additions and changes made based on the
previous discussion within [5] as well as others I felt were useful.
In line with the conclusions of the original discussion of AT_NO_JUMPS,
the flag has been split up into separate flags:
* O_XDEV blocks all mountpoint crossings (upwards, downwards, or
through absolute links). Absolute pathnames alone in openat(2) do
not trigger this.
* O_NOMAGICLINKS blocks resolution through /proc/$pid/fd-style links.
This is done by blocking the usage of nd_jump_link() during
resolution in a filesystem. The term "magic links" is used to match
with the only reference to these links in Documentation/, but I'm
happy to change the name.
It should be noted that this is different to the scope of O_NOFOLLOW
in that it applies to all path components. However, you can do
open(O_NOFOLLOW|O_NOMAGICLINKS|O_PATH) on a "magic link" and it will
*not* fail (assuming that no parent component was a "magic link"),
and you will have an fd for the "magic link".
* O_BENEATH disallows escapes to outside the starting dirfd's tree,
using techniques such as ".." or absolute links. Absolute paths in
openat(2) are also disallowed. Conceptually this flag is to ensure
you "stay below" a certain point in the filesystem tree -- but this
requires some additional to protect against various races that would
allow escape using ".." (see patch 4 for more detail).
Currently O_BENEATH implies O_NOMAGICLINKS, because it can trivially
beam you around the filesystem (breaking the protection). In future,
there might be similar safety checks as in patch 4, but that
requires more discussion.
In addition, two new flags were added that expand on the above ideas:
* O_NOSYMLINKS does what it says on the tin. No symlink resolution is
allowed at all, including "magic links". Just as with O_NOMAGICLINKS
this can still be used with (O_PATH|O_NOFOLLOW) to open an fd for
the symlink as long as no parent path had a symlink component.
* O_THISROOT is an extension of O_BENEATH that, rather than blocking
attempts to move past the root, forces all such movements to be
scoped to the starting point. This provides chroot(2)-like
protection but without the cost of a chroot(2) for each filesystem
operation, as well as being safe against race attacks that chroot(2)
is not.
If a race is detected (as with O_BENEATH) then an error is
generated, and similar to O_BENEATH it is not permitted to cross
"magic links" with O_THISROOT.
The primary need for this is from container runtimes, which
currently need to do symlink scoping in userspace[6] when opening
paths in a potentially malicious container. There is a long list of
CVEs that could have bene mitigated by having O_THISROOT (such as
CVE-2017-1002101, CVE-2017-1002102, CVE-2018-15664, and
CVE-2019-5736, just to name a few).
In addition, a mirror set of AT_* flags have been added (though
currently these are only supported for execveat(2) -- and not for any
other syscall). The need for these is explained in the final patch in
the series (it's motivated by CVE-2019-5736).
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Jann Horn <jannh@google.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: David Drysdale <drysdale@google.com>
Cc: Tycho Andersen <tycho@tycho.ws>
Cc: Kees Cook <keescook@chromium.org>
Cc: <containers@lists.linux-foundation.org>
Cc: <linux-fsdevel@vger.kernel.org>
Cc: <linux-api@vger.kernel.org>
[1]: https://lwn.net/Articles/721443/
[2]: https://lore.kernel.org/patchwork/patch/784221/
[3]: https://lwn.net/Articles/619151/
[4]: https://lwn.net/Articles/603929/
[5]: https://lwn.net/Articles/723057/
[6]: https://github.com/cyphar/filepath-securejoin
Aleksa Sarai (5):
namei: split out nd->dfd handling to dirfd_path_init
namei: O_BENEATH-style path resolution flags
namei: O_THISROOT: chroot-like path resolution
namei: aggressively check for nd->root escape on ".." resolution
binfmt_*: scope path resolution of interpreters
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/binfmt_em86.c | 4 +-
fs/binfmt_misc.c | 2 +-
fs/binfmt_script.c | 2 +-
fs/exec.c | 26 +++-
fs/fcntl.c | 2 +-
fs/namei.c | 205 ++++++++++++++++++++++---------
fs/open.c | 13 +-
include/linux/binfmts.h | 1 +
include/linux/fcntl.h | 3 +-
include/linux/fs.h | 9 +-
include/linux/namei.h | 8 ++
include/uapi/asm-generic/fcntl.h | 17 +++
include/uapi/linux/fcntl.h | 6 +
15 files changed, 228 insertions(+), 74 deletions(-)
--
2.21.0
^ permalink raw reply
* [PATCH RESEND v5 1/5] namei: split out nd->dfd handling to dirfd_path_init
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Christian Brauner,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
Linus Torvalds, containers, linux-fsdevel, linux-api,
linux-kernel, linux-arch
In-Reply-To: <20190320143717.2523-1-cyphar@cyphar.com>
Previously, path_init's handling of *at(dfd, ...) was only done once,
but with O_BENEATH (and O_THISROOT) we have to parse the initial
nd->path at different times (before or after absolute path handling)
depending on whether we have been asked to scope resolution within a
root.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 103 ++++++++++++++++++++++++++++++-----------------------
1 file changed, 59 insertions(+), 44 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index a85deb55d0c9..4fdcb36f7c01 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2168,9 +2168,59 @@ static int link_path_walk(const char *name, struct nameidata *nd)
}
}
+/*
+ * Configure nd->path based on the nd->dfd. This is only used as part of
+ * path_init().
+ */
+static inline int dirfd_path_init(struct nameidata *nd)
+{
+ if (nd->dfd == AT_FDCWD) {
+ if (nd->flags & LOOKUP_RCU) {
+ struct fs_struct *fs = current->fs;
+ unsigned seq;
+
+ do {
+ seq = read_seqcount_begin(&fs->seq);
+ nd->path = fs->pwd;
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
+ } while (read_seqcount_retry(&fs->seq, seq));
+ } else {
+ get_fs_pwd(current->fs, &nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ } else {
+ /* Caller must check execute permissions on the starting path component */
+ struct fd f = fdget_raw(nd->dfd);
+ struct dentry *dentry;
+
+ if (!f.file)
+ return -EBADF;
+
+ dentry = f.file->f_path.dentry;
+
+ if (*nd->name->name && unlikely(!d_can_lookup(dentry))) {
+ fdput(f);
+ return -ENOTDIR;
+ }
+
+ nd->path = f.file->f_path;
+ if (nd->flags & LOOKUP_RCU) {
+ nd->inode = nd->path.dentry->d_inode;
+ nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
+ } else {
+ path_get(&nd->path);
+ nd->inode = nd->path.dentry->d_inode;
+ }
+ fdput(f);
+ }
+ return 0;
+}
+
/* must be paired with terminate_walk() */
static const char *path_init(struct nameidata *nd, unsigned flags)
{
+ int error;
const char *s = nd->name->name;
if (!*s)
@@ -2204,52 +2254,17 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
if (*s == '/') {
- set_root(nd);
- if (likely(!nd_jump_root(nd)))
- return s;
- return ERR_PTR(-ECHILD);
- } else if (nd->dfd == AT_FDCWD) {
- if (flags & LOOKUP_RCU) {
- struct fs_struct *fs = current->fs;
- unsigned seq;
-
- do {
- seq = read_seqcount_begin(&fs->seq);
- nd->path = fs->pwd;
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
- } while (read_seqcount_retry(&fs->seq, seq));
- } else {
- get_fs_pwd(current->fs, &nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- return s;
- } else {
- /* Caller must check execute permissions on the starting path component */
- struct fd f = fdget_raw(nd->dfd);
- struct dentry *dentry;
-
- if (!f.file)
- return ERR_PTR(-EBADF);
-
- dentry = f.file->f_path.dentry;
-
- if (*s && unlikely(!d_can_lookup(dentry))) {
- fdput(f);
- return ERR_PTR(-ENOTDIR);
- }
-
- nd->path = f.file->f_path;
- if (flags & LOOKUP_RCU) {
- nd->inode = nd->path.dentry->d_inode;
- nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);
- } else {
- path_get(&nd->path);
- nd->inode = nd->path.dentry->d_inode;
- }
- fdput(f);
+ if (likely(!nd->root.mnt))
+ set_root(nd);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ s = ERR_PTR(error);
return s;
}
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ return s;
}
static const char *trailing_symlink(struct nameidata *nd)
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 2/5] namei: O_BENEATH-style path resolution flags
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, Kees Cook,
David Drysdale, Andy Lutomirski, Linus Torvalds, Andrew Morton,
Alexei Starovoitov, Jann Horn, Chanho Min, Oleg Nesterov,
Aleksa Sarai, containers, linux-fsdevel, linux-api, linux-kernel,
linux-arch
In-Reply-To: <20190320143717.2523-1-cyphar@cyphar.com>
Add the following flags to allow various restrictions on path
resolution (these affect the *entire* resolution, rather than just the
final path component -- as is the case with most other AT_* flags).
The primary justification for these flags is to allow for programs to be
far more strict about how they want path resolution to handle symlinks,
mountpoint crossings, and paths that escape the dirfd (through an
absolute path or ".." shenanigans).
This is of particular concern to container runtimes that want to be very
careful about malicious root filesystems that a container's init might
have screwed around with (and there is no real way to protect against
this in userspace if you consider potential races against a malicious
container's init). More classical applications (which have their own
potentially buggy userspace path sanitisation code) include web
servers, archive extraction tools, network file servers, and so on.
* O_XDEV: Disallow mount-point crossing (both *down* into one, or *up*
from one). The primary "scoping" use is to blocking resolution that
crosses a bind-mount, which has a similar property to a symlink (in
the way that it allows for escape from the starting-point). Since it
is not possible to differentiate bind-mounts However since
bind-mounting requires privileges (in ways symlinks don't) this has
been split from LOOKUP_BENEATH. The naming is based on "find -xdev"
(though find(1) doesn't walk upwards, the semantics seem obvious).
* O_NOMAGICLINKS: Disallows ->get_link "symlink" jumping. This is a very
specific restriction, and it exists because /proc/$pid/fd/...
"symlinks" allow for access outside nd->root and pose risk to
container runtimes that don't want to be tricked into accessing a host
path (but do want to allow no-funny-business symlink resolution).
* O_NOSYMLINKS: Disallows symlink jumping *of any kind*. Implies
O_NOMAGICLINKS (obviously).
* O_BENEATH: Disallow "escapes" from the starting point of the
filesystem tree during resolution (you must stay "beneath" the
starting point at all times). Currently this is done by disallowing
".." and absolute paths (either in the given path or found during
symlink resolution) entirely, as well as all "magic link" jumping.
The wholesale banning of ".." is because it is currently not safe to
allow ".." resolution (races can cause the path to be moved outside of
the root -- this is conceptually similar to historical chroot(2)
escape attacks). Future patches in this series will address this, and
will re-enable ".." resolution once it is safe. With those patches,
".." resolution will only be allowed if it remains in the root
throughout resolution (such as "a/../b" not "a/../../outside/b").
The banning of "magic link" jumping is done because it is not clear
whether semantically they should be allowed -- while some "magic
links" are safe there are many that can cause escapes (and once a
resolution is outside of the root, O_BENEATH will no longer detect
it). Future patches may re-enable "magic link" jumping when such jumps
would remain inside the root.
The O_NO*LINK flags return -ELOOP if path resolution would violates
their requirement, while the others all return -EXDEV. Currently these
are only enabled for openat(2). In future it may be necessary to enable
these for *at(2) flags, as well as expanding support for AT_EMPTY_PATH.
This is a refresh of Al's AT_NO_JUMPS patchset[1] (which was a variation
on David Drysdale's O_BENEATH patchset[2], which in turn was based on
the Capsicum project[3]). Input from Linus and Andy in the AT_NO_JUMPS
thread[4] determined most of the API changes made in this refresh.
[1]: https://lwn.net/Articles/721443/
[2]: https://lwn.net/Articles/619151/
[3]: https://lwn.net/Articles/603929/
[4]: https://lwn.net/Articles/723057/
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Kees Cook <keescook@chromium.org>
Suggested-by: David Drysdale <drysdale@google.com>
Suggested-by: Al Viro <viro@zeniv.linux.org.uk>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 76 +++++++++++++++++++++++++++-----
fs/open.c | 11 ++++-
include/linux/fcntl.h | 3 +-
include/linux/namei.h | 7 +++
include/uapi/asm-generic/fcntl.h | 14 ++++++
6 files changed, 99 insertions(+), 14 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index 083185174c6d..f782e99700f0 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(21 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index 4fdcb36f7c01..c9a07a8c005a 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -845,6 +845,12 @@ static inline void path_to_nameidata(const struct path *path,
static int nd_jump_root(struct nameidata *nd)
{
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
+ if (unlikely(nd->flags & LOOKUP_XDEV)) {
+ if (nd->path.mnt != nd->root.mnt)
+ return -EXDEV;
+ }
if (nd->flags & LOOKUP_RCU) {
struct dentry *d;
nd->path = nd->root;
@@ -1053,6 +1059,9 @@ const char *get_link(struct nameidata *nd)
int error;
const char *res;
+ if (unlikely(nd->flags & LOOKUP_NO_SYMLINKS))
+ return ERR_PTR(-ELOOP);
+
if (!(nd->flags & LOOKUP_RCU)) {
touch_atime(&last->link);
cond_resched();
@@ -1083,14 +1092,23 @@ const char *get_link(struct nameidata *nd)
} else {
res = get(dentry, inode, &last->done);
}
+ /* If we just jumped it was because of a procfs-style link. */
+ if (unlikely(nd->flags & LOOKUP_JUMPED)) {
+ if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
+ return ERR_PTR(-ELOOP);
+ /* Not currently safe. */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return ERR_PTR(-EXDEV);
+ }
if (IS_ERR_OR_NULL(res))
return res;
}
if (*res == '/') {
if (!nd->root.mnt)
set_root(nd);
- if (unlikely(nd_jump_root(nd)))
- return ERR_PTR(-ECHILD);
+ error = nd_jump_root(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
while (unlikely(*++res == '/'))
;
}
@@ -1271,12 +1289,16 @@ static int follow_managed(struct path *path, struct nameidata *nd)
break;
}
- if (need_mntput && path->mnt == mnt)
- mntput(path->mnt);
+ if (need_mntput) {
+ if (path->mnt == mnt)
+ mntput(path->mnt);
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ ret = -EXDEV;
+ else
+ nd->flags |= LOOKUP_JUMPED;
+ }
if (ret == -EISDIR || !ret)
ret = 1;
- if (need_mntput)
- nd->flags |= LOOKUP_JUMPED;
if (unlikely(ret < 0))
path_put_conditional(path, nd);
return ret;
@@ -1333,6 +1355,8 @@ static bool __follow_mount_rcu(struct nameidata *nd, struct path *path,
mounted = __lookup_mnt(path->mnt, path->dentry);
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return false;
path->mnt = &mounted->mnt;
path->dentry = mounted->mnt.mnt_root;
nd->flags |= LOOKUP_JUMPED;
@@ -1353,8 +1377,11 @@ static int follow_dotdot_rcu(struct nameidata *nd)
struct inode *inode = nd->inode;
while (1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
struct dentry *old = nd->path.dentry;
struct dentry *parent = old->d_parent;
@@ -1379,6 +1406,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (&mparent->mnt == nd->path.mnt)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
/* we know that mountpoint was pinned */
nd->path.dentry = mountpoint;
nd->path.mnt = &mparent->mnt;
@@ -1393,6 +1422,8 @@ static int follow_dotdot_rcu(struct nameidata *nd)
return -ECHILD;
if (!mounted)
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
nd->path.mnt = &mounted->mnt;
nd->path.dentry = mounted->mnt.mnt_root;
inode = nd->path.dentry->d_inode;
@@ -1481,8 +1512,11 @@ static int path_parent_directory(struct path *path)
static int follow_dotdot(struct nameidata *nd)
{
while(1) {
- if (path_equal(&nd->path, &nd->root))
+ if (path_equal(&nd->path, &nd->root)) {
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
break;
+ }
if (nd->path.dentry != nd->path.mnt->mnt_root) {
int ret = path_parent_directory(&nd->path);
if (ret)
@@ -1491,6 +1525,8 @@ static int follow_dotdot(struct nameidata *nd)
}
if (!follow_up(&nd->path))
break;
+ if (unlikely(nd->flags & LOOKUP_XDEV))
+ return -EXDEV;
}
follow_mount(&nd->path);
nd->inode = nd->path.dentry->d_inode;
@@ -1705,6 +1741,13 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
+ /*
+ * LOOKUP_BENEATH resolving ".." is not currently safe -- races can
+ * cause our parent to have moved outside of the root and us to skip
+ * over it.
+ */
+ if (unlikely(nd->flags & LOOKUP_BENEATH))
+ return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
if (nd->flags & LOOKUP_RCU) {
@@ -2253,6 +2296,15 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.dentry = NULL;
nd->m_seq = read_seqbegin(&mount_lock);
+
+ if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ nd->root = nd->path;
+ if (!(nd->flags & LOOKUP_RCU))
+ path_get(&nd->root);
+ }
if (*s == '/') {
if (likely(!nd->root.mnt))
set_root(nd);
@@ -2261,9 +2313,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
s = ERR_PTR(error);
return s;
}
- error = dirfd_path_init(nd);
- if (unlikely(error))
- return ERR_PTR(error);
+ if (likely(!nd->path.mnt)) {
+ error = dirfd_path_init(nd);
+ if (unlikely(error))
+ return ERR_PTR(error);
+ }
return s;
}
diff --git a/fs/open.c b/fs/open.c
index 0285ce7dbd51..3e73f940f56e 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -959,7 +959,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* If we have O_PATH in the open flag. Then we
* cannot have anything other than the below set of flags
*/
- flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH;
+ flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
acc_mode = 0;
}
@@ -988,6 +989,14 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_DIRECTORY;
if (!(flags & O_NOFOLLOW))
lookup_flags |= LOOKUP_FOLLOW;
+ if (flags & O_BENEATH)
+ lookup_flags |= LOOKUP_BENEATH;
+ if (flags & O_XDEV)
+ lookup_flags |= LOOKUP_XDEV;
+ if (flags & O_NOMAGICLINKS)
+ lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (flags & O_NOSYMLINKS)
+ lookup_flags |= LOOKUP_NO_SYMLINKS;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 27dc7a60693e..864399c2fdd2 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -9,7 +9,8 @@
(O_RDONLY | O_WRONLY | O_RDWR | O_CREAT | O_EXCL | O_NOCTTY | O_TRUNC | \
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
- O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE)
+ O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
+ O_NOMAGICLINKS | O_NOSYMLINKS)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index a78606e8e3df..82b5039d27a6 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -47,6 +47,13 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_EMPTY 0x4000
#define LOOKUP_DOWN 0x8000
+/* Scoping flags for lookup. */
+#define LOOKUP_BENEATH 0x010000 /* No escaping from starting point. */
+#define LOOKUP_XDEV 0x020000 /* No mountpoint crossing. */
+#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
+#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
+ Implies LOOKUP_NO_MAGICLINKS. */
+
extern int path_pts(struct path *path);
extern int user_path_at_empty(int, const char __user *, unsigned, struct path *, int *empty);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index 9dc0bf0c5a6e..ba53e0836cd4 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -97,6 +97,20 @@
#define O_NDELAY O_NONBLOCK
#endif
+/* Type of path-resolution scoping we are applying. */
+#ifndef O_BENEATH
+#define O_BENEATH 00040000000 /* - Block "lexical" trickery like "..", symlinks, absolute paths, etc. */
+#endif
+#ifndef O_XDEV
+#define O_XDEV 00100000000 /* - Block mount-point crossings (includes bind-mounts). */
+#endif
+#ifndef O_NOMAGICLINKS
+#define O_NOMAGICLINKS 00200000000 /* - Block procfs-style "magic" symlinks. */
+#endif
+#ifndef O_NOSYMLINKS
+#define O_NOSYMLINKS 01000000000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
+#endif
+
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
#define F_SETFD 2 /* set/clear close_on_exec */
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 3/5] namei: O_THISROOT: chroot-like path resolution
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Christian Brauner, Kees Cook,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov, Jann Horn,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
Linus Torvalds, containers, linux-fsdevel, linux-api,
linux-kernel, linux-arch
In-Reply-To: <20190320143717.2523-1-cyphar@cyphar.com>
The primary motivation for the need for this flag is container runtimes
which have to interact with malicious root filesystems in the host
namespaces. One of the first requirements for a container runtime to be
secure against a malicious rootfs is that they correctly scope symlinks
(that is, they should be scoped as though they are chroot(2)ed into the
container's rootfs) and ".."-style paths[*]. The already-existing O_XDEV
and O_NOMAGICLINKS[**] help defend against other potential attacks in a
malicious rootfs scenario.
Currently most container runtimes try to do this resolution in
userspace[1], causing many potential race conditions. In addition, the
"obvious" alternative (actually performing a {ch,pivot_}root(2))
requires a fork+exec (for some runtimes) which is *very* costly if
necessary for every filesystem operation involving a container.
[*] At the moment, ".." and "magic link" jumping are disallowed for the
same reason it is disabled for O_BENEATH -- currently it is not safe
to allow it. Future patches may enable it unconditionally once we
have resolved the possible races (for "..") and semantics (for
"magic link" jumping).
The most significant openat(2) semantic change with O_THISROOT is that
absolute pathnames no longer cause dirfd to be ignored completely. The
rationale is that O_THISROOT must necessarily chroot-scope symlinks with
absolute paths to dirfd, and so doing it for the base path seems to be
the most consistent behaviour (and also avoids foot-gunning users who
want to scope paths that are absolute).
Currently this is only enabled for openat(2), and similar to O_BENEATH
and family requires more discussion about extending it to more *at(2)
syscalls as well as extending AT_EMPTY_PATH support.
[1]: https://github.com/cyphar/filepath-securejoin
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Christian Brauner <christian@brauner.io>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/fcntl.c | 2 +-
fs/namei.c | 6 +++---
fs/open.c | 4 +++-
include/linux/fcntl.h | 2 +-
include/linux/namei.h | 1 +
include/uapi/asm-generic/fcntl.h | 3 +++
6 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index f782e99700f0..a6b4565a903d 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -1031,7 +1031,7 @@ static int __init fcntl_init(void)
* Exceptions: O_NONBLOCK is a two bit define on parisc; O_NDELAY
* is defined as O_NONBLOCK on some platforms and not on others.
*/
- BUILD_BUG_ON(25 - 1 /* for O_RDONLY being 0 */ !=
+ BUILD_BUG_ON(26 - 1 /* for O_RDONLY being 0 */ !=
HWEIGHT32(
(VALID_OPEN_FLAGS & ~(O_NONBLOCK | O_NDELAY)) |
__FMODE_EXEC | __FMODE_NONOTIFY));
diff --git a/fs/namei.c b/fs/namei.c
index c9a07a8c005a..798eb1702a0c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1097,7 +1097,7 @@ const char *get_link(struct nameidata *nd)
if (unlikely(nd->flags & LOOKUP_NO_MAGICLINKS))
return ERR_PTR(-ELOOP);
/* Not currently safe. */
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return ERR_PTR(-EXDEV);
}
if (IS_ERR_OR_NULL(res))
@@ -1746,7 +1746,7 @@ static inline int handle_dots(struct nameidata *nd, int type)
* cause our parent to have moved outside of the root and us to skip
* over it.
*/
- if (unlikely(nd->flags & LOOKUP_BENEATH))
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
return -EXDEV;
if (!nd->root.mnt)
set_root(nd);
@@ -2297,7 +2297,7 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->m_seq = read_seqbegin(&mount_lock);
- if (unlikely(nd->flags & LOOKUP_BENEATH)) {
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
return ERR_PTR(error);
diff --git a/fs/open.c b/fs/open.c
index 3e73f940f56e..5914aed2fac8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -960,7 +960,7 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
* cannot have anything other than the below set of flags
*/
flags &= O_DIRECTORY | O_NOFOLLOW | O_PATH | O_BENEATH |
- O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS;
+ O_XDEV | O_NOSYMLINKS | O_NOMAGICLINKS | O_THISROOT;
acc_mode = 0;
}
@@ -997,6 +997,8 @@ static inline int build_open_flags(int flags, umode_t mode, struct open_flags *o
lookup_flags |= LOOKUP_NO_MAGICLINKS;
if (flags & O_NOSYMLINKS)
lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & O_THISROOT)
+ lookup_flags |= LOOKUP_IN_ROOT;
op->lookup_flags = lookup_flags;
return 0;
}
diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
index 864399c2fdd2..46c92bbfce4a 100644
--- a/include/linux/fcntl.h
+++ b/include/linux/fcntl.h
@@ -10,7 +10,7 @@
O_APPEND | O_NDELAY | O_NONBLOCK | O_NDELAY | __O_SYNC | O_DSYNC | \
FASYNC | O_DIRECT | O_LARGEFILE | O_DIRECTORY | O_NOFOLLOW | \
O_NOATIME | O_CLOEXEC | O_PATH | __O_TMPFILE | O_BENEATH | O_XDEV | \
- O_NOMAGICLINKS | O_NOSYMLINKS)
+ O_NOMAGICLINKS | O_NOSYMLINKS | O_THISROOT)
#ifndef force_o_largefile
#define force_o_largefile() (BITS_PER_LONG != 32)
diff --git a/include/linux/namei.h b/include/linux/namei.h
index 82b5039d27a6..0969313b518f 100644
--- a/include/linux/namei.h
+++ b/include/linux/namei.h
@@ -53,6 +53,7 @@ enum {LAST_NORM, LAST_ROOT, LAST_DOT, LAST_DOTDOT, LAST_BIND};
#define LOOKUP_NO_MAGICLINKS 0x040000 /* No /proc/$pid/fd/ "symlink" crossing. */
#define LOOKUP_NO_SYMLINKS 0x080000 /* No symlink crossing *at all*.
Implies LOOKUP_NO_MAGICLINKS. */
+#define LOOKUP_IN_ROOT 0x100000 /* Treat dirfd as %current->fs->root. */
extern int path_pts(struct path *path);
diff --git a/include/uapi/asm-generic/fcntl.h b/include/uapi/asm-generic/fcntl.h
index ba53e0836cd4..3d0fe0de2ba2 100644
--- a/include/uapi/asm-generic/fcntl.h
+++ b/include/uapi/asm-generic/fcntl.h
@@ -110,6 +110,9 @@
#ifndef O_NOSYMLINKS
#define O_NOSYMLINKS 01000000000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
#endif
+#ifndef O_THISROOT
+#define O_THISROOT 02000000000 /* - Scope ".." resolution to dirfd (like chroot(2)). */
+#endif
#define F_DUPFD 0 /* dup */
#define F_GETFD 1 /* get close_on_exec */
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 4/5] namei: aggressively check for nd->root escape on ".." resolution
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Jann Horn, Kees Cook, Eric Biederman,
Andy Lutomirski, Andrew Morton, Alexei Starovoitov,
Christian Brauner, David Drysdale, Chanho Min, Oleg Nesterov,
Aleksa Sarai, Linus Torvalds, containers, linux-fsdevel,
linux-api, linux-kernel, linux-arch
In-Reply-To: <20190320143717.2523-1-cyphar@cyphar.com>
This patch allows for O_BENEATH and O_THISROOT to safely permit ".."
resolution (in the case of O_BENEATH the resolution will still fail if
".." resolution would resolve a path outside of the root -- while
O_THISROOT will chroot(2)-style scope it). "magic link" jumps are still
disallowed entirely because now they could result in inconsistent
behaviour if resolution encounters a subsequent "..".
The need for this patch is explained by observing there is a fairly
easy-to-exploit race condition with chroot(2) (and thus by extension
O_THISROOT and O_BENEATH) where a rename(2) of a path can be used to
"skip over" nd->root and thus escape to the filesystem above nd->root.
thread1 [attacker]:
for (;;)
renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
thread2 [victim]:
for (;;)
openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
With fairly significant regularity, thread2 will resolve to
"/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
(though somewhat more privileged) attack using MS_MOVE.
With this patch, such cases will be detected *during* ".." resolution
(which is the weak point of chroot(2) -- since walking *into* a
subdirectory tautologically cannot result in you walking *outside*
nd->root -- except through a bind-mount or "magic link"). By detecting
this at ".." resolution (rather than checking only at the end of the
entire resolution) we can both correct escapes by jumping back to the
root (in the case of O_THISROOT), as well as avoid revealing to
attackers the structure of the filesystem outside of the root (through
timing attacks for instance).
In order to avoid a quadratic lookup with each ".." entry, we only
activate the slow path if a write through &rename_lock or &mount_lock
have occurred during path resolution (&rename_lock and &mount_lock are
re-taken to further optimise the lookup). Since the primary attack being
protected against is MS_MOVE or rename(2), not doing additional checks
unless a mount or rename have occurred avoids making the common case
slow.
The use of path_is_under() here might seem suspect, but on further
inspection of the most important race (a path was *inside* the root but
is now *outside*), there appears to be no attack potential. If
path_is_under() occurs before the rename, then the path will be resolved
but since the path was originally inside the root there is no escape.
Subsequent ".." jumps are guaranteed to check path_is_under() (by
construction, &rename_lock or &mount_lock must have been taken by the
attacker after path_is_under() returned in the victim), and thus will
not be able to escape from the previously-inside-root path. Walking down
is still safe since the entire subtree was moved (either by rename(2) or
MS_MOVE) and because (as discussed above) walking down is safe.
Cc: Al Viro <viro@zeniv.linux.org.uk>
Cc: Jann Horn <jannh@google.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/namei.c | 48 +++++++++++++++++++++++++++++++++---------------
1 file changed, 33 insertions(+), 15 deletions(-)
diff --git a/fs/namei.c b/fs/namei.c
index 798eb1702a0c..af06b10b13a0 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -493,7 +493,7 @@ struct nameidata {
struct path root;
struct inode *inode; /* path.dentry.d_inode */
unsigned int flags;
- unsigned seq, m_seq;
+ unsigned seq, m_seq, r_seq;
int last_type;
unsigned depth;
int total_link_count;
@@ -1741,19 +1741,35 @@ static inline int may_lookup(struct nameidata *nd)
static inline int handle_dots(struct nameidata *nd, int type)
{
if (type == LAST_DOTDOT) {
- /*
- * LOOKUP_BENEATH resolving ".." is not currently safe -- races can
- * cause our parent to have moved outside of the root and us to skip
- * over it.
- */
- if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
- return -EXDEV;
+ int error = 0;
+
if (!nd->root.mnt)
set_root(nd);
- if (nd->flags & LOOKUP_RCU) {
- return follow_dotdot_rcu(nd);
- } else
- return follow_dotdot(nd);
+ if (nd->flags & LOOKUP_RCU)
+ error = follow_dotdot_rcu(nd);
+ else
+ error = follow_dotdot(nd);
+ if (error)
+ return error;
+
+ if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
+ bool m_retry = read_seqretry(&mount_lock, nd->m_seq);
+ bool r_retry = read_seqretry(&rename_lock, nd->r_seq);
+
+ /*
+ * Don't bother checking unless there's a racing
+ * rename(2) or MS_MOVE.
+ */
+ if (likely(!m_retry && !r_retry))
+ return 0;
+
+ if (m_retry && !(nd->flags & LOOKUP_RCU))
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (r_retry)
+ nd->r_seq = read_seqbegin(&rename_lock);
+ if (!path_is_under(&nd->path, &nd->root))
+ return -EXDEV;
+ }
}
return 0;
}
@@ -2274,6 +2290,11 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->last_type = LAST_ROOT; /* if there are only slashes... */
nd->flags = flags | LOOKUP_JUMPED | LOOKUP_PARENT;
nd->depth = 0;
+
+ nd->m_seq = read_seqbegin(&mount_lock);
+ if (unlikely(flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT)))
+ nd->r_seq = read_seqbegin(&rename_lock);
+
if (flags & LOOKUP_ROOT) {
struct dentry *root = nd->root.dentry;
struct inode *inode = root->d_inode;
@@ -2284,7 +2305,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
if (flags & LOOKUP_RCU) {
nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);
nd->root_seq = nd->seq;
- nd->m_seq = read_seqbegin(&mount_lock);
} else {
path_get(&nd->path);
}
@@ -2295,8 +2315,6 @@ static const char *path_init(struct nameidata *nd, unsigned flags)
nd->path.mnt = NULL;
nd->path.dentry = NULL;
- nd->m_seq = read_seqbegin(&mount_lock);
-
if (unlikely(nd->flags & (LOOKUP_BENEATH | LOOKUP_IN_ROOT))) {
error = dirfd_path_init(nd);
if (unlikely(error))
--
2.21.0
^ permalink raw reply related
* [PATCH RESEND v5 5/5] binfmt_*: scope path resolution of interpreters
From: Aleksa Sarai @ 2019-03-20 14:37 UTC (permalink / raw)
To: Al Viro, Jeff Layton, J. Bruce Fields, Arnd Bergmann,
David Howells
Cc: Aleksa Sarai, Eric Biederman, Andy Lutomirski, Andrew Morton,
Alexei Starovoitov, Kees Cook, Jann Horn, Christian Brauner,
David Drysdale, Chanho Min, Oleg Nesterov, Aleksa Sarai,
Linus Torvalds, containers, linux-fsdevel, linux-api,
linux-kernel, linux-arch
In-Reply-To: <20190320143717.2523-1-cyphar@cyphar.com>
The need to be able to scope path resolution of interpreters became
clear with one of the possible vectors used in CVE-2019-5736 (which
most major container runtimes were vulnerable to).
Naively, it might seem that openat(2) -- which supports path scoping --
can be combined with execveat(AT_EMPTY_PATH) to trivially scope the
binary being executed. Unfortunately, a "bad binary" (usually a symlink)
could be written as a #!-style script with the symlink target as the
interpreter -- which would be completely missed by just scoping the
openat(2). An example of this being exploitable is CVE-2019-5736.
In order to get around this, we need to pass down to each binfmt_*
implementation the scoping flags requested in execveat(2). In order to
maintain backwards-compatibility we only pass the scoping AT_* flags.
To avoid breaking userspace (in the exceptionally rare cases where you
have #!-scripts with a relative path being execveat(2)-ed with dfd !=
AT_FDCWD), we only pass dfd down to binfmt_* if any of our new flags are
set in execveat(2).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
fs/binfmt_elf.c | 2 +-
fs/binfmt_elf_fdpic.c | 2 +-
fs/binfmt_em86.c | 4 ++--
fs/binfmt_misc.c | 2 +-
fs/binfmt_script.c | 2 +-
fs/exec.c | 26 ++++++++++++++++++++++----
include/linux/binfmts.h | 1 +
include/linux/fs.h | 9 +++++++--
include/uapi/linux/fcntl.h | 6 ++++++
9 files changed, 42 insertions(+), 12 deletions(-)
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 54207327f98f..eef86ffa38c8 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -777,7 +777,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
if (elf_interpreter[elf_ppnt->p_filesz - 1] != '\0')
goto out_free_interp;
- interpreter = open_exec(elf_interpreter);
+ interpreter = openat_exec(bprm->dfd, elf_interpreter, bprm->flags);
retval = PTR_ERR(interpreter);
if (IS_ERR(interpreter))
goto out_free_interp;
diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c
index b53bb3729ac1..c463c6428f77 100644
--- a/fs/binfmt_elf_fdpic.c
+++ b/fs/binfmt_elf_fdpic.c
@@ -263,7 +263,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm)
kdebug("Using ELF interpreter %s", interpreter_name);
/* replace the program with the interpreter */
- interpreter = open_exec(interpreter_name);
+ interpreter = openat_exec(bprm->dfd, interpreter_name, bprm->flags);
retval = PTR_ERR(interpreter);
if (IS_ERR(interpreter)) {
interpreter = NULL;
diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index dd2d3f0cd55d..3ee46b0dc0d4 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -81,10 +81,10 @@ static int load_em86(struct linux_binprm *bprm)
/*
* OK, now restart the process with the interpreter's inode.
- * Note that we use open_exec() as the name is now in kernel
+ * Note that we use openat_exec() as the name is now in kernel
* space, and we don't need to copy it.
*/
- file = open_exec(interp);
+ file = openat_exec(binprm->dfd, interp, binprm->flags);
if (IS_ERR(file))
return PTR_ERR(file);
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index aa4a7a23ff99..573ef06ff5a1 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -209,7 +209,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
if (!IS_ERR(interp_file))
deny_write_access(interp_file);
} else {
- interp_file = open_exec(fmt->interpreter);
+ interp_file = openat_exec(bprm->dfd, fmt->interpreter, bprm->flags);
}
retval = PTR_ERR(interp_file);
if (IS_ERR(interp_file))
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index d0078cbb718b..340f63635aac 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -102,7 +102,7 @@ static int load_script(struct linux_binprm *bprm)
/*
* OK, now restart the process with the interpreter's dentry.
*/
- file = open_exec(i_name);
+ file = openat_exec(bprm->dfd, i_name, bprm->flags);
if (IS_ERR(file))
return PTR_ERR(file);
diff --git a/fs/exec.c b/fs/exec.c
index bcf383730bea..e63063b2de23 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -846,12 +846,24 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
.lookup_flags = LOOKUP_FOLLOW,
};
- if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH)) != 0)
+ if ((flags & ~(AT_SYMLINK_NOFOLLOW | AT_EMPTY_PATH | AT_BENEATH |
+ AT_XDEV | AT_NO_MAGICLINKS | AT_NO_SYMLINKS |
+ AT_THIS_ROOT)) != 0)
return ERR_PTR(-EINVAL);
if (flags & AT_SYMLINK_NOFOLLOW)
open_exec_flags.lookup_flags &= ~LOOKUP_FOLLOW;
if (flags & AT_EMPTY_PATH)
open_exec_flags.lookup_flags |= LOOKUP_EMPTY;
+ if (flags & AT_BENEATH)
+ open_exec_flags.lookup_flags |= LOOKUP_BENEATH;
+ if (flags & AT_XDEV)
+ open_exec_flags.lookup_flags |= LOOKUP_XDEV;
+ if (flags & AT_NO_MAGICLINKS)
+ open_exec_flags.lookup_flags |= LOOKUP_NO_MAGICLINKS;
+ if (flags & AT_NO_SYMLINKS)
+ open_exec_flags.lookup_flags |= LOOKUP_NO_SYMLINKS;
+ if (flags & AT_THIS_ROOT)
+ open_exec_flags.lookup_flags |= LOOKUP_IN_ROOT;
file = do_filp_open(fd, name, &open_exec_flags);
if (IS_ERR(file))
@@ -879,18 +891,18 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
return ERR_PTR(err);
}
-struct file *open_exec(const char *name)
+struct file *openat_exec(int dfd, const char *name, int flags)
{
struct filename *filename = getname_kernel(name);
struct file *f = ERR_CAST(filename);
if (!IS_ERR(filename)) {
- f = do_open_execat(AT_FDCWD, filename, 0);
+ f = do_open_execat(dfd, filename, flags);
putname(filename);
}
return f;
}
-EXPORT_SYMBOL(open_exec);
+EXPORT_SYMBOL(openat_exec);
int kernel_read_file(struct file *file, void **buf, loff_t *size,
loff_t max_size, enum kernel_read_file_id id)
@@ -1762,6 +1774,12 @@ static int __do_execve_file(int fd, struct filename *filename,
sched_exec();
+ bprm->flags = flags & (AT_XDEV | AT_NO_MAGICLINKS | AT_NO_SYMLINKS |
+ AT_THIS_ROOT);
+ bprm->dfd = AT_FDCWD;
+ if (bprm->flags)
+ bprm->dfd = fd;
+
bprm->file = file;
if (!filename) {
bprm->filename = "none";
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 688ab0de7810..e4da2d36e97f 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -50,6 +50,7 @@ struct linux_binprm {
unsigned int taso:1;
#endif
unsigned int recursion_depth; /* only for search_binary_handler() */
+ int dfd, flags; /* passed down to execat_open() */
struct file * file;
struct cred *cred; /* new credentials */
int unsafe; /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 3e85cb8e8c20..a82c8dd44ad9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2937,8 +2937,13 @@ extern int kernel_read_file_from_fd(int, void **, loff_t *, loff_t,
extern ssize_t kernel_read(struct file *, void *, size_t, loff_t *);
extern ssize_t kernel_write(struct file *, const void *, size_t, loff_t *);
extern ssize_t __kernel_write(struct file *, const void *, size_t, loff_t *);
-extern struct file * open_exec(const char *);
-
+
+extern struct file *openat_exec(int, const char *, int);
+static inline struct file *open_exec(const char *name)
+{
+ return openat_exec(AT_FDCWD, name, 0);
+}
+
/* fs/dcache.c -- generic fs support functions */
extern bool is_subdir(struct dentry *, struct dentry *);
extern bool path_is_under(const struct path *, const struct path *);
diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 6448cdd9a350..607bc98813e3 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -90,5 +90,11 @@
#define AT_STATX_FORCE_SYNC 0x2000 /* - Force the attributes to be sync'd with the server */
#define AT_STATX_DONT_SYNC 0x4000 /* - Don't sync attributes with the server */
+#define AT_RESOLUTION_TYPE 0xF8000 /* Type of path-resolution scoping we are applying. */
+#define AT_BENEATH 0x08000 /* - Block "lexical" trickery like "..", symlinks, absolute paths, etc. */
+#define AT_XDEV 0x10000 /* - Block mount-point crossings (includes bind-mounts). */
+#define AT_NO_MAGICLINKS 0x20000 /* - Block procfs-style "magic" symlinks. */
+#define AT_NO_SYMLINKS 0x40000 /* - Block all symlinks (implies AT_NO_MAGICLINKS). */
+#define AT_THIS_ROOT 0x80000 /* - Scope ".." resolution to dirfd (like chroot(2)). */
#endif /* _UAPI_LINUX_FCNTL_H */
--
2.21.0
^ permalink raw reply related
* Re: [net-next PATCH v3 4/8] net: Change return type of sk_busy_loop from bool to void
From: Christoph Paasch @ 2019-03-20 18:35 UTC (permalink / raw)
To: Alexander Duyck
Cc: netdev, linux-kernel, sridhar.samudrala, Eric Dumazet,
David Miller, linux-api
In-Reply-To: <20170324170812.15226.97497.stgit@localhost.localdomain>
Hello,
On Fri, Mar 24, 2017 at 3:23 PM Alexander Duyck
<alexander.duyck@gmail.com> wrote:
>
> From: Alexander Duyck <alexander.h.duyck@intel.com>
>
> >From what I can tell there is only a couple spots where we are actually
> checking the return value of sk_busy_loop. As there are only a few
> consumers of that data, and the data being checked for can be replaced
> with a check for !skb_queue_empty() we might as well just pull the code
> out of sk_busy_loop and place it in the spots that actually need it.
>
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
> ---
> include/net/busy_poll.h | 5 ++---
> net/core/datagram.c | 8 ++++++--
> net/core/dev.c | 25 +++++++++++--------------
> net/sctp/socket.c | 9 ++++++---
> 4 files changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/include/net/busy_poll.h b/include/net/busy_poll.h
> index b82d6ba70a14..c55760f4820f 100644
> --- a/include/net/busy_poll.h
> +++ b/include/net/busy_poll.h
> @@ -74,7 +74,7 @@ static inline bool busy_loop_timeout(unsigned long end_time)
> return time_after(now, end_time);
> }
>
> -bool sk_busy_loop(struct sock *sk, int nonblock);
> +void sk_busy_loop(struct sock *sk, int nonblock);
>
> #else /* CONFIG_NET_RX_BUSY_POLL */
> static inline unsigned long net_busy_loop_on(void)
> @@ -97,9 +97,8 @@ static inline bool busy_loop_timeout(unsigned long end_time)
> return true;
> }
>
> -static inline bool sk_busy_loop(struct sock *sk, int nonblock)
> +static inline void sk_busy_loop(struct sock *sk, int nonblock)
> {
> - return false;
> }
>
> #endif /* CONFIG_NET_RX_BUSY_POLL */
> diff --git a/net/core/datagram.c b/net/core/datagram.c
> index ea633342ab0d..4608aa245410 100644
> --- a/net/core/datagram.c
> +++ b/net/core/datagram.c
> @@ -256,8 +256,12 @@ struct sk_buff *__skb_try_recv_datagram(struct sock *sk, unsigned int flags,
> }
>
> spin_unlock_irqrestore(&queue->lock, cpu_flags);
> - } while (sk_can_busy_loop(sk) &&
> - sk_busy_loop(sk, flags & MSG_DONTWAIT));
> +
> + if (!sk_can_busy_loop(sk))
> + break;
> +
> + sk_busy_loop(sk, flags & MSG_DONTWAIT);
> + } while (!skb_queue_empty(&sk->sk_receive_queue));
since this change I am hitting stalls where it's looping in this
while-loop with syzkaller.
It worked prior to this change because sk->sk_napi_id was not set thus
sk_busy_loop would make us get out of the loop.
Now, it keeps on looping because there is an skb in the queue with
skb->len == 0 and we are peeking with an offset, thus
__skb_try_recv_from_queue will return NULL and thus we have no way of
getting out of the loop.
I'm not sure what would be the best way to fix it. I don't see why we
end up with an skb in the list with skb->len == 0. So, shooting a
quick e-mail, maybe somebody has an idea :-)
I have the syzkaller-reproducer if needed.
Thanks,
Christoph
>
> error = -EAGAIN;
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index ab337bf5bbf4..af70eb6ba682 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -5060,21 +5060,19 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock)
> do_softirq();
> }
>
> -bool sk_busy_loop(struct sock *sk, int nonblock)
> +void sk_busy_loop(struct sock *sk, int nonblock)
> {
> unsigned long end_time = !nonblock ? sk_busy_loop_end_time(sk) : 0;
> int (*napi_poll)(struct napi_struct *napi, int budget);
> void *have_poll_lock = NULL;
> struct napi_struct *napi;
> unsigned int napi_id;
> - int rc;
>
> restart:
> napi_id = READ_ONCE(sk->sk_napi_id);
> if (napi_id < MIN_NAPI_ID)
> - return 0;
> + return;
>
> - rc = false;
> napi_poll = NULL;
>
> rcu_read_lock();
> @@ -5085,7 +5083,8 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
>
> preempt_disable();
> for (;;) {
> - rc = 0;
> + int work = 0;
> +
> local_bh_disable();
> if (!napi_poll) {
> unsigned long val = READ_ONCE(napi->state);
> @@ -5103,12 +5102,12 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
> have_poll_lock = netpoll_poll_lock(napi);
> napi_poll = napi->poll;
> }
> - rc = napi_poll(napi, BUSY_POLL_BUDGET);
> - trace_napi_poll(napi, rc, BUSY_POLL_BUDGET);
> + work = napi_poll(napi, BUSY_POLL_BUDGET);
> + trace_napi_poll(napi, work, BUSY_POLL_BUDGET);
> count:
> - if (rc > 0)
> + if (work > 0)
> __NET_ADD_STATS(sock_net(sk),
> - LINUX_MIB_BUSYPOLLRXPACKETS, rc);
> + LINUX_MIB_BUSYPOLLRXPACKETS, work);
> local_bh_enable();
>
> if (nonblock || !skb_queue_empty(&sk->sk_receive_queue) ||
> @@ -5121,9 +5120,9 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
> preempt_enable();
> rcu_read_unlock();
> cond_resched();
> - rc = !skb_queue_empty(&sk->sk_receive_queue);
> - if (rc || busy_loop_timeout(end_time))
> - return rc;
> + if (!skb_queue_empty(&sk->sk_receive_queue) ||
> + busy_loop_timeout(end_time))
> + return;
> goto restart;
> }
> cpu_relax();
> @@ -5131,10 +5130,8 @@ bool sk_busy_loop(struct sock *sk, int nonblock)
> if (napi_poll)
> busy_poll_stop(napi, have_poll_lock);
> preempt_enable();
> - rc = !skb_queue_empty(&sk->sk_receive_queue);
> out:
> rcu_read_unlock();
> - return rc;
> }
> EXPORT_SYMBOL(sk_busy_loop);
>
> diff --git a/net/sctp/socket.c b/net/sctp/socket.c
> index 72cc3ecf6516..ccc08fc39722 100644
> --- a/net/sctp/socket.c
> +++ b/net/sctp/socket.c
> @@ -7518,9 +7518,12 @@ struct sk_buff *sctp_skb_recv_datagram(struct sock *sk, int flags,
> if (sk->sk_shutdown & RCV_SHUTDOWN)
> break;
>
> - if (sk_can_busy_loop(sk) &&
> - sk_busy_loop(sk, noblock))
> - continue;
> + if (sk_can_busy_loop(sk)) {
> + sk_busy_loop(sk, noblock);
> +
> + if (!skb_queue_empty(&sk->sk_receive_queue))
> + continue;
> + }
>
> /* User doesn't want to wait. */
> error = -EAGAIN;
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox