* [PATCH ghak90 V8 12/16] audit: contid check descendancy and nesting
From: Richard Guy Briggs @ 2019-12-31 19:48 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: nhorman, Richard Guy Briggs, dhowells, ebiederm, simo, eparis,
mpatel, serge
In-Reply-To: <cover.1577736799.git.rgb@redhat.com>
Require the target task to be a descendant of the container
orchestrator/engine.
You would only change the audit container ID from one set or inherited
value to another if you were nesting containers.
If changing the contid, the container orchestrator/engine must be a
descendant and not same orchestrator as the one that set it so it is not
possible to change the contid of another orchestrator's container.
Since the task_is_descendant() function is used in YAMA and in audit,
remove the duplication and pull the function into kernel/core/sched.c
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/sched.h | 3 +++
kernel/audit.c | 44 ++++++++++++++++++++++++++++++++++++--------
kernel/sched/core.c | 33 +++++++++++++++++++++++++++++++++
security/yama/yama_lsm.c | 33 ---------------------------------
4 files changed, 72 insertions(+), 41 deletions(-)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index aebe24192b23..009d2cb2e2bf 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2006,4 +2006,7 @@ static inline void rseq_syscall(struct pt_regs *regs)
const struct cpumask *sched_trace_rd_span(struct root_domain *rd);
+extern int task_is_descendant(struct task_struct *parent,
+ struct task_struct *child);
+
#endif
diff --git a/kernel/audit.c b/kernel/audit.c
index f7a8d3288ca0..ef8e07524c46 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2575,6 +2575,13 @@ int audit_signal_info(int sig, struct task_struct *t)
return audit_signal_info_syscall(t);
}
+static bool audit_contid_isowner(struct task_struct *tsk)
+{
+ if (tsk->audit && tsk->audit->cont)
+ return current == tsk->audit->cont->owner;
+ return false;
+}
+
/*
* audit_set_contid - set current task's audit contid
* @task: target task
@@ -2603,22 +2610,43 @@ int audit_set_contid(struct task_struct *task, u64 contid)
oldcontid = audit_get_contid(task);
read_lock(&tasklist_lock);
/* Don't allow the contid to be unset */
- if (!audit_contid_valid(contid))
+ if (!audit_contid_valid(contid)) {
rc = -EINVAL;
+ goto unlock;
+ }
/* Don't allow the contid to be set to the same value again */
- else if (contid == oldcontid) {
+ if (contid == oldcontid) {
rc = -EADDRINUSE;
+ goto unlock;
+ }
/* if we don't have caps, reject */
- else if (!capable(CAP_AUDIT_CONTROL))
+ if (!capable(CAP_AUDIT_CONTROL)) {
rc = -EPERM;
- /* if task has children or is not single-threaded, deny */
- else if (!list_empty(&task->children))
+ goto unlock;
+ }
+ /* if task has children, deny */
+ if (!list_empty(&task->children)) {
rc = -EBUSY;
- else if (!(thread_group_leader(task) && thread_group_empty(task)))
+ goto unlock;
+ }
+ /* if task is not single-threaded, deny */
+ if (!(thread_group_leader(task) && thread_group_empty(task))) {
rc = -EALREADY;
- /* if contid is already set, deny */
- else if (audit_contid_set(task))
+ goto unlock;
+ }
+ /* if task is not descendant, block */
+ if (task == current) {
+ rc = -EBADSLT;
+ goto unlock;
+ }
+ if (!task_is_descendant(current, task)) {
+ rc = -EXDEV;
+ goto unlock;
+ }
+ /* only allow contid setting again if nesting */
+ if (audit_contid_set(task) && audit_contid_isowner(task))
rc = -ECHILD;
+unlock:
read_unlock(&tasklist_lock);
if (!rc) {
struct audit_contobj *oldcont = _audit_contobj(task);
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 90e4b00ace89..7d8145285eb9 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -7916,6 +7916,39 @@ void dump_cpu_task(int cpu)
}
/*
+ * task_is_descendant - walk up a process family tree looking for a match
+ * @parent: the process to compare against while walking up from child
+ * @child: the process to start from while looking upwards for parent
+ *
+ * Returns 1 if child is a descendant of parent, 0 if not.
+ */
+int task_is_descendant(struct task_struct *parent,
+ struct task_struct *child)
+{
+ int rc = 0;
+ struct task_struct *walker = child;
+
+ if (!parent || !child)
+ return 0;
+
+ rcu_read_lock();
+ if (!thread_group_leader(parent))
+ parent = rcu_dereference(parent->group_leader);
+ while (walker->pid > 0) {
+ if (!thread_group_leader(walker))
+ walker = rcu_dereference(walker->group_leader);
+ if (walker == parent) {
+ rc = 1;
+ break;
+ }
+ walker = rcu_dereference(walker->real_parent);
+ }
+ rcu_read_unlock();
+
+ return rc;
+}
+
+/*
* Nice levels are multiplicative, with a gentle 10% change for every
* nice level changed. I.e. when a CPU-bound task goes from nice 0 to
* nice 1, it will get ~10% less CPU time than another CPU-bound task
diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index 94dc346370b1..25eae205eae8 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -263,39 +263,6 @@ static int yama_task_prctl(int option, unsigned long arg2, unsigned long arg3,
}
/**
- * task_is_descendant - walk up a process family tree looking for a match
- * @parent: the process to compare against while walking up from child
- * @child: the process to start from while looking upwards for parent
- *
- * Returns 1 if child is a descendant of parent, 0 if not.
- */
-static int task_is_descendant(struct task_struct *parent,
- struct task_struct *child)
-{
- int rc = 0;
- struct task_struct *walker = child;
-
- if (!parent || !child)
- return 0;
-
- rcu_read_lock();
- if (!thread_group_leader(parent))
- parent = rcu_dereference(parent->group_leader);
- while (walker->pid > 0) {
- if (!thread_group_leader(walker))
- walker = rcu_dereference(walker->group_leader);
- if (walker == parent) {
- rc = 1;
- break;
- }
- walker = rcu_dereference(walker->real_parent);
- }
- rcu_read_unlock();
-
- return rc;
-}
-
-/**
* ptracer_exception_found - tracer registered as exception for this tracee
* @tracer: the task_struct of the process attempting ptrace
* @tracee: the task_struct of the process to be ptraced
--
1.8.3.1
^ permalink raw reply related
* [PATCH ghak90 V8 13/16] audit: track container nesting
From: Richard Guy Briggs @ 2019-12-31 19:48 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: nhorman, Richard Guy Briggs, dhowells, ebiederm, simo, eparis,
mpatel, serge
In-Reply-To: <cover.1577736799.git.rgb@redhat.com>
Track the parent container of a container to be able to filter and
report nesting.
Now that we have a way to track and check the parent container of a
container, modify the contid field format to be able to report that
nesting using a carrat ("^") separator to indicate nesting. The
original field format was "contid=<contid>" for task-associated records
and "contid=<contid>[,<contid>[...]]" for network-namespace-associated
records. The new field format is
"contid=<contid>[^<contid>[...]][,<contid>[...]]".
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/audit.h | 1 +
kernel/audit.c | 53 +++++++++++++++++++++++++++++++++++++++++++--------
kernel/audit.h | 1 +
kernel/auditfilter.c | 17 ++++++++++++++++-
kernel/auditsc.c | 2 +-
5 files changed, 64 insertions(+), 10 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index ed8d5b74758d..4272b468417a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -109,6 +109,7 @@ struct audit_contobj {
struct task_struct *owner;
refcount_t refcount;
struct rcu_head rcu;
+ struct audit_contobj *parent;
};
struct audit_task_info {
diff --git a/kernel/audit.c b/kernel/audit.c
index ef8e07524c46..68be59d1a89b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -251,6 +251,7 @@ static void _audit_contobj_put(struct audit_contobj *cont)
return;
if (refcount_dec_and_test(&cont->refcount)) {
put_task_struct(cont->owner);
+ _audit_contobj_put(cont->parent);
list_del_rcu(&cont->list);
kfree_rcu(cont, rcu);
}
@@ -492,6 +493,7 @@ void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
audit_netns_contid_add(new->net_ns, contid);
}
+void audit_log_contid(struct audit_buffer *ab, u64 contid);
/**
* audit_log_netns_contid_list - List contids for the given network namespace
* @net: the network namespace of interest
@@ -523,7 +525,7 @@ void audit_log_netns_contid_list(struct net *net, struct audit_context *context)
audit_log_format(ab, "contid=");
} else
audit_log_format(ab, ",");
- audit_log_format(ab, "%llu", cont->id);
+ audit_log_contid(ab, cont->id);
}
audit_log_end(ab);
out:
@@ -2311,6 +2313,36 @@ void audit_log_session_info(struct audit_buffer *ab)
audit_log_format(ab, "auid=%u ses=%u", auid, sessionid);
}
+void audit_log_contid(struct audit_buffer *ab, u64 contid)
+{
+ struct audit_contobj *cont = NULL, *prcont = NULL;
+ int h;
+
+ if (!audit_contid_valid(contid)) {
+ audit_log_format(ab, "%llu", contid);
+ return;
+ }
+ h = audit_hash_contid(contid);
+ rcu_read_lock();
+ list_for_each_entry_rcu(cont, &audit_contid_hash[h], list)
+ if (cont->id == contid) {
+ prcont = cont;
+ break;
+ }
+ if (!prcont) {
+ audit_log_format(ab, "%llu", contid);
+ goto out;
+ }
+ while (prcont) {
+ audit_log_format(ab, "%llu", prcont->id);
+ prcont = prcont->parent;
+ if (prcont)
+ audit_log_format(ab, "^");
+ }
+out:
+ rcu_read_unlock();
+}
+
/*
* audit_log_container_id - report container info
* @context: task or local context for record
@@ -2326,7 +2358,8 @@ void audit_log_container_id(struct audit_context *context, u64 contid)
ab = audit_log_start(context, GFP_KERNEL, AUDIT_CONTAINER_ID);
if (!ab)
return;
- audit_log_format(ab, "contid=%llu", contid);
+ audit_log_format(ab, "contid=");
+ audit_log_contid(ab, contid);
audit_log_end(ab);
}
EXPORT_SYMBOL(audit_log_container_id);
@@ -2675,6 +2708,9 @@ int audit_set_contid(struct task_struct *task, u64 contid)
newcont->id = contid;
get_task_struct(current);
newcont->owner = current;
+ newcont->parent = _audit_contobj(newcont->owner);
+ if (newcont->parent)
+ _audit_contobj_hold(newcont->parent);
refcount_set(&newcont->refcount, 1);
spin_lock(&audit_contobj_list_lock);
list_add_rcu(&newcont->list, &audit_contid_hash[h]);
@@ -2705,9 +2741,10 @@ int audit_set_contid(struct task_struct *task, u64 contid)
if (!ab)
return rc;
- audit_log_format(ab,
- "op=set opid=%d contid=%llu old-contid=%llu",
- task_tgid_nr(task), contid, oldcontid);
+ audit_log_format(ab, "op=set opid=%d contid=", task_tgid_nr(task));
+ audit_log_contid(ab, contid);
+ audit_log_format(ab, " old-contid=");
+ audit_log_contid(ab, oldcontid);
audit_log_end(ab);
return rc;
}
@@ -2723,9 +2760,9 @@ void audit_log_container_drop(void)
if (!ab)
return;
- audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=%llu",
- task_tgid_nr(current), audit_get_contid(current),
- audit_get_contid(current));
+ audit_log_format(ab, "op=drop opid=%d contid=%llu old-contid=",
+ task_tgid_nr(current), AUDIT_CID_UNSET);
+ audit_log_contid(ab, audit_get_contid(current));
audit_log_end(ab);
}
diff --git a/kernel/audit.h b/kernel/audit.h
index 5e2f5c9820d8..de814fcbb38c 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -226,6 +226,7 @@ static inline int audit_hash_contid(u64 contid)
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_contid_comparator(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 9606f973fe33..1757896740e8 100644
--- a/kernel/auditfilter.c
+++ b/kernel/auditfilter.c
@@ -1297,6 +1297,21 @@ int audit_gid_comparator(kgid_t left, u32 op, kgid_t right)
}
}
+int audit_contid_comparator(u64 left, u32 op, u64 right)
+{
+ struct audit_contobj *cont = NULL;
+ int h;
+ int result = 0;
+
+ h = audit_hash_contid(left);
+ list_for_each_entry_rcu(cont, &audit_contid_hash[h], list) {
+ result = audit_comparator64(cont->id, op, right);
+ if (result)
+ break;
+ }
+ return result;
+}
+
/**
* parent_len - find the length of the parent portion of a pathname
* @path: pathname of which to determine length
@@ -1388,7 +1403,7 @@ int audit_filter(int msgtype, unsigned int listtype)
f->op, f->val);
break;
case AUDIT_CONTID:
- result = audit_comparator64(audit_get_contid(current),
+ result = audit_contid_comparator(audit_get_contid(current),
f->op, f->val64);
break;
case AUDIT_MSGTYPE:
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a658fe775b86..6bf6d8b9dfd1 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -630,7 +630,7 @@ static int audit_filter_rules(struct task_struct *tsk,
f->op, f->val);
break;
case AUDIT_CONTID:
- result = audit_comparator64(audit_get_contid(tsk),
+ result = audit_contid_comparator(audit_get_contid(tsk),
f->op, f->val64);
break;
case AUDIT_SUBJ_USER:
--
1.8.3.1
^ permalink raw reply related
* [PATCH ghak90 V8 14/16] audit: check contid depth and add limit config param
From: Richard Guy Briggs @ 2019-12-31 19:48 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: Paul Moore, sgrubb, omosnace, dhowells, simo, eparis, serge,
ebiederm, nhorman, dwalsh, mpatel, Richard Guy Briggs
In-Reply-To: <cover.1577736799.git.rgb@redhat.com>
Clamp the depth of audit container identifier nesting to limit the
netlink and disk bandwidth used and to prevent losing information from
record text size overflow in the contid field.
Add a configuration parameter AUDIT_STATUS_CONTID_DEPTH_LIMIT (0x80) to
set the audit container identifier depth limit. This can be used to
prevent overflow of the contid field in CONTAINER_OP and CONTAINER_ID
messages, losing information, and to limit bandwidth used by these
messages.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/uapi/linux/audit.h | 2 ++
kernel/audit.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
kernel/audit.h | 2 ++
3 files changed, 50 insertions(+)
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index ea6638bb914b..dcb076b0d2e1 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -343,6 +343,7 @@ enum {
#define AUDIT_STATUS_BACKLOG_LIMIT 0x0010
#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
#define AUDIT_STATUS_LOST 0x0040
+#define AUDIT_STATUS_CONTID_DEPTH_LIMIT 0x0080
#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
@@ -471,6 +472,7 @@ struct audit_status {
__u32 feature_bitmap; /* bitmap of kernel audit features */
};
__u32 backlog_wait_time;/* message queue wait timeout */
+ __u32 contid_depth_limit;/* container depth limit */
};
struct audit_features {
diff --git a/kernel/audit.c b/kernel/audit.c
index 68be59d1a89b..e5e39aedaf86 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -157,6 +157,7 @@ struct auditd_connection {
* of container objects to tasks and refcount changes. There should be
* no need for interaction with tasklist_lock */
static DEFINE_SPINLOCK(audit_contobj_list_lock);
+static u32 audit_contid_depth_limit = AUDIT_CONTID_DEPTH_LIMIT;
static struct kmem_cache *audit_buffer_cache;
@@ -678,6 +679,20 @@ static int audit_set_backlog_wait_time(u32 timeout)
&audit_backlog_wait_time, timeout);
}
+static int audit_set_contid_depth_limit(u32 limit)
+{
+ int rc = 0;
+
+ if (limit > 20 * AUDIT_CONTID_DEPTH_LIMIT) {
+ rc = -ENOSPC;
+ audit_log_config_change("audit_contid_depth_limit",
+ limit, audit_contid_depth_limit, 0);
+ return rc;
+ }
+ return audit_do_config_change("audit_contid_depth_limit",
+ &audit_contid_depth_limit, limit);
+}
+
static int audit_set_enabled(u32 state)
{
int rc;
@@ -1439,6 +1454,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
s.backlog = skb_queue_len(&audit_queue);
s.feature_bitmap = AUDIT_FEATURE_BITMAP_ALL;
s.backlog_wait_time = audit_backlog_wait_time;
+ s.contid_depth_limit = audit_contid_depth_limit;
audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
break;
}
@@ -1542,6 +1558,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
audit_log_config_change("lost", 0, lost, 1);
return lost;
}
+ if (s.mask & AUDIT_STATUS_CONTID_DEPTH_LIMIT) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ return -EINVAL;
+ err = audit_set_contid_depth_limit(s.contid_depth_limit);
+ if (err < 0)
+ return err;
+ }
break;
}
case AUDIT_GET_FEATURE:
@@ -2608,6 +2631,22 @@ int audit_signal_info(int sig, struct task_struct *t)
return audit_signal_info_syscall(t);
}
+static int audit_contid_depth(struct audit_contobj *cont)
+{
+ struct audit_contobj *parent;
+ int depth = 1;
+
+ if (!cont)
+ return 0;
+
+ parent = cont->parent;
+ while (parent) {
+ depth++;
+ parent = parent->parent;
+ }
+ return depth;
+}
+
static bool audit_contid_isowner(struct task_struct *tsk)
{
if (tsk->audit && tsk->audit->cont)
@@ -2701,6 +2740,13 @@ int audit_set_contid(struct task_struct *task, u64 contid)
}
break;
}
+ /* Clamp max container id depth */
+ if (audit_contid_depth_limit != 0 &&
+ audit_contid_depth(_audit_contobj(rcu_dereference(current->real_parent)))
+ >= audit_contid_depth_limit) {
+ rc = -EMLINK;
+ goto conterror;
+ }
if (!newcont) {
newcont = kmalloc(sizeof(*newcont), GFP_ATOMIC);
if (newcont) {
diff --git a/kernel/audit.h b/kernel/audit.h
index de814fcbb38c..fbca07a49c03 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -220,6 +220,8 @@ static inline int audit_hash_contid(u64 contid)
return (contid & (AUDIT_CONTID_BUCKETS-1));
}
+#define AUDIT_CONTID_DEPTH_LIMIT 4
+
/* Indicates that audit should log the full pathname. */
#define AUDIT_NAME_FULL -1
--
1.8.3.1
^ permalink raw reply related
* [PATCH ghak90 V8 15/16] audit: check contid count per netns and add config param limit
From: Richard Guy Briggs @ 2019-12-31 19:48 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: Paul Moore, sgrubb, omosnace, dhowells, simo, eparis, serge,
ebiederm, nhorman, dwalsh, mpatel, Richard Guy Briggs
In-Reply-To: <cover.1577736799.git.rgb@redhat.com>
Clamp the number of audit container identifiers associated with a
network namespace to limit the netlink and disk bandwidth used and to
prevent losing information from record text size overflow in the contid
field.
Add a configuration parameter AUDIT_STATUS_CONTID_NETNS_LIMIT (0x100)
to set the audit container identifier netns limit. This is used to
prevent overflow of the contid field in CONTAINER_OP and CONTAINER_ID
messages, losing information, and to limit bandwidth used by these
messages.
This value must be balanced with the audit container identifier nesting
depth limit to multiply out to no more than 400. This is determined by
the total audit message length less message overhead divided by the
length of the text representation of an audit container identifier.
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
include/linux/audit.h | 16 +++++++----
include/linux/nsproxy.h | 2 +-
include/uapi/linux/audit.h | 2 ++
kernel/audit.c | 68 ++++++++++++++++++++++++++++++++++++++--------
kernel/audit.h | 7 +++++
kernel/fork.c | 10 +++++--
kernel/nsproxy.c | 27 +++++++++++++++---
7 files changed, 107 insertions(+), 25 deletions(-)
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 4272b468417a..28b9c7cd86a6 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -234,9 +234,9 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
}
extern void audit_log_container_id(struct audit_context *context, u64 contid);
-extern void audit_netns_contid_add(struct net *net, u64 contid);
+extern int 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,
+extern int 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);
@@ -312,13 +312,17 @@ static inline u64 audit_get_contid(struct task_struct *tsk)
static inline void audit_log_container_id(struct audit_context *context, u64 contid)
{ }
-static inline void audit_netns_contid_add(struct net *net, u64 contid)
-{ }
+static inline int audit_netns_contid_add(struct net *net, u64 contid)
+{
+ return 0;
+}
static inline void audit_netns_contid_del(struct net *net, u64 contid)
{ }
-static inline void audit_switch_task_namespaces(struct nsproxy *ns,
+static inline int audit_switch_task_namespaces(struct nsproxy *ns,
struct task_struct *p)
-{ }
+{
+ return 0;
+}
static inline void audit_log_netns_contid_list(struct net *net,
struct audit_context *context)
{ }
diff --git a/include/linux/nsproxy.h b/include/linux/nsproxy.h
index 2ae1b1a4d84d..3ca35cbf2cd8 100644
--- a/include/linux/nsproxy.h
+++ b/include/linux/nsproxy.h
@@ -67,7 +67,7 @@ struct nsproxy {
int copy_namespaces(unsigned long flags, struct task_struct *tsk);
void exit_task_namespaces(struct task_struct *tsk);
-void switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
+int switch_task_namespaces(struct task_struct *tsk, struct nsproxy *new);
void free_nsproxy(struct nsproxy *ns);
int unshare_nsproxy_namespaces(unsigned long, struct nsproxy **,
struct cred *, struct fs_struct *);
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index dcb076b0d2e1..2844d78cd7af 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -344,6 +344,7 @@ enum {
#define AUDIT_STATUS_BACKLOG_WAIT_TIME 0x0020
#define AUDIT_STATUS_LOST 0x0040
#define AUDIT_STATUS_CONTID_DEPTH_LIMIT 0x0080
+#define AUDIT_STATUS_CONTID_NETNS_LIMIT 0x0100
#define AUDIT_FEATURE_BITMAP_BACKLOG_LIMIT 0x00000001
#define AUDIT_FEATURE_BITMAP_BACKLOG_WAIT_TIME 0x00000002
@@ -473,6 +474,7 @@ struct audit_status {
};
__u32 backlog_wait_time;/* message queue wait timeout */
__u32 contid_depth_limit;/* container depth limit */
+ __u32 contid_netns_limit;/* container netns limit */
};
struct audit_features {
diff --git a/kernel/audit.c b/kernel/audit.c
index e5e39aedaf86..1287f0b63757 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -89,11 +89,13 @@
* @sk: communication socket
* @contid_list: audit container identifier list
* @contid_list_lock audit container identifier list lock
+ * @contid_count count of audit container identifiers using this netns
*/
struct audit_net {
struct sock *sk;
struct list_head contid_list;
spinlock_t contid_list_lock;
+ int contid_count;
};
/**
@@ -158,6 +160,7 @@ struct auditd_connection {
* no need for interaction with tasklist_lock */
static DEFINE_SPINLOCK(audit_contobj_list_lock);
static u32 audit_contid_depth_limit = AUDIT_CONTID_DEPTH_LIMIT;
+static u32 audit_contid_netns_limit = AUDIT_CONTID_NETNS_LIMIT;
static struct kmem_cache *audit_buffer_cache;
@@ -419,19 +422,20 @@ static struct sock *audit_get_sk(const struct net *net)
return aunet->sk;
}
-void audit_netns_contid_add(struct net *net, u64 contid)
+int audit_netns_contid_add(struct net *net, u64 contid)
{
struct audit_net *aunet;
struct list_head *contid_list;
struct audit_contobj_netns *cont;
+ int rc = 0;
if (!net)
- return;
+ return 0;
if (!audit_contid_valid(contid))
- return;
+ return 0;
aunet = net_generic(net, audit_net_id);
if (!aunet)
- return;
+ return 0;
contid_list = &aunet->contid_list;
rcu_read_lock();
list_for_each_entry_rcu(cont, contid_list, list)
@@ -447,11 +451,22 @@ void audit_netns_contid_add(struct net *net, u64 contid)
cont->id = contid;
refcount_set(&cont->refcount, 1);
spin_lock(&aunet->contid_list_lock);
- list_add_rcu(&cont->list, contid_list);
+ if (audit_contid_netns_limit != 0 &&
+ aunet->contid_count < audit_contid_netns_limit) {
+ list_add_rcu(&cont->list, contid_list);
+ aunet->contid_count++;
+ } else {
+ rc = -ENOSR;
+ }
spin_unlock(&aunet->contid_list_lock);
+ if (rc)
+ kfree(cont);
+ } else {
+ rc = -ENOMEM;
}
out:
rcu_read_unlock();
+ return rc;
}
void audit_netns_contid_del(struct net *net, u64 contid)
@@ -475,6 +490,7 @@ void audit_netns_contid_del(struct net *net, u64 contid)
if (refcount_dec_and_test(&cont->refcount)) {
list_del_rcu(&cont->list);
kfree_rcu(cont, rcu);
+ aunet->contid_count--;
}
spin_unlock(&aunet->contid_list_lock);
break;
@@ -482,16 +498,21 @@ void audit_netns_contid_del(struct net *net, u64 contid)
rcu_read_unlock();
}
-void audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
+int audit_switch_task_namespaces(struct nsproxy *ns, struct task_struct *p)
{
u64 contid = audit_get_contid(p);
struct nsproxy *new = p->nsproxy;
+ int rc = 0;
if (!audit_contid_valid(contid))
- return;
+ return 0;
audit_netns_contid_del(ns->net_ns, contid);
- if (new)
- audit_netns_contid_add(new->net_ns, contid);
+ if (new) {
+ rc = audit_netns_contid_add(new->net_ns, contid);
+ if (rc)
+ audit_netns_contid_add(ns->net_ns, contid);
+ }
+ return rc;
}
void audit_log_contid(struct audit_buffer *ab, u64 contid);
@@ -683,7 +704,7 @@ static int audit_set_contid_depth_limit(u32 limit)
{
int rc = 0;
- if (limit > 20 * AUDIT_CONTID_DEPTH_LIMIT) {
+ if (limit * audit_contid_netns_limit > AUDIT_CONTID_MSG_LIMIT) {
rc = -ENOSPC;
audit_log_config_change("audit_contid_depth_limit",
limit, audit_contid_depth_limit, 0);
@@ -693,6 +714,20 @@ static int audit_set_contid_depth_limit(u32 limit)
&audit_contid_depth_limit, limit);
}
+static int audit_set_contid_netns_limit(u32 limit)
+{
+ int rc = 0;
+
+ if (limit * audit_contid_depth_limit > AUDIT_CONTID_MSG_LIMIT) {
+ rc = -ENOSPC;
+ audit_log_config_change("audit_contid_netns_limit",
+ limit, audit_contid_netns_limit, 0);
+ return rc;
+ }
+ return audit_do_config_change("audit_contid_netns_limit",
+ &audit_contid_netns_limit, limit);
+}
+
static int audit_set_enabled(u32 state)
{
int rc;
@@ -1455,6 +1490,7 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
s.feature_bitmap = AUDIT_FEATURE_BITMAP_ALL;
s.backlog_wait_time = audit_backlog_wait_time;
s.contid_depth_limit = audit_contid_depth_limit;
+ s.contid_netns_limit = audit_contid_netns_limit;
audit_send_reply(skb, seq, AUDIT_GET, 0, 0, &s, sizeof(s));
break;
}
@@ -1565,6 +1601,13 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
if (err < 0)
return err;
}
+ if (s.mask & AUDIT_STATUS_CONTID_NETNS_LIMIT) {
+ if (sizeof(s) > (size_t)nlh->nlmsg_len)
+ return -EINVAL;
+ err = audit_set_contid_netns_limit(s.contid_netns_limit);
+ if (err < 0)
+ return err;
+ }
break;
}
case AUDIT_GET_FEATURE:
@@ -1834,6 +1877,7 @@ static int __net_init audit_net_init(struct net *net)
aunet->sk->sk_sndtimeo = MAX_SCHEDULE_TIMEOUT;
INIT_LIST_HEAD(&aunet->contid_list);
spin_lock_init(&aunet->contid_list_lock);
+ aunet->contid_count = 0;
return 0;
}
@@ -2776,7 +2820,9 @@ int audit_set_contid(struct task_struct *task, u64 contid)
if (!rc) {
if (audit_contid_valid(oldcontid))
audit_netns_contid_del(net, oldcontid);
- audit_netns_contid_add(net, contid);
+ rc = audit_netns_contid_add(net, contid);
+ if (rc && audit_contid_valid(oldcontid))
+ audit_netns_contid_add(net, oldcontid);
}
task_unlock(task);
diff --git a/kernel/audit.h b/kernel/audit.h
index fbca07a49c03..5701a42e564f 100644
--- a/kernel/audit.h
+++ b/kernel/audit.h
@@ -222,6 +222,13 @@ static inline int audit_hash_contid(u64 contid)
#define AUDIT_CONTID_DEPTH_LIMIT 4
+#define AUDIT_CONTID_NETNS_LIMIT 100
+
+/* this value is determined by AUDIT_MESSAGE_TEXT_MAX (8560) minus
+ * overhead (128) all divided by the max text representation of a full
+ * u64 (21) */
+#define AUDIT_CONTID_MSG_LIMIT 400
+
/* Indicates that audit should log the full pathname. */
#define AUDIT_NAME_FULL -1
diff --git a/kernel/fork.c b/kernel/fork.c
index edf034e5cbb4..7431649d6a5a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2935,6 +2935,13 @@ int ksys_unshare(unsigned long unshare_flags)
new_cred, new_fs);
if (err)
goto bad_unshare_cleanup_cred;
+ if (new_nsproxy) {
+ err = switch_task_namespaces(current, new_nsproxy);
+ if (err) {
+ free_nsproxy(new_nsproxy);
+ goto bad_unshare_cleanup_cred;
+ }
+ }
if (new_fs || new_fd || do_sysvsem || new_cred || new_nsproxy) {
if (do_sysvsem) {
@@ -2949,9 +2956,6 @@ int ksys_unshare(unsigned long unshare_flags)
shm_init_task(current);
}
- if (new_nsproxy)
- switch_task_namespaces(current, new_nsproxy);
-
task_lock(current);
if (new_fs) {
diff --git a/kernel/nsproxy.c b/kernel/nsproxy.c
index bbdb5bbf5446..5181a41172a8 100644
--- a/kernel/nsproxy.c
+++ b/kernel/nsproxy.c
@@ -138,6 +138,7 @@ int copy_namespaces(unsigned long flags, struct task_struct *tsk)
struct user_namespace *user_ns = task_cred_xxx(tsk, user_ns);
struct nsproxy *new_ns;
u64 contid = audit_get_contid(tsk);
+ int rc;
if (likely(!(flags & (CLONE_NEWNS | CLONE_NEWUTS | CLONE_NEWIPC |
CLONE_NEWPID | CLONE_NEWNET |
@@ -165,7 +166,12 @@ 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);
+ rc = audit_netns_contid_add(new_ns->net_ns, contid);
+ if (rc) {
+ tsk->nsproxy = old_ns;
+ free_nsproxy(new_ns);
+ return rc;
+ }
return 0;
}
@@ -213,9 +219,10 @@ int unshare_nsproxy_namespaces(unsigned long unshare_flags,
return err;
}
-void switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
+int switch_task_namespaces(struct task_struct *p, struct nsproxy *new)
{
struct nsproxy *ns;
+ int rc;
might_sleep();
@@ -223,10 +230,17 @@ 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);
+ rc = audit_switch_task_namespaces(ns, p);
+ if (rc) {
+ task_lock(p);
+ p->nsproxy = ns;
+ task_unlock(p);
+ return rc;
+ }
if (ns && atomic_dec_and_test(&ns->count))
free_nsproxy(ns);
+ return 0;
}
void exit_task_namespaces(struct task_struct *p)
@@ -262,7 +276,12 @@ void exit_task_namespaces(struct task_struct *p)
free_nsproxy(new_nsproxy);
goto out;
}
- switch_task_namespaces(tsk, new_nsproxy);
+ err = switch_task_namespaces(tsk, new_nsproxy);
+ if (err) {
+ ns->ops->install(tsk->nsproxy, ns);
+ free_nsproxy(new_nsproxy);
+ goto out;
+ }
perf_event_namespaces(tsk);
out:
--
1.8.3.1
^ permalink raw reply related
* [PATCH ghak90 V8 16/16] audit: add capcontid to set contid outside init_user_ns
From: Richard Guy Briggs @ 2019-12-31 19:48 UTC (permalink / raw)
To: containers, linux-api, Linux-Audit Mailing List, linux-fsdevel,
LKML, netdev, netfilter-devel
Cc: Paul Moore, sgrubb, omosnace, dhowells, simo, eparis, serge,
ebiederm, nhorman, dwalsh, mpatel, Richard Guy Briggs
In-Reply-To: <cover.1577736799.git.rgb@redhat.com>
Provide a mechanism similar to CAP_AUDIT_CONTROL to explicitly give a
process in a non-init user namespace the capability to set audit
container identifiers.
Provide /proc/$PID/audit_capcontid interface to capcontid.
Valid values are: 1==enabled, 0==disabled
Report this action in message type AUDIT_SET_CAPCONTID 1022 with fields
opid= capcontid= old-capcontid=
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
fs/proc/base.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/audit.h | 14 ++++++++++++
include/uapi/linux/audit.h | 1 +
kernel/audit.c | 35 +++++++++++++++++++++++++++++
4 files changed, 105 insertions(+)
diff --git a/fs/proc/base.c b/fs/proc/base.c
index 26091800180c..283ef8e006e7 100644
--- a/fs/proc/base.c
+++ b/fs/proc/base.c
@@ -1360,6 +1360,59 @@ static ssize_t proc_contid_write(struct file *file, const char __user *buf,
.write = proc_contid_write,
.llseek = generic_file_llseek,
};
+
+static ssize_t proc_capcontid_read(struct file *file, char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct inode *inode = file_inode(file);
+ struct task_struct *task = get_proc_task(inode);
+ ssize_t length;
+ char tmpbuf[TMPBUFLEN];
+
+ if (!task)
+ return -ESRCH;
+ /* if we don't have caps, reject */
+ if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+ return -EPERM;
+ length = scnprintf(tmpbuf, TMPBUFLEN, "%u", audit_get_capcontid(task));
+ put_task_struct(task);
+ return simple_read_from_buffer(buf, count, ppos, tmpbuf, length);
+}
+
+static ssize_t proc_capcontid_write(struct file *file, const char __user *buf,
+ size_t count, loff_t *ppos)
+{
+ struct inode *inode = file_inode(file);
+ u32 capcontid;
+ int rv;
+ struct task_struct *task = get_proc_task(inode);
+
+ if (!task)
+ return -ESRCH;
+ if (*ppos != 0) {
+ /* No partial writes. */
+ put_task_struct(task);
+ return -EINVAL;
+ }
+
+ rv = kstrtou32_from_user(buf, count, 10, &capcontid);
+ if (rv < 0) {
+ put_task_struct(task);
+ return rv;
+ }
+
+ rv = audit_set_capcontid(task, capcontid);
+ put_task_struct(task);
+ if (rv < 0)
+ return rv;
+ return count;
+}
+
+static const struct file_operations proc_capcontid_operations = {
+ .read = proc_capcontid_read,
+ .write = proc_capcontid_write,
+ .llseek = generic_file_llseek,
+};
#endif
#ifdef CONFIG_FAULT_INJECTION
@@ -3121,6 +3174,7 @@ static int proc_stack_depth(struct seq_file *m, struct pid_namespace *ns,
REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid", S_IRUGO, proc_sessionid_operations),
REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+ REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
#endif
#ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
@@ -3522,6 +3576,7 @@ static int proc_tid_comm_permission(struct inode *inode, int mask)
REG("loginuid", S_IWUSR|S_IRUGO, proc_loginuid_operations),
REG("sessionid", S_IRUGO, proc_sessionid_operations),
REG("audit_containerid", S_IWUSR|S_IRUSR, proc_contid_operations),
+ REG("audit_capcontainerid", S_IWUSR|S_IRUSR|S_IRUSR, proc_capcontid_operations),
#endif
#ifdef CONFIG_FAULT_INJECTION
REG("make-it-fail", S_IRUGO|S_IWUSR, proc_fault_inject_operations),
diff --git a/include/linux/audit.h b/include/linux/audit.h
index 28b9c7cd86a6..62c453306c2a 100644
--- a/include/linux/audit.h
+++ b/include/linux/audit.h
@@ -116,6 +116,7 @@ struct audit_task_info {
kuid_t loginuid;
unsigned int sessionid;
struct audit_contobj *cont;
+ u32 capcontid;
#ifdef CONFIG_AUDITSYSCALL
struct audit_context *ctx;
#endif
@@ -224,6 +225,14 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
return tsk->audit->sessionid;
}
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+ if (!tsk->audit)
+ return 0;
+ return tsk->audit->capcontid;
+}
+
+extern int audit_set_capcontid(struct task_struct *tsk, u32 enable);
extern int audit_set_contid(struct task_struct *tsk, u64 contid);
static inline u64 audit_get_contid(struct task_struct *tsk)
@@ -305,6 +314,11 @@ static inline unsigned int audit_get_sessionid(struct task_struct *tsk)
return AUDIT_SID_UNSET;
}
+static inline u32 audit_get_capcontid(struct task_struct *tsk)
+{
+ return 0;
+}
+
static inline u64 audit_get_contid(struct task_struct *tsk)
{
return AUDIT_CID_UNSET;
diff --git a/include/uapi/linux/audit.h b/include/uapi/linux/audit.h
index 2844d78cd7af..01251e6dcec0 100644
--- a/include/uapi/linux/audit.h
+++ b/include/uapi/linux/audit.h
@@ -73,6 +73,7 @@
#define AUDIT_GET_FEATURE 1019 /* Get which features are enabled */
#define AUDIT_CONTAINER_OP 1020 /* Define the container id and info */
#define AUDIT_SIGNAL_INFO2 1021 /* Get info auditd signal sender */
+#define AUDIT_SET_CAPCONTID 1022 /* Set cap_contid of a task */
#define AUDIT_FIRST_USER_MSG 1100 /* Userspace messages mostly uninteresting to kernel */
#define AUDIT_USER_AVC 1107 /* We filter this differently */
diff --git a/kernel/audit.c b/kernel/audit.c
index 1287f0b63757..1c22dd084ae8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -2698,6 +2698,41 @@ static bool audit_contid_isowner(struct task_struct *tsk)
return false;
}
+int audit_set_capcontid(struct task_struct *task, u32 enable)
+{
+ u32 oldcapcontid;
+ int rc = 0;
+ struct audit_buffer *ab;
+
+ if (!task->audit)
+ return -ENOPROTOOPT;
+ oldcapcontid = audit_get_capcontid(task);
+ /* if task is not descendant, block */
+ if (task == current)
+ rc = -EBADSLT;
+ else if (!task_is_descendant(current, task))
+ rc = -EXDEV;
+ else if (current_user_ns() == &init_user_ns) {
+ if (!capable(CAP_AUDIT_CONTROL) && !audit_get_capcontid(current))
+ rc = -EPERM;
+ }
+ if (!rc)
+ task->audit->capcontid = enable;
+
+ if (!audit_enabled)
+ return rc;
+
+ ab = audit_log_start(audit_context(), GFP_KERNEL, AUDIT_SET_CAPCONTID);
+ if (!ab)
+ return rc;
+
+ audit_log_format(ab,
+ "opid=%d capcontid=%u old-capcontid=%u",
+ task_tgid_nr(task), enable, oldcapcontid);
+ audit_log_end(ab);
+ return rc;
+}
+
/*
* audit_set_contid - set current task's audit contid
* @task: target task
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-01 0:43 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20191230072959.62kcojxpthhdwmfa@yavin.dot.cyphar.com>
On Mon, Dec 30, 2019 at 06:29:59PM +1100, Aleksa Sarai wrote:
> On 2019-12-30, Aleksa Sarai <cyphar@cyphar.com> wrote:
> > On 2019-12-30, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > On Mon, Dec 30, 2019 at 04:20:35PM +1100, Aleksa Sarai wrote:
> > >
> > > > A reasonably detailed explanation of the issues is provided in the patch
> > > > itself, but the full traces produced by both the oopses and deadlocks is
> > > > included below (it makes little sense to include them in the commit since we
> > > > are disabling this feature, not directly fixing the bugs themselves).
> > > >
> > > > I've posted this as an RFC on whether this feature should be allowed at
> > > > all (and if anyone knows of legitimate uses for it), or if we should
> > > > work on fixing these other kernel bugs that it exposes.
> > >
> > > Umm... Are all of those traces
> > > a) reproducible on mainline and
> >
> > This was on viro/for-next, I'll retry it on v5.5-rc4.
>
> The NULL deref oops is reproducible on v5.5-rc4. Strangely it seems
> harder to reproduce than on viro/for-next (I kept reproducing it there
> by accident), but I'll double-check if that really is the case.
>
> The simplest reproducer is (using the attached programs and .config):
>
> ln -s . link
> sudo ./umount_symlink link
FWIW, the problem with that reproducer is that we *CAN'T* resolve that
path. Look: you have /proc/self/fd/3 resolve to ./link. OK, you've
asked to follow that. Got ./link, which is a symlink, so we need to
follow it further. Relative to what, though?
The meaning of symlink is dependent upon the directory you find it in.
And we don't have any here.
The bug is in mountpoint_last() - we have
if (unlikely(nd->last_type != LAST_NORM)) {
error = handle_dots(nd, nd->last_type);
if (error)
return error;
path.dentry = dget(nd->path.dentry);
} else {
path.dentry = d_lookup(dir, &nd->last);
if (!path.dentry) {
/*
* No cached dentry. Mounted dentries are pinned in the
* cache, so that means that this dentry is probably
* a symlink or the path doesn't actually point
* to a mounted dentry.
*/
path.dentry = lookup_slow(&nd->last, dir,
nd->flags | LOOKUP_NO_REVAL);
if (IS_ERR(path.dentry))
return PTR_ERR(path.dentry);
}
}
if (d_flags_negative(smp_load_acquire(&path.dentry->d_flags))) {
dput(path.dentry);
return -ENOENT;
}
path.mnt = nd->path.mnt;
return step_into(nd, &path, 0, d_backing_inode(path.dentry), 0);
in there, and that ends up with step_into() called in case of LAST_DOT/LAST_DOTDOT
(where it's harmless) *AND* in case of LAST_BIND. Where it very much isn't.
I'm not sure if you have caught anything else, but we really, really should *NOT*
consider the LAST_BIND as "maybe we should follow the result" material. So
at least the following is needed; could you check if anything else remains
with that applied?
diff --git a/fs/namei.c b/fs/namei.c
index d6c91d1e88cb..d4fbbda8a7ff 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2656,10 +2656,7 @@ mountpoint_last(struct nameidata *nd)
nd->flags &= ~LOOKUP_PARENT;
if (unlikely(nd->last_type != LAST_NORM)) {
- error = handle_dots(nd, nd->last_type);
- if (error)
- return error;
- path.dentry = dget(nd->path.dentry);
+ return handle_dots(nd, nd->last_type);
} else {
path.dentry = d_lookup(dir, &nd->last);
if (!path.dentry) {
^ permalink raw reply related
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-01 0:54 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101004324.GA11269@ZenIV.linux.org.uk>
On Wed, Jan 01, 2020 at 12:43:24AM +0000, Al Viro wrote:
> I'm not sure if you have caught anything else, but we really, really should *NOT*
> consider the LAST_BIND as "maybe we should follow the result" material. So
> at least the following is needed; could you check if anything else remains
> with that applied?
>
> diff --git a/fs/namei.c b/fs/namei.c
> index d6c91d1e88cb..d4fbbda8a7ff 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2656,10 +2656,7 @@ mountpoint_last(struct nameidata *nd)
> nd->flags &= ~LOOKUP_PARENT;
>
> if (unlikely(nd->last_type != LAST_NORM)) {
> - error = handle_dots(nd, nd->last_type);
> - if (error)
> - return error;
> - path.dentry = dget(nd->path.dentry);
> + return handle_dots(nd, nd->last_type);
> } else {
> path.dentry = d_lookup(dir, &nd->last);
> if (!path.dentry) {
Note, BTW, that lookup_last() (aka walk_component()) does just
that - we only hit step_into() on LAST_NORM. The same goes
for do_last(). mountpoint_last() not doing the same is _not_
intentional - it's definitely a bug.
Consider your testcase; link points to . here. So the only
thing you could expect from trying to follow it would be
the directory 'link' lives in. And you don't have it
when you reach the fscker via /proc/self/fd/3; what happens
instead is nd->path set to ./link (by nd_jump_link()) *AND*
step_into() called, pushing the same ./link onto stack.
It violates all kinds of assumptions made by fs/namei.c -
when pushing a symlink onto stack nd->path is expected to
contain the base directory for resolving it.
I'm fairly sure that this is the cause of at least some
of the insanity you've caught; there always could be
something else, of course, but this hole needs to be
closed in any case.
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-01 3:08 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101005446.GH4203@ZenIV.linux.org.uk>
On Wed, Jan 01, 2020 at 12:54:46AM +0000, Al Viro wrote:
> Note, BTW, that lookup_last() (aka walk_component()) does just
> that - we only hit step_into() on LAST_NORM. The same goes
> for do_last(). mountpoint_last() not doing the same is _not_
> intentional - it's definitely a bug.
>
> Consider your testcase; link points to . here. So the only
> thing you could expect from trying to follow it would be
> the directory 'link' lives in. And you don't have it
> when you reach the fscker via /proc/self/fd/3; what happens
> instead is nd->path set to ./link (by nd_jump_link()) *AND*
> step_into() called, pushing the same ./link onto stack.
> It violates all kinds of assumptions made by fs/namei.c -
> when pushing a symlink onto stack nd->path is expected to
> contain the base directory for resolving it.
>
> I'm fairly sure that this is the cause of at least some
> of the insanity you've caught; there always could be
> something else, of course, but this hole needs to be
> closed in any case.
... and with removal of now unused local variable, that's
mountpoint_last(): fix the treatment of LAST_BIND
step_into() should be attempted only in LAST_NORM
case, when we have the parent directory (in nd->path).
We get away with that for LAST_DOT and LOST_DOTDOT,
since those can't be symlinks, making step_init() and
equivalent of path_to_nameidata() - we do a bit of
useless work, but that's it. For LAST_BIND (i.e.
the case when we'd just followed a procfs-style
symlink) we really can't go there - result might
be a symlink and we really can't attempt following
it.
lookup_last() and do_last() do handle that properly;
mountpoint_last() should do the same.
Cc: stable@vger.kernel.org
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/namei.c b/fs/namei.c
index d6c91d1e88cb..13f9f973722b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2643,7 +2643,6 @@ EXPORT_SYMBOL(user_path_at_empty);
static int
mountpoint_last(struct nameidata *nd)
{
- int error = 0;
struct dentry *dir = nd->path.dentry;
struct path path;
@@ -2656,10 +2655,7 @@ mountpoint_last(struct nameidata *nd)
nd->flags &= ~LOOKUP_PARENT;
if (unlikely(nd->last_type != LAST_NORM)) {
- error = handle_dots(nd, nd->last_type);
- if (error)
- return error;
- path.dentry = dget(nd->path.dentry);
+ return handle_dots(nd, nd->last_type);
} else {
path.dentry = d_lookup(dir, &nd->last);
if (!path.dentry) {
^ permalink raw reply related
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2020-01-01 14:44 UTC (permalink / raw)
To: Al Viro
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101030815.GA17593@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 3198 bytes --]
On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Wed, Jan 01, 2020 at 12:54:46AM +0000, Al Viro wrote:
> > Note, BTW, that lookup_last() (aka walk_component()) does just
> > that - we only hit step_into() on LAST_NORM. The same goes
> > for do_last(). mountpoint_last() not doing the same is _not_
> > intentional - it's definitely a bug.
> >
> > Consider your testcase; link points to . here. So the only
> > thing you could expect from trying to follow it would be
> > the directory 'link' lives in. And you don't have it
> > when you reach the fscker via /proc/self/fd/3; what happens
> > instead is nd->path set to ./link (by nd_jump_link()) *AND*
> > step_into() called, pushing the same ./link onto stack.
> > It violates all kinds of assumptions made by fs/namei.c -
> > when pushing a symlink onto stack nd->path is expected to
> > contain the base directory for resolving it.
> >
> > I'm fairly sure that this is the cause of at least some
> > of the insanity you've caught; there always could be
> > something else, of course, but this hole needs to be
> > closed in any case.
>
> ... and with removal of now unused local variable, that's
>
> mountpoint_last(): fix the treatment of LAST_BIND
>
> step_into() should be attempted only in LAST_NORM
> case, when we have the parent directory (in nd->path).
> We get away with that for LAST_DOT and LOST_DOTDOT,
> since those can't be symlinks, making step_init() and
> equivalent of path_to_nameidata() - we do a bit of
> useless work, but that's it. For LAST_BIND (i.e.
> the case when we'd just followed a procfs-style
> symlink) we really can't go there - result might
> be a symlink and we really can't attempt following
> it.
>
> lookup_last() and do_last() do handle that properly;
> mountpoint_last() should do the same.
>
> Cc: stable@vger.kernel.org
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
Thanks, this fixes the issue for me (and also fixes another reproducer I
found -- mounting a symlink on top of itself then trying to umount it).
Reported-by: Aleksa Sarai <cyphar@cyphar.com>
Tested-by: Aleksa Sarai <cyphar@cyphar.com>
As for the original topic of bind-mounting symlinks -- given this is a
supported feature, would you be okay with me sending an updated
O_EMPTYPATH series?
> ---
> diff --git a/fs/namei.c b/fs/namei.c
> index d6c91d1e88cb..13f9f973722b 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -2643,7 +2643,6 @@ EXPORT_SYMBOL(user_path_at_empty);
> static int
> mountpoint_last(struct nameidata *nd)
> {
> - int error = 0;
> struct dentry *dir = nd->path.dentry;
> struct path path;
>
> @@ -2656,10 +2655,7 @@ mountpoint_last(struct nameidata *nd)
> nd->flags &= ~LOOKUP_PARENT;
>
> if (unlikely(nd->last_type != LAST_NORM)) {
> - error = handle_dots(nd, nd->last_type);
> - if (error)
> - return error;
> - path.dentry = dget(nd->path.dentry);
> + return handle_dots(nd, nd->last_type);
> } else {
> path.dentry = d_lookup(dir, &nd->last);
> if (!path.dentry) {
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-01 23:40 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101144407.ugjwzk7zxrucaa6a@yavin.dot.cyphar.com>
On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
> Thanks, this fixes the issue for me (and also fixes another reproducer I
> found -- mounting a symlink on top of itself then trying to umount it).
>
> Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> Tested-by: Aleksa Sarai <cyphar@cyphar.com>
Pushed into #fixes.
> As for the original topic of bind-mounting symlinks -- given this is a
> supported feature, would you be okay with me sending an updated
> O_EMPTYPATH series?
Post it on fsdevel; I'll need to reread it anyway to say anything useful...
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2020-01-02 3:59 UTC (permalink / raw)
To: Al Viro
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101234009.GB8904@ZenIV.linux.org.uk>
[-- Attachment #1: Type: text/plain, Size: 855 bytes --]
On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
>
> > Thanks, this fixes the issue for me (and also fixes another reproducer I
> > found -- mounting a symlink on top of itself then trying to umount it).
> >
> > Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> > Tested-by: Aleksa Sarai <cyphar@cyphar.com>
>
> Pushed into #fixes.
Thanks. One other thing I noticed is that umount applies to the
underlying symlink rather than the mountpoint on top. So, for example
(using the same scripts I posted in the thread):
# ln -s /tmp/foo link
# ./mount_to_symlink /etc/passwd link
# umount -l link # will attempt to unmount "/tmp/foo"
Is that intentional?
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* RE: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: David Laight @ 2020-01-02 8:58 UTC (permalink / raw)
To: 'Aleksa Sarai', Linus Torvalds
Cc: Al Viro, David Howells, Eric Biederman, stable, Christian Brauner,
Serge Hallyn, dev@opencontainers.org, Linux Containers, Linux API,
linux-fsdevel, Linux Kernel Mailing List
In-Reply-To: <20191230083224.sbk2jspqmup43obs@yavin.dot.cyphar.com>
From: Aleksa Sarai
> Sent: 30 December 2019 08:32
...
> I'm not sure I agree -- as I mentioned in my other mail, re-opening
> through /proc/self/fd/$n works *very* well and has for a long time (in
> fact, both LXC and runc depend on this working).
I thought it was marginally broken because it is followed as a symlink?
On, for example, NetBSD /proc/<n>/fd/<n> is a real reference to the
filesystem inode and can be used to link the file back into the filesystem
if all the directory entries have been removed.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Aleksa Sarai @ 2020-01-02 9:09 UTC (permalink / raw)
To: David Laight
Cc: Linus Torvalds, Al Viro, David Howells, Eric Biederman, stable,
Christian Brauner, Serge Hallyn, dev@opencontainers.org,
Linux Containers, Linux API, linux-fsdevel,
Linux Kernel Mailing List
In-Reply-To: <e1066da936244de99e7ee827695d6583@AcuMS.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 937 bytes --]
On 2020-01-02, David Laight <David.Laight@ACULAB.COM> wrote:
> From: Aleksa Sarai
> > Sent: 30 December 2019 08:32
> ...
> > I'm not sure I agree -- as I mentioned in my other mail, re-opening
> > through /proc/self/fd/$n works *very* well and has for a long time (in
> > fact, both LXC and runc depend on this working).
>
> I thought it was marginally broken because it is followed as a symlink?
> On, for example, NetBSD /proc/<n>/fd/<n> is a real reference to the
> filesystem inode and can be used to link the file back into the filesystem
> if all the directory entries have been removed.
That is also the case on Linux. It (strictly speaking) isn't a symlink
in the normal sense of the word, it's a magic-link (nd_jump_link
switches the nd->path to the actual 'struct file' in the case of
/proc/self/fd/$n).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes
From: Yang Shi @ 2020-01-02 22:15 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Michal Hocko, John Hubbard, Michael Kerrisk (man-pages), cl, cai,
akpm, linux-man, linux-api, linux-mm, linux-kernel
In-Reply-To: <87lfqtcfyo.fsf@x220.int.ebiederm.org>
On 12/30/19 7:49 PM, Eric W. Biederman wrote:
> Yang Shi <yang.shi@linux.alibaba.com> writes:
>
>> On 12/18/19 2:17 AM, Michal Hocko wrote:
>>> On Tue 17-12-19 23:36:09, John Hubbard wrote:
>>> [...]
>>>> diff --git a/man2/move_pages.2 b/man2/move_pages.2
>>>> index 2d96468fa..1bf1053f2 100644
>>>> --- a/man2/move_pages.2
>>>> +++ b/man2/move_pages.2
>>>> @@ -191,12 +191,6 @@ was specified or an attempt was made to migrate pages of a kernel thread.
>>>> .B ENODEV
>>>> One of the target nodes is not online.
>>>> .TP
>>>> -.B ENOENT
>>>> -No pages were found that require moving.
>>>> -All pages are either already
>>>> -on the target node, not present, had an invalid address or could not be
>>>> -moved because they were mapped by multiple processes.
>>>> -.TP
>>>> .B EPERM
>>>> The caller specified
>>>> .B MPOL_MF_MOVE_ALL
>>>>
>>>> ...But I'm not sure if we should change the implementation, instead, so
>>>> that it *can* return ENOENT. That's the main question to resolve before
>>>> creating any more patches, I think.
>>> I would start by dropping any note about ENOENT first. I am not really
>>> sure there is a reasonable usecase for it but maybe somebody comes up
>>> with something and only then we should consider it.
>>>
>>> Feel free to add
>>> Acked-by: Michal Hocko <mhocko@suse.com>
>>>
>>> ideally with a kernel commit which removed the ENOENT.
>> A quick audit doesn't show kernel code or comment notes about ENOENT
>> wrongly. The status could be set as ENOENT if the page is not present
>> (follow_page() returns NULL), and man page does match what kernel
>> does.
> Doesn't the function one layer up then consume the ENOENT?
No, it doesn't. The return value would be reset unconditionally by
store_status(). This is what the man page patch tries to correct.
>
> Eric
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Al Viro @ 2020-01-03 1:49 UTC (permalink / raw)
To: Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel, Ian Kent
In-Reply-To: <20200102035920.dsycgxnb6ba2jhz2@yavin.dot.cyphar.com>
On Thu, Jan 02, 2020 at 02:59:20PM +1100, Aleksa Sarai wrote:
> On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
> >
> > > Thanks, this fixes the issue for me (and also fixes another reproducer I
> > > found -- mounting a symlink on top of itself then trying to umount it).
> > >
> > > Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> > > Tested-by: Aleksa Sarai <cyphar@cyphar.com>
> >
> > Pushed into #fixes.
>
> Thanks. One other thing I noticed is that umount applies to the
> underlying symlink rather than the mountpoint on top. So, for example
> (using the same scripts I posted in the thread):
>
> # ln -s /tmp/foo link
> # ./mount_to_symlink /etc/passwd link
> # umount -l link # will attempt to unmount "/tmp/foo"
>
> Is that intentional?
It's a mess, again in mountpoint_last(). FWIW, at some point I proposed
to have nd_jump_link() to fail with -ELOOP if the target was a symlink;
Linus asked for reasons deeper than my dislike of the semantics, I looked
around and hadn't spotted anything. And there hadn't been at the time,
but when four months later umount_lookup_last() went in I failed to look
for that source of potential problems in it ;-/
I've looked at that area again now. Aside of usual cursing at do_last()
horrors (yes, its control flow is a horror; yes, it needs serious massage;
no, it's not a good idea to get sidetracked into that right now), there
are several fun questions:
* d_manage() and d_automount(). We almost certainly don't
want those for autofs on the final component of pathname in umount,
including the trailing symlinks. But do we want those on usual access
via /proc/*/fd/*? I.e. suppose somebody does open() (O_PATH or not)
in autofs; do we want ->d_manage()/->d_automount() called when
resolving /proc/self/fd/<whatever>/foo/bar? We do not; is that
correct from autofs point of view? I suspect that refusing to
do ->d_automount() is correct, but I don't understand ->d_manage()
purpose well enough to tell.
* I really hope that the weird "trailing / forces automount
even in cases when we normally wouldn't trigger it" (stat /mnt/foo
vs. stat /mnt/foo/) is not meant to extend to umount. I'd like
Ian's confirmation, though.
* do we want ->d_manage() on following .. into overmounted
directory? Again, autofs question...
The minimal fix to mountpoint_last() would be to have
follow_mount() done in LAST_NORM case. However, I'd like to understand
(and hopefully regularize) the rules for follow_mount()/follow_managed().
Additional scary question is nfsd iterplay with automount. For nfs4
exports it's potentially interesting...
Ian, could you comment on the autofs questions above?
I'd rather avoid doing changes in that area without your input -
it's subtle and breakage in automount-related behaviour can be
mysterious as hell.
^ permalink raw reply
* Re: [PATCH v6 07/10] proc: flush task dcache entries from all procfs instances
From: Alexey Gladkov @ 2020-01-03 8:56 UTC (permalink / raw)
To: J Freyensee
Cc: LKML, Kernel Hardening, Linux API, Linux FS Devel,
Linux Security Module, Akinobu Mita, Alexander Viro,
Alexey Dobriyan, Andrew Morton, Andy Lutomirski, Daniel Micay,
Djalal Harouni, Dmitry V . Levin, Eric W . Biederman,
Greg Kroah-Hartman, Ingo Molnar, J . Bruce Fields, Jeff Layton
In-Reply-To: <8d85ba43-0759-358e-137d-246107bac747@gmail.com>
On Mon, Dec 30, 2019 at 02:03:29PM -0800, J Freyensee wrote:
> > +#ifdef CONFIG_PROC_FS
> > +static inline void pidns_proc_lock(struct pid_namespace *pid_ns)
> > +{
> > + down_write(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_unlock(struct pid_namespace *pid_ns)
> > +{
> > + up_write(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_lock_shared(struct pid_namespace *pid_ns)
> > +{
> > + down_read(&pid_ns->rw_proc_mounts);
> > +}
> > +
> > +static inline void pidns_proc_unlock_shared(struct pid_namespace *pid_ns)
> > +{
> > + up_read(&pid_ns->rw_proc_mounts);
> > +}
> > +#else /* !CONFIG_PROC_FS */
> > +
> Apologies for my newbie question. I couldn't help but notice all these
> function calls are assuming that the parameter struct pid_namespace *pid_ns
> will never be NULL. Is that a good assumption?
These inline helpers are introduced to improve readability. They only make
sense inside procfs. I don't think that defensive programming is useful
here.
--
Rgrds, legion
^ permalink raw reply
* [PATCH v8 0/3] Add pidfd_getfd syscall
From: Sargun Dhillon @ 2020-01-03 16:29 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, jannh, cyphar, christian.brauner, oleg,
luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
This patchset introduces a mechanism (pidfd_getfd syscall) to get file
descriptors from other processes via pidfd. Although this can be achieved
using SCM_RIGHTS, and parasitic code injection, this offers a more
straightforward mechanism, with less overhead and complexity. The process
under manipulation's fd still remains valid, and unmodified by the
copy operation.
It introduces a flags field. The flags field is reserved a the moment,
but the intent is to extend it with the following capabilities:
* Close the remote FD when copying it
* Drop the cgroup data if it's a fd pointing a socket when copying it
The syscall numbers were chosen to be one greater than openat2.
Summary of history:
This initially started as a ptrace command. It did not require the process
to be stopped, and felt like kind of an awkward fit for ptrace. After that,
it moved to an ioctl on the pidfd. Given functionality, it made sense to
make it a syscall which did not require the process to be stopped.
Previous versions:
V7: https://lore.kernel.org/lkml/20191226180227.GA29389@ircssh-2.c.rugged-nimbus-611.internal/
V6: https://lore.kernel.org/lkml/20191223210823.GA25083@ircssh-2.c.rugged-nimbus-611.internal/
V5: https://lore.kernel.org/lkml/20191220232746.GA20215@ircssh-2.c.rugged-nimbus-611.internal/
V4: https://lore.kernel.org/lkml/20191218235310.GA17259@ircssh-2.c.rugged-nimbus-611.internal/
V3: https://lore.kernel.org/lkml/20191217005842.GA14379@ircssh-2.c.rugged-nimbus-611.internal/
V2: https://lore.kernel.org/lkml/20191209070446.GA32336@ircssh-2.c.rugged-nimbus-611.internal/
RFC V1: https://lore.kernel.org/lkml/20191205234450.GA26369@ircssh-2.c.rugged-nimbus-611.internal/
Changes since v7:
* No longer put security_file_recv at the end, and align with other
usages of putting it at the end of the file_recv.
* Rewrite self-tests in kselftest harness.
* Minor refactoring
Changes since v6:
* Proper attribution of get_task_file helper
* Move all types for syscall to int to represent fd
Changes since v5:
* Drop pidfd_getfd_options struct and replace with a flags field
Changes since v4:
* Turn into a syscall
* Move to PTRACE_MODE_ATTACH_REALCREDS from PTRACE_MODE_READ_REALCREDS
* Remove the sample code. This will come in another patchset, as the
new self-tests cover all the functionality.
Changes since v3:
* Add self-test
* Move to ioctl passing fd directly, versus args struct
* Shuffle around include files
Changes since v2:
* Move to ioctl on pidfd instead of ptrace function
* Add security check before moving file descriptor
Changes since the RFC v1:
* Introduce a new helper to fs/file.c to fetch a file descriptor from
any process. It largely uses the code suggested by Oleg, with a few
changes to fix locking
* It uses an extensible options struct to supply the FD, and option.
* I added a sample, using the code from the user-ptrace sample
Sargun Dhillon (3):
vfs, fdtable: Add get_task_file helper
pid: Introduce pidfd_getfd syscall
test: Add test for pidfd getfd
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
fs/file.c | 22 +-
include/linux/file.h | 2 +
include/linux/syscalls.h | 1 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/pid.c | 90 +++++++
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 4 +-
.../selftests/pidfd/pidfd_getfd_test.c | 227 ++++++++++++++++++
26 files changed, 365 insertions(+), 6 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
--
2.20.1
^ permalink raw reply
* [PATCH v8 1/3] vfs, fdtable: Add get_task_file helper
From: Sargun Dhillon @ 2020-01-03 16:29 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, jannh, cyphar, christian.brauner, oleg,
luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-1-sargun@sargun.me>
This introduces a function which can be used to fetch a file, given an
arbitrary task. As long as the user holds a reference (refcnt) to the
task_struct it is safe to call, and will either return NULL on failure,
or a pointer to the file, with a refcnt.
This patch is based on Oleg Nesterov's (cf. [1]) patch from September
2018.
[1]: Link: https://lore.kernel.org/r/20180915160423.GA31461@redhat.com
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Suggested-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
---
fs/file.c | 22 ++++++++++++++++++++--
include/linux/file.h | 2 ++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/fs/file.c b/fs/file.c
index 2f4fcf985079..2fc5eeef54a4 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -706,9 +706,9 @@ void do_close_on_exec(struct files_struct *files)
spin_unlock(&files->file_lock);
}
-static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
+static struct file *__fget_files(struct files_struct *files, unsigned int fd,
+ fmode_t mask, unsigned int refs)
{
- struct files_struct *files = current->files;
struct file *file;
rcu_read_lock();
@@ -729,6 +729,12 @@ static struct file *__fget(unsigned int fd, fmode_t mask, unsigned int refs)
return file;
}
+static inline struct file *__fget(unsigned int fd, fmode_t mask,
+ unsigned int refs)
+{
+ return __fget_files(current->files, fd, mask, refs);
+}
+
struct file *fget_many(unsigned int fd, unsigned int refs)
{
return __fget(fd, FMODE_PATH, refs);
@@ -746,6 +752,18 @@ struct file *fget_raw(unsigned int fd)
}
EXPORT_SYMBOL(fget_raw);
+struct file *fget_task(struct task_struct *task, unsigned int fd)
+{
+ struct file *file = NULL;
+
+ task_lock(task);
+ if (task->files)
+ file = __fget_files(task->files, fd, 0, 1);
+ task_unlock(task);
+
+ return file;
+}
+
/*
* Lightweight file lookup - no refcnt increment if fd table isn't shared.
*
diff --git a/include/linux/file.h b/include/linux/file.h
index 3fcddff56bc4..c6c7b24ea9f7 100644
--- a/include/linux/file.h
+++ b/include/linux/file.h
@@ -16,6 +16,7 @@ extern void fput(struct file *);
extern void fput_many(struct file *, unsigned int);
struct file_operations;
+struct task_struct;
struct vfsmount;
struct dentry;
struct inode;
@@ -47,6 +48,7 @@ static inline void fdput(struct fd fd)
extern struct file *fget(unsigned int fd);
extern struct file *fget_many(unsigned int fd, unsigned int refs);
extern struct file *fget_raw(unsigned int fd);
+extern struct file *fget_task(struct task_struct *task, unsigned int fd);
extern unsigned long __fdget(unsigned int fd);
extern unsigned long __fdget_raw(unsigned int fd);
extern unsigned long __fdget_pos(unsigned int fd);
--
2.20.1
^ permalink raw reply related
* [PATCH v8 2/3] pid: Introduce pidfd_getfd syscall
From: Sargun Dhillon @ 2020-01-03 16:29 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, jannh, cyphar, christian.brauner, oleg,
luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-1-sargun@sargun.me>
This syscall allows for the retrieval of file descriptors from other
processes, based on their pidfd. This is possible using ptrace, and
injection of parasitic code to inject code which leverages SCM_RIGHTS
to move file descriptors between a tracee and a tracer. Unfortunately,
ptrace comes with a high cost of requiring the process to be stopped,
and breaks debuggers. This does not require stopping the process under
manipulation.
One reason to use this is to allow sandboxers to take actions on file
descriptors on the behalf of another process. For example, this can be
combined with seccomp-bpf's user notification to do on-demand fd
extraction and take privileged actions. One such privileged action
is binding a socket to a privileged port.
This also adds the syscall to all architectures at the same time.
/* prototype */
/* flags is currently reserved and should be set to 0 */
int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
/* testing */
Ran self-test suite on x86_64
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
Cc: Christian Brauner <christian.brauner@ubuntu.com>
---
arch/alpha/kernel/syscalls/syscall.tbl | 1 +
arch/arm/tools/syscall.tbl | 1 +
arch/arm64/include/asm/unistd.h | 2 +-
arch/arm64/include/asm/unistd32.h | 2 +
arch/ia64/kernel/syscalls/syscall.tbl | 1 +
arch/m68k/kernel/syscalls/syscall.tbl | 1 +
arch/microblaze/kernel/syscalls/syscall.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n32.tbl | 1 +
arch/mips/kernel/syscalls/syscall_n64.tbl | 1 +
arch/mips/kernel/syscalls/syscall_o32.tbl | 1 +
arch/parisc/kernel/syscalls/syscall.tbl | 1 +
arch/powerpc/kernel/syscalls/syscall.tbl | 1 +
arch/s390/kernel/syscalls/syscall.tbl | 1 +
arch/sh/kernel/syscalls/syscall.tbl | 1 +
arch/sparc/kernel/syscalls/syscall.tbl | 1 +
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
arch/xtensa/kernel/syscalls/syscall.tbl | 1 +
include/linux/syscalls.h | 1 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/pid.c | 90 +++++++++++++++++++++
21 files changed, 113 insertions(+), 2 deletions(-)
diff --git a/arch/alpha/kernel/syscalls/syscall.tbl b/arch/alpha/kernel/syscalls/syscall.tbl
index 8e13b0b2928d..82301080f5e7 100644
--- a/arch/alpha/kernel/syscalls/syscall.tbl
+++ b/arch/alpha/kernel/syscalls/syscall.tbl
@@ -475,3 +475,4 @@
543 common fspick sys_fspick
544 common pidfd_open sys_pidfd_open
# 545 reserved for clone3
+548 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/arm/tools/syscall.tbl b/arch/arm/tools/syscall.tbl
index 6da7dc4d79cc..ba045e2f3a60 100644
--- a/arch/arm/tools/syscall.tbl
+++ b/arch/arm/tools/syscall.tbl
@@ -449,3 +449,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/arm64/include/asm/unistd.h b/arch/arm64/include/asm/unistd.h
index 2629a68b8724..b722e47377a5 100644
--- a/arch/arm64/include/asm/unistd.h
+++ b/arch/arm64/include/asm/unistd.h
@@ -38,7 +38,7 @@
#define __ARM_NR_compat_set_tls (__ARM_NR_COMPAT_BASE + 5)
#define __ARM_NR_COMPAT_END (__ARM_NR_COMPAT_BASE + 0x800)
-#define __NR_compat_syscalls 436
+#define __NR_compat_syscalls 439
#endif
#define __ARCH_WANT_SYS_CLONE
diff --git a/arch/arm64/include/asm/unistd32.h b/arch/arm64/include/asm/unistd32.h
index 94ab29cf4f00..a8da97a2de41 100644
--- a/arch/arm64/include/asm/unistd32.h
+++ b/arch/arm64/include/asm/unistd32.h
@@ -879,6 +879,8 @@ __SYSCALL(__NR_fspick, sys_fspick)
__SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
+#define __NR_pidfd_getfd 438
+__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
/*
* Please add new compat syscalls above this comment and update
diff --git a/arch/ia64/kernel/syscalls/syscall.tbl b/arch/ia64/kernel/syscalls/syscall.tbl
index 36d5faf4c86c..2b11adfc860c 100644
--- a/arch/ia64/kernel/syscalls/syscall.tbl
+++ b/arch/ia64/kernel/syscalls/syscall.tbl
@@ -356,3 +356,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/m68k/kernel/syscalls/syscall.tbl b/arch/m68k/kernel/syscalls/syscall.tbl
index a88a285a0e5f..44e879e98459 100644
--- a/arch/m68k/kernel/syscalls/syscall.tbl
+++ b/arch/m68k/kernel/syscalls/syscall.tbl
@@ -435,3 +435,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/microblaze/kernel/syscalls/syscall.tbl b/arch/microblaze/kernel/syscalls/syscall.tbl
index 09b0cd7dab0a..7afa00125cc4 100644
--- a/arch/microblaze/kernel/syscalls/syscall.tbl
+++ b/arch/microblaze/kernel/syscalls/syscall.tbl
@@ -441,3 +441,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n32.tbl b/arch/mips/kernel/syscalls/syscall_n32.tbl
index e7c5ab38e403..856d5ba34461 100644
--- a/arch/mips/kernel/syscalls/syscall_n32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n32.tbl
@@ -374,3 +374,4 @@
433 n32 fspick sys_fspick
434 n32 pidfd_open sys_pidfd_open
435 n32 clone3 __sys_clone3
+438 n32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_n64.tbl b/arch/mips/kernel/syscalls/syscall_n64.tbl
index 13cd66581f3b..2db6075352f3 100644
--- a/arch/mips/kernel/syscalls/syscall_n64.tbl
+++ b/arch/mips/kernel/syscalls/syscall_n64.tbl
@@ -350,3 +350,4 @@
433 n64 fspick sys_fspick
434 n64 pidfd_open sys_pidfd_open
435 n64 clone3 __sys_clone3
+438 n64 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/mips/kernel/syscalls/syscall_o32.tbl b/arch/mips/kernel/syscalls/syscall_o32.tbl
index 353539ea4140..e9f9d4a9b105 100644
--- a/arch/mips/kernel/syscalls/syscall_o32.tbl
+++ b/arch/mips/kernel/syscalls/syscall_o32.tbl
@@ -423,3 +423,4 @@
433 o32 fspick sys_fspick
434 o32 pidfd_open sys_pidfd_open
435 o32 clone3 __sys_clone3
+438 o32 pidfd_getfd sys_pidfd_getfd
diff --git a/arch/parisc/kernel/syscalls/syscall.tbl b/arch/parisc/kernel/syscalls/syscall.tbl
index 285ff516150c..c58c7eb144ca 100644
--- a/arch/parisc/kernel/syscalls/syscall.tbl
+++ b/arch/parisc/kernel/syscalls/syscall.tbl
@@ -433,3 +433,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3_wrapper
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/powerpc/kernel/syscalls/syscall.tbl b/arch/powerpc/kernel/syscalls/syscall.tbl
index 43f736ed47f2..707609bfe3ea 100644
--- a/arch/powerpc/kernel/syscalls/syscall.tbl
+++ b/arch/powerpc/kernel/syscalls/syscall.tbl
@@ -517,3 +517,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 nospu clone3 ppc_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/s390/kernel/syscalls/syscall.tbl b/arch/s390/kernel/syscalls/syscall.tbl
index 3054e9c035a3..185cd624face 100644
--- a/arch/s390/kernel/syscalls/syscall.tbl
+++ b/arch/s390/kernel/syscalls/syscall.tbl
@@ -438,3 +438,4 @@
433 common fspick sys_fspick sys_fspick
434 common pidfd_open sys_pidfd_open sys_pidfd_open
435 common clone3 sys_clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sh/kernel/syscalls/syscall.tbl b/arch/sh/kernel/syscalls/syscall.tbl
index b5ed26c4c005..88f90895aad8 100644
--- a/arch/sh/kernel/syscalls/syscall.tbl
+++ b/arch/sh/kernel/syscalls/syscall.tbl
@@ -438,3 +438,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/sparc/kernel/syscalls/syscall.tbl b/arch/sparc/kernel/syscalls/syscall.tbl
index 8c8cc7537fb2..218df6a2326e 100644
--- a/arch/sparc/kernel/syscalls/syscall.tbl
+++ b/arch/sparc/kernel/syscalls/syscall.tbl
@@ -481,3 +481,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
# 435 reserved for clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index 15908eb9b17e..9c3101b65e0f 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -440,3 +440,4 @@
433 i386 fspick sys_fspick __ia32_sys_fspick
434 i386 pidfd_open sys_pidfd_open __ia32_sys_pidfd_open
435 i386 clone3 sys_clone3 __ia32_sys_clone3
+438 i386 pidfd_getfd sys_pidfd_getfd __ia32_sys_pidfd_getfd
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index c29976eca4a8..cef85db75a62 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -357,6 +357,7 @@
433 common fspick __x64_sys_fspick
434 common pidfd_open __x64_sys_pidfd_open
435 common clone3 __x64_sys_clone3/ptregs
+438 common pidfd_getfd __x64_sys_pidfd_getfd
#
# x32-specific system call numbers start at 512 to avoid cache impact
diff --git a/arch/xtensa/kernel/syscalls/syscall.tbl b/arch/xtensa/kernel/syscalls/syscall.tbl
index 25f4de729a6d..ae15183def12 100644
--- a/arch/xtensa/kernel/syscalls/syscall.tbl
+++ b/arch/xtensa/kernel/syscalls/syscall.tbl
@@ -406,3 +406,4 @@
433 common fspick sys_fspick
434 common pidfd_open sys_pidfd_open
435 common clone3 sys_clone3
+438 common pidfd_getfd sys_pidfd_getfd
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 2960dedcfde8..5edbc31af51f 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -1000,6 +1000,7 @@ asmlinkage long sys_fspick(int dfd, const char __user *path, unsigned int flags)
asmlinkage long sys_pidfd_send_signal(int pidfd, int sig,
siginfo_t __user *info,
unsigned int flags);
+asmlinkage long sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
/*
* Architecture-specific system calls
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index 1fc8faa6e973..d36ec3d645bd 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -850,9 +850,11 @@ __SYSCALL(__NR_pidfd_open, sys_pidfd_open)
#define __NR_clone3 435
__SYSCALL(__NR_clone3, sys_clone3)
#endif
+#define __NR_pidfd_getfd 438
+__SYSCALL(__NR_pidfd_getfd, sys_pidfd_getfd)
#undef __NR_syscalls
-#define __NR_syscalls 436
+#define __NR_syscalls 439
/*
* 32 bit systems traditionally used different
diff --git a/kernel/pid.c b/kernel/pid.c
index 2278e249141d..0f4ecb57214c 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -578,3 +578,93 @@ void __init pid_idr_init(void)
init_pid_ns.pid_cachep = KMEM_CACHE(pid,
SLAB_HWCACHE_ALIGN | SLAB_PANIC | SLAB_ACCOUNT);
}
+
+static struct file *__pidfd_fget(struct task_struct *task, int fd)
+{
+ struct file *file;
+ int ret;
+
+ ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
+ if (ret)
+ return ERR_PTR(ret);
+
+ if (ptrace_may_access(task, PTRACE_MODE_ATTACH_REALCREDS))
+ file = fget_task(task, fd);
+ else
+ file = ERR_PTR(-EPERM);
+
+ mutex_unlock(&task->signal->cred_guard_mutex);
+
+ return file ?: ERR_PTR(-EBADF);
+}
+
+static int pidfd_getfd(struct pid *pid, int fd)
+{
+ struct task_struct *task;
+ struct file *file;
+ int ret;
+
+ task = get_pid_task(pid, PIDTYPE_PID);
+ if (!task)
+ return -ESRCH;
+
+ file = __pidfd_fget(task, fd);
+ put_task_struct(task);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ ret = security_file_receive(file);
+ if (ret) {
+ fput(file);
+ return ret;
+ }
+
+ ret = get_unused_fd_flags(O_CLOEXEC);
+ if (ret < 0)
+ fput(file);
+ else
+ fd_install(ret, file);
+
+ return ret;
+}
+
+/**
+ * sys_pidfd_getfd() - Get a file descriptor from another process
+ *
+ * @pidfd: the pidfd file descriptor of the process
+ * @fd: the file descriptor number to get
+ * @flags: flags on how to get the fd (reserved)
+ *
+ * This syscall gets a copy of a file descriptor from another process
+ * based on the pidfd, and file descriptor number. It requires that
+ * the calling process has the ability to ptrace the process represented
+ * by the pidfd. The process which is having its file descriptor copied
+ * is otherwise unaffected.
+ *
+ * Return: On success, a cloexec file descriptor is returned.
+ * On error, a negative errno number will be returned.
+ */
+SYSCALL_DEFINE3(pidfd_getfd, int, pidfd, int, fd,
+ unsigned int, flags)
+{
+ struct pid *pid;
+ struct fd f;
+ int ret;
+
+ /* flags is currently unused - make sure it's unset */
+ if (flags)
+ return -EINVAL;
+
+ f = fdget(pidfd);
+ if (!f.file)
+ return -EBADF;
+
+ pid = pidfd_pid(f.file);
+ if (IS_ERR(pid))
+ ret = PTR_ERR(pid);
+ else
+ ret = pidfd_getfd(pid, fd);
+
+ fdput(f);
+ return ret;
+}
--
2.20.1
^ permalink raw reply related
* [PATCH v8 3/3] test: Add test for pidfd getfd
From: Sargun Dhillon @ 2020-01-03 16:29 UTC (permalink / raw)
To: linux-kernel, containers, linux-api, linux-fsdevel
Cc: Sargun Dhillon, tycho, jannh, cyphar, christian.brauner, oleg,
luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-1-sargun@sargun.me>
The following tests:
* Fetch FD, and then compare via kcmp
* Make sure getfd can be blocked by blocking ptrace_may_access
* Making sure fetching bad FDs fails
* Make sure trying to set flags to non-zero results in an EINVAL
Signed-off-by: Sargun Dhillon <sargun@sargun.me>
---
tools/testing/selftests/pidfd/.gitignore | 1 +
tools/testing/selftests/pidfd/Makefile | 4 +-
.../selftests/pidfd/pidfd_getfd_test.c | 227 ++++++++++++++++++
3 files changed, 230 insertions(+), 2 deletions(-)
create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
index 8d069490e17b..3a779c084d96 100644
--- a/tools/testing/selftests/pidfd/.gitignore
+++ b/tools/testing/selftests/pidfd/.gitignore
@@ -2,3 +2,4 @@ pidfd_open_test
pidfd_poll_test
pidfd_test
pidfd_wait
+pidfd_getfd_test
diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
index 43db1b98e845..2071f7ab5dc9 100644
--- a/tools/testing/selftests/pidfd/Makefile
+++ b/tools/testing/selftests/pidfd/Makefile
@@ -1,7 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
-CFLAGS += -g -I../../../../usr/include/ -pthread
+CFLAGS += -g -I../../../../usr/include/ -pthread -Wall
-TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait
+TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait pidfd_getfd_test
include ../lib.mk
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
new file mode 100644
index 000000000000..26ca75597812
--- /dev/null
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -0,0 +1,227 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#define _GNU_SOURCE
+#include <errno.h>
+#include <fcntl.h>
+#include <limits.h>
+#include <linux/types.h>
+#include <sched.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <syscall.h>
+#include <sys/prctl.h>
+#include <sys/wait.h>
+#include <unistd.h>
+#include <sys/socket.h>
+#include <linux/kcmp.h>
+
+#include "pidfd.h"
+#include "../kselftest.h"
+#include "../kselftest_harness.h"
+
+/*
+ * UNKNOWN_FD is an fd number that should never exist in the child, as it is
+ * used to check the negative case.
+ */
+#define UNKNOWN_FD 111
+
+static int sys_kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1,
+ unsigned long idx2)
+{
+ return syscall(__NR_kcmp, pid1, pid2, type, idx1, idx2);
+}
+
+static int sys_pidfd_getfd(int pidfd, int fd, int flags)
+{
+ return syscall(__NR_pidfd_getfd, pidfd, fd, flags);
+}
+
+static int sys_memfd_create(const char *name, unsigned int flags)
+{
+ return syscall(__NR_memfd_create, name, flags);
+}
+
+static int __child(int sk, int memfd)
+{
+ int ret;
+ char buf;
+
+ /*
+ * Ensure we don't leave around a bunch of orphaned children if our
+ * tests fail.
+ */
+ ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
+ if (ret) {
+ fprintf(stderr, "%s: Child could not set DEATHSIG\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ ret = send(sk, &memfd, sizeof(memfd), 0);
+ if (ret != sizeof(memfd)) {
+ fprintf(stderr, "%s: Child failed to send fd number\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+
+ while ((ret = recv(sk, &buf, sizeof(buf), 0)) > 0) {
+ if (buf == 'P') {
+ ret = prctl(PR_SET_DUMPABLE, 0);
+ if (ret < 0) {
+ fprintf(stderr,
+ "%s: Child failed to disable ptrace\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+ } else {
+ fprintf(stderr, "Child received unknown command %c\n",
+ buf);
+ return EXIT_FAILURE;
+ }
+ ret = send(sk, &buf, sizeof(buf), 0);
+ if (ret != 1) {
+ fprintf(stderr, "%s: Child failed to ack\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+ }
+
+ if (ret < 0) {
+ fprintf(stderr, "%s: Child failed to read from socket\n",
+ strerror(errno));
+ }
+
+ return EXIT_SUCCESS;
+}
+
+static int child(int sk)
+{
+ int memfd, ret;
+
+ memfd = sys_memfd_create("test", 0);
+ if (memfd < 0) {
+ fprintf(stderr, "%s: Child could not create memfd\n",
+ strerror(errno));
+ ret = EXIT_FAILURE;
+ } else {
+ ret = __child(sk, memfd);
+ close(memfd);
+ }
+
+ close(sk);
+ return ret;
+}
+
+FIXTURE(child)
+{
+ pid_t pid;
+ int pidfd, sk, remote_fd;
+};
+
+FIXTURE_SETUP(child)
+{
+ int ret, sk_pair[2];
+
+ ASSERT_EQ(0, socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair))
+ {
+ TH_LOG("%s: failed to create socketpair", strerror(errno));
+ }
+ self->sk = sk_pair[0];
+
+ self->pid = fork();
+ ASSERT_GE(self->pid, 0);
+
+ if (self->pid == 0) {
+ close(sk_pair[0]);
+ exit(child(sk_pair[1]));
+ }
+
+ close(sk_pair[1]);
+
+ self->pidfd = sys_pidfd_open(self->pid, 0);
+ ASSERT_GE(self->pidfd, 0);
+
+ /*
+ * Wait for the child to complete setup. It'll send the remote memfd's
+ * number when ready.
+ */
+ ret = recv(sk_pair[0], &self->remote_fd, sizeof(self->remote_fd), 0);
+ ASSERT_EQ(sizeof(self->remote_fd), ret);
+}
+
+FIXTURE_TEARDOWN(child)
+{
+ int status;
+
+ EXPECT_EQ(0, close(self->pidfd));
+ EXPECT_EQ(0, close(self->sk));
+
+ EXPECT_EQ(waitpid(self->pid, &status, 0), self->pid);
+ EXPECT_EQ(true, WIFEXITED(status));
+ EXPECT_EQ(0, WEXITSTATUS(status));
+}
+
+TEST_F(child, disable_ptrace)
+{
+ int uid, fd;
+ char c;
+
+ /*
+ * Turn into nobody if we're root, to avoid CAP_SYS_PTRACE
+ *
+ * The tests should run in their own process, so even this test fails,
+ * it shouldn't result in subsequent tests failing.
+ */
+ uid = getuid();
+ if (uid == 0)
+ ASSERT_EQ(0, seteuid(USHRT_MAX));
+
+ ASSERT_EQ(1, send(self->sk, "P", 1, 0));
+ ASSERT_EQ(1, recv(self->sk, &c, 1, 0));
+
+ fd = sys_pidfd_getfd(self->pidfd, self->remote_fd, 0);
+ EXPECT_EQ(-1, fd);
+ EXPECT_EQ(EPERM, errno);
+
+ if (uid == 0)
+ ASSERT_EQ(0, seteuid(0));
+}
+
+TEST_F(child, fetch_fd)
+{
+ int fd, ret;
+
+ fd = sys_pidfd_getfd(self->pidfd, self->remote_fd, 0);
+ ASSERT_GE(fd, 0);
+
+ EXPECT_EQ(0, sys_kcmp(getpid(), self->pid, KCMP_FILE, fd, self->remote_fd));
+
+ ret = fcntl(fd, F_GETFD);
+ ASSERT_GE(ret, 0);
+ EXPECT_GE(ret & FD_CLOEXEC, 0);
+
+ close(fd);
+}
+
+TEST_F(child, test_unknown_fd)
+{
+ int fd;
+
+ fd = sys_pidfd_getfd(self->pidfd, UNKNOWN_FD, 0);
+ EXPECT_EQ(-1, fd) {
+ TH_LOG("getfd succeeded while fetching unknown fd");
+ };
+ EXPECT_EQ(EBADF, errno) {
+ TH_LOG("%s: getfd did not get EBADF", strerror(errno));
+ }
+}
+
+TEST(flags_set)
+{
+ ASSERT_EQ(-1, sys_pidfd_getfd(0, 0, 1));
+ EXPECT_EQ(errno, EINVAL);
+}
+
+TEST_HARNESS_MAIN
--
2.20.1
^ permalink raw reply related
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Ian Kent @ 2020-01-04 4:46 UTC (permalink / raw)
To: Al Viro, Aleksa Sarai
Cc: David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200103014901.GC8904@ZenIV.linux.org.uk>
It may be a bit off-topic here but, in autofs symlinks can be used
in place of mounts. That mechanism can be used (mostly nowadays) with
amd map format maps.
If I'm using symlinks instead of mounts (where I can) I definitely
don't want these to be over mounted by a mount.
I haven't seen problems like that happening but if it did happen
that would be a bug in automount or user mis-use of some sort.
On Fri, 2020-01-03 at 01:49 +0000, Al Viro wrote:
> On Thu, Jan 02, 2020 at 02:59:20PM +1100, Aleksa Sarai wrote:
> > On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
> > > On Thu, Jan 02, 2020 at 01:44:07AM +1100, Aleksa Sarai wrote:
> > >
> > > > Thanks, this fixes the issue for me (and also fixes another
> > > > reproducer I
> > > > found -- mounting a symlink on top of itself then trying to
> > > > umount it).
> > > >
> > > > Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> > > > Tested-by: Aleksa Sarai <cyphar@cyphar.com>
> > >
> > > Pushed into #fixes.
> >
> > Thanks. One other thing I noticed is that umount applies to the
> > underlying symlink rather than the mountpoint on top. So, for
> > example
> > (using the same scripts I posted in the thread):
> >
> > # ln -s /tmp/foo link
> > # ./mount_to_symlink /etc/passwd link
> > # umount -l link # will attempt to unmount "/tmp/foo"
> >
> > Is that intentional?
>
> It's a mess, again in mountpoint_last(). FWIW, at some point I
> proposed
> to have nd_jump_link() to fail with -ELOOP if the target was a
> symlink;
> Linus asked for reasons deeper than my dislike of the semantics, I
> looked
> around and hadn't spotted anything. And there hadn't been at the
> time,
> but when four months later umount_lookup_last() went in I failed to
> look
> for that source of potential problems in it ;-/
>
> I've looked at that area again now. Aside of usual cursing at
> do_last()
> horrors (yes, its control flow is a horror; yes, it needs serious
> massage;
> no, it's not a good idea to get sidetracked into that right now),
> there
> are several fun questions:
> * d_manage() and d_automount(). We almost certainly don't
> want those for autofs on the final component of pathname in umount,
> including the trailing symlinks. But do we want those on usual
> access
> via /proc/*/fd/*? I.e. suppose somebody does open() (O_PATH or not)
> in autofs; do we want ->d_manage()/->d_automount() called when
> resolving /proc/self/fd/<whatever>/foo/bar? We do not; is that
> correct from autofs point of view? I suspect that refusing to
> do ->d_automount() is correct, but I don't understand ->d_manage()
> purpose well enough to tell.
Yes, we don't want those on the final component of the path in
umount. The following of a symlink will give use a new path of
some sort so the rules would change to the usual ones for the
new path.
The semantics of following a symlink, be the source a proc entry
or not (I think) should always be the same. If the follow takes
us to an autofs file system (be it a trigger mount or an indirect
mount in an autofs file system) the behaviour should be that of
the autofs file system when we arrive there, from an auto-mount
POV.
The original intent of ->d_manage() was to prevent walks into an
under construction mount and that might not be as simple as mounting
a source on a mount point.
For example take the case of an automount indirect mount map entry
like this:
test /some/path/one server:/source/path1 \
/some/path/two server2:/source/path2 \
/some/other/path server:/source/path3 \
/some/other/path/three server:/source/path4
This entry has no mount at the root of the tree (so called root-less
multi-mount) but walks need to block when it's under construction as
the topology isn't known until the directory tree and any associated
mounts (usually trigger mounts) have been completed.
In this case it's needed to go to ref-walk mode and block until it's
done.
The other (perhaps not so obvious) use of ->d_manage() is to detect
expire to mount races. When an automount is expiring at the same time
a process (that would cause an automount) is traversing the path. The
base (I'll not say root, since the root of the expire might not be the
root of the tree) needs to block the walk until the expire is done.
These multi-mounts are meant to provide a "mount as you go" mechanism
so that only portions of the tree of mounts are mounted or expired at
any one time.
For example, the offsets in the above entry are /some/path/one,
/some/path/two, /some/other/path and /some/other/path/three.
On access to <autofs mount>/test automount is meant to mount trigger
mounts for offsets /some/path/one, /some/path/two and /some/other/path
and mount an offset trigger for /some/other/path/three into the mount
for /some/other/path when it's accessed and that might not happen
during the initial mount of the tree. The reverse being done on umount
in sub-trees of mounts when a nesting point like /some/other/path is
encountered.
But that's something of an aside because in all cases below the root
there will be an actual mount preventing walks into the tree under
nesting point mounts being constructed or expired.
Anyway, returning to the topic at hand, the answer to whether we want
->d_manage()/->d_automount() after a symlink has been followed is
yes, I think, because at that point we could be within a file system
that has automounts of some sort.
But perhaps I'm missing something about the description of the case
above ...
> * I really hope that the weird "trailing / forces automount
> even in cases when we normally wouldn't trigger it" (stat /mnt/foo
> vs. stat /mnt/foo/) is not meant to extend to umount. I'd like
> Ian's confirmation, though.
I can't see any way that the trailing "/" can realte to umount.
It has always been meant to be used to trigger a mount on something
that would otherwise not be mounted and that's the only case I'm
aware of.
> * do we want ->d_manage() on following .. into overmounted
> directory? Again, autofs question...
I think that amounts to asking "can the target of the ../ be in the
process of being constructed or expired at this time" and that's
probably yes. A root-less multi-mount would be one case where this
could happen (although it's not strictly an over-mounted directory).
>
> The minimal fix to mountpoint_last() would be to have
> follow_mount() done in LAST_NORM case. However, I'd like to
> understand
> (and hopefully regularize) the rules for
> follow_mount()/follow_managed().
> Additional scary question is nfsd iterplay with automount. For nfs4
> exports it's potentially interesting...
I'm not sure about nfs (and other cross mounting file systems). The
automounting in file systems other than autofs always have a real
mount as the target (AFAIK) so there's an implied blocking that occurs
on crossing the mount point. That's always made the nfs automounting
case simpler to my thinking anyway.
The real problem with nfs automount trees is when the topology of
the exports tree changes while parts of it are in use. People that
have any idea of how nfs cross mounting (and mount dependencies in
general) work shouldn't do that but they do it and then wonder why
things go wrong ...
>
> Ian, could you comment on the autofs questions above?
> I'd rather avoid doing changes in that area without your input -
> it's subtle and breakage in automount-related behaviour can be
> mysterious as hell.
Thanks for the heads up.
As always I can run tests on changes you want to do.
Fortunately that's generally worked out ok for us in the past.
Ian
^ permalink raw reply
* Re: [PATCH RFC 0/1] mount: universally disallow mounting over symlinks
From: Andy Lutomirski @ 2020-01-04 5:52 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, David Howells, Eric Biederman, Linus Torvalds, stable,
Christian Brauner, Serge Hallyn, dev, containers, linux-api,
linux-fsdevel, linux-kernel
In-Reply-To: <20200101144407.ugjwzk7zxrucaa6a@yavin.dot.cyphar.com>
> On Jan 1, 2020, at 11:44 PM, Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> On 2020-01-01, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>> On Wed, Jan 01, 2020 at 12:54:46AM +0000, Al Viro wrote:
>>> Note, BTW, that lookup_last() (aka walk_component()) does just
>>> that - we only hit step_into() on LAST_NORM. The same goes
>>> for do_last(). mountpoint_last() not doing the same is _not_
>>> intentional - it's definitely a bug.
>>>
>>> Consider your testcase; link points to . here. So the only
>>> thing you could expect from trying to follow it would be
>>> the directory 'link' lives in. And you don't have it
>>> when you reach the fscker via /proc/self/fd/3; what happens
>>> instead is nd->path set to ./link (by nd_jump_link()) *AND*
>>> step_into() called, pushing the same ./link onto stack.
>>> It violates all kinds of assumptions made by fs/namei.c -
>>> when pushing a symlink onto stack nd->path is expected to
>>> contain the base directory for resolving it.
>>>
>>> I'm fairly sure that this is the cause of at least some
>>> of the insanity you've caught; there always could be
>>> something else, of course, but this hole needs to be
>>> closed in any case.
>>
>> ... and with removal of now unused local variable, that's
>>
>> mountpoint_last(): fix the treatment of LAST_BIND
>>
>> step_into() should be attempted only in LAST_NORM
>> case, when we have the parent directory (in nd->path).
>> We get away with that for LAST_DOT and LOST_DOTDOT,
>> since those can't be symlinks, making step_init() and
>> equivalent of path_to_nameidata() - we do a bit of
>> useless work, but that's it. For LAST_BIND (i.e.
>> the case when we'd just followed a procfs-style
>> symlink) we really can't go there - result might
>> be a symlink and we really can't attempt following
>> it.
>>
>> lookup_last() and do_last() do handle that properly;
>> mountpoint_last() should do the same.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> Thanks, this fixes the issue for me (and also fixes another reproducer I
> found -- mounting a symlink on top of itself then trying to umount it).
>
> Reported-by: Aleksa Sarai <cyphar@cyphar.com>
> Tested-by: Aleksa Sarai <cyphar@cyphar.com>
>
> As for the original topic of bind-mounting symlinks -- given this is a
> supported feature, would you be okay with me sending an updated
> O_EMPTYPATH series?
FWIW, I have an actual use case for mounting over a symlink: replacing /etc/resolv.conf. My virtme tool is presented with somewhat arbitrary crud in /etc, where /etc/resolv.conf might be a plain file or a symlink, but, regardless, has inappropriate contents. If it’s a file, I can mount a new file over it. If it’s a symlink and the kernel properly supported it, I could also mount over it.
Yes, I could also use overlayfs. Maybe I should regardless.
^ permalink raw reply
* Re: [PATCH v8 1/3] vfs, fdtable: Add get_task_file helper
From: Christian Brauner @ 2020-01-05 12:47 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-2-sargun@sargun.me>
On Fri, Jan 03, 2020 at 08:29:26AM -0800, Sargun Dhillon wrote:
> This introduces a function which can be used to fetch a file, given an
> arbitrary task. As long as the user holds a reference (refcnt) to the
> task_struct it is safe to call, and will either return NULL on failure,
> or a pointer to the file, with a refcnt.
>
> This patch is based on Oleg Nesterov's (cf. [1]) patch from September
> 2018.
>
> [1]: Link: https://lore.kernel.org/r/20180915160423.GA31461@redhat.com
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Suggested-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Nit: the patch is titled "vfs, fdtable: Add get_task_file helper"
but the actual helper is called fget_task()
^ permalink raw reply
* Re: [PATCH v8 2/3] pid: Introduce pidfd_getfd syscall
From: Christian Brauner @ 2020-01-05 13:30 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-3-sargun@sargun.me>
On Fri, Jan 03, 2020 at 08:29:27AM -0800, Sargun Dhillon wrote:
> This syscall allows for the retrieval of file descriptors from other
> processes, based on their pidfd. This is possible using ptrace, and
> injection of parasitic code to inject code which leverages SCM_RIGHTS
> to move file descriptors between a tracee and a tracer. Unfortunately,
> ptrace comes with a high cost of requiring the process to be stopped,
> and breaks debuggers. This does not require stopping the process under
> manipulation.
>
> One reason to use this is to allow sandboxers to take actions on file
> descriptors on the behalf of another process. For example, this can be
> combined with seccomp-bpf's user notification to do on-demand fd
> extraction and take privileged actions. One such privileged action
> is binding a socket to a privileged port.
>
> This also adds the syscall to all architectures at the same time.
>
> /* prototype */
> /* flags is currently reserved and should be set to 0 */
> int sys_pidfd_getfd(int pidfd, int fd, unsigned int flags);
>
> /* testing */
> Ran self-test suite on x86_64
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> Cc: Christian Brauner <christian.brauner@ubuntu.com>
The prefered way of adding a syscall is to keep the implementation
separate from the wiring up into the syscall tables. So please split the
patch into two:
- [2/4] pidfd_getfd() implementation
- [3/4] pidfd_getfd() wiring up
otherwise
Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
^ permalink raw reply
* Re: [PATCH v8 3/3] test: Add test for pidfd getfd
From: Christian Brauner @ 2020-01-05 14:20 UTC (permalink / raw)
To: Sargun Dhillon
Cc: linux-kernel, containers, linux-api, linux-fsdevel, tycho, jannh,
cyphar, oleg, luto, viro, gpascutto, ealvarez, fweimer, jld, arnd
In-Reply-To: <20200103162928.5271-4-sargun@sargun.me>
On Fri, Jan 03, 2020 at 08:29:28AM -0800, Sargun Dhillon wrote:
> The following tests:
> * Fetch FD, and then compare via kcmp
> * Make sure getfd can be blocked by blocking ptrace_may_access
> * Making sure fetching bad FDs fails
> * Make sure trying to set flags to non-zero results in an EINVAL
>
> Signed-off-by: Sargun Dhillon <sargun@sargun.me>
> ---
> tools/testing/selftests/pidfd/.gitignore | 1 +
> tools/testing/selftests/pidfd/Makefile | 4 +-
> .../selftests/pidfd/pidfd_getfd_test.c | 227 ++++++++++++++++++
> 3 files changed, 230 insertions(+), 2 deletions(-)
> create mode 100644 tools/testing/selftests/pidfd/pidfd_getfd_test.c
>
> diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore
> index 8d069490e17b..3a779c084d96 100644
> --- a/tools/testing/selftests/pidfd/.gitignore
> +++ b/tools/testing/selftests/pidfd/.gitignore
> @@ -2,3 +2,4 @@ pidfd_open_test
> pidfd_poll_test
> pidfd_test
> pidfd_wait
> +pidfd_getfd_test
> diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile
> index 43db1b98e845..2071f7ab5dc9 100644
> --- a/tools/testing/selftests/pidfd/Makefile
> +++ b/tools/testing/selftests/pidfd/Makefile
> @@ -1,7 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> -CFLAGS += -g -I../../../../usr/include/ -pthread
> +CFLAGS += -g -I../../../../usr/include/ -pthread -Wall
>
> -TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait
> +TEST_GEN_PROGS := pidfd_test pidfd_fdinfo_test pidfd_open_test pidfd_poll_test pidfd_wait pidfd_getfd_test
>
> include ../lib.mk
>
> diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> new file mode 100644
> index 000000000000..26ca75597812
> --- /dev/null
> +++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
> @@ -0,0 +1,227 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#define _GNU_SOURCE
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <linux/types.h>
> +#include <sched.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <syscall.h>
> +#include <sys/prctl.h>
> +#include <sys/wait.h>
> +#include <unistd.h>
> +#include <sys/socket.h>
> +#include <linux/kcmp.h>
> +
> +#include "pidfd.h"
> +#include "../kselftest.h"
> +#include "../kselftest_harness.h"
> +
> +/*
> + * UNKNOWN_FD is an fd number that should never exist in the child, as it is
> + * used to check the negative case.
> + */
> +#define UNKNOWN_FD 111
> +
> +static int sys_kcmp(pid_t pid1, pid_t pid2, int type, unsigned long idx1,
> + unsigned long idx2)
> +{
> + return syscall(__NR_kcmp, pid1, pid2, type, idx1, idx2);
> +}
> +
> +static int sys_pidfd_getfd(int pidfd, int fd, int flags)
> +{
> + return syscall(__NR_pidfd_getfd, pidfd, fd, flags);
> +}
I think you can move this to the pidfd.h header as:
static inline int sys_pidfd_getfd(int pidfd, int fd, int flags)
{
return syscall(__NR_pidfd_getfd, pidfd, fd, flags);
}
Note, this also needs an
#ifndef __NR_pidfd_getfd
__NR_pidfd_getfd -1
#endif
so that compilation doesn't fail.
> +
> +static int sys_memfd_create(const char *name, unsigned int flags)
> +{
> + return syscall(__NR_memfd_create, name, flags);
> +}
> +
> +static int __child(int sk, int memfd)
> +{
> + int ret;
> + char buf;
> +
> + /*
> + * Ensure we don't leave around a bunch of orphaned children if our
> + * tests fail.
> + */
> + ret = prctl(PR_SET_PDEATHSIG, SIGKILL);
> + if (ret) {
> + fprintf(stderr, "%s: Child could not set DEATHSIG\n",
> + strerror(errno));
> + return EXIT_FAILURE;
return -1
> + }
> +
> + ret = send(sk, &memfd, sizeof(memfd), 0);
> + if (ret != sizeof(memfd)) {
> + fprintf(stderr, "%s: Child failed to send fd number\n",
> + strerror(errno));
> + return EXIT_FAILURE;
return -1
> + }
> +
> + while ((ret = recv(sk, &buf, sizeof(buf), 0)) > 0) {
> + if (buf == 'P') {
> + ret = prctl(PR_SET_DUMPABLE, 0);
> + if (ret < 0) {
> + fprintf(stderr,
> + "%s: Child failed to disable ptrace\n",
> + strerror(errno));
> + return EXIT_FAILURE;
return -1
> + }
> + } else {
> + fprintf(stderr, "Child received unknown command %c\n",
> + buf);
> + return EXIT_FAILURE;
return -1
> + }
> + ret = send(sk, &buf, sizeof(buf), 0);
> + if (ret != 1) {
> + fprintf(stderr, "%s: Child failed to ack\n",
> + strerror(errno));
> + return EXIT_FAILURE;
return -1
> + }
> + }
> +
> + if (ret < 0) {
> + fprintf(stderr, "%s: Child failed to read from socket\n",
> + strerror(errno));
Is this intentional that this is no failure?
> + }
> +
> + return EXIT_SUCCESS;
return 0
> +}
> +
> +static int child(int sk)
> +{
> + int memfd, ret;
> +
> + memfd = sys_memfd_create("test", 0);
> + if (memfd < 0) {
> + fprintf(stderr, "%s: Child could not create memfd\n",
> + strerror(errno));
> + ret = EXIT_FAILURE;
ret = -1;
> + } else {
> + ret = __child(sk, memfd);
> + close(memfd);
> + }
> +
> + close(sk);
> + return ret;
> +}
> +
> +FIXTURE(child)
> +{
> + pid_t pid;
> + int pidfd, sk, remote_fd;
> +};
> +
> +FIXTURE_SETUP(child)
> +{
> + int ret, sk_pair[2];
> +
> + ASSERT_EQ(0, socketpair(PF_LOCAL, SOCK_SEQPACKET, 0, sk_pair))
> + {
> + TH_LOG("%s: failed to create socketpair", strerror(errno));
> + }
> + self->sk = sk_pair[0];
> +
> + self->pid = fork();
> + ASSERT_GE(self->pid, 0);
> +
> + if (self->pid == 0) {
> + close(sk_pair[0]);
> + exit(child(sk_pair[1]));
if (child(sk_pair[1]))
_exit(EXIT_FAILURE);
_exit(EXIT_SUCCESS);
I would like to only use exit macros where one actually calls
{_}exit()s. It makes the logic easier to follow and ensures that one
doesn't accidently do an exit(-21345) or something (e.g. when adding new
code).
> + }
> +
> + close(sk_pair[1]);
> +
> + self->pidfd = sys_pidfd_open(self->pid, 0);
> + ASSERT_GE(self->pidfd, 0);
> +
> + /*
> + * Wait for the child to complete setup. It'll send the remote memfd's
> + * number when ready.
> + */
> + ret = recv(sk_pair[0], &self->remote_fd, sizeof(self->remote_fd), 0);
> + ASSERT_EQ(sizeof(self->remote_fd), ret);
> +}
> +
> +FIXTURE_TEARDOWN(child)
> +{
> + int status;
> +
> + EXPECT_EQ(0, close(self->pidfd));
> + EXPECT_EQ(0, close(self->sk));
> +
> + EXPECT_EQ(waitpid(self->pid, &status, 0), self->pid);
> + EXPECT_EQ(true, WIFEXITED(status));
> + EXPECT_EQ(0, WEXITSTATUS(status));
> +}
> +
> +TEST_F(child, disable_ptrace)
> +{
> + int uid, fd;
> + char c;
> +
> + /*
> + * Turn into nobody if we're root, to avoid CAP_SYS_PTRACE
> + *
> + * The tests should run in their own process, so even this test fails,
> + * it shouldn't result in subsequent tests failing.
> + */
> + uid = getuid();
> + if (uid == 0)
> + ASSERT_EQ(0, seteuid(USHRT_MAX));
Hm, isn't it safer to do 65535 explicitly? Since USHRT_MAX can
technically be greater than 65535.
> +
> + ASSERT_EQ(1, send(self->sk, "P", 1, 0));
> + ASSERT_EQ(1, recv(self->sk, &c, 1, 0));
> +
> + fd = sys_pidfd_getfd(self->pidfd, self->remote_fd, 0);
> + EXPECT_EQ(-1, fd);
> + EXPECT_EQ(EPERM, errno);
> +
> + if (uid == 0)
> + ASSERT_EQ(0, seteuid(0));
> +}
> +
> +TEST_F(child, fetch_fd)
> +{
> + int fd, ret;
> +
> + fd = sys_pidfd_getfd(self->pidfd, self->remote_fd, 0);
> + ASSERT_GE(fd, 0);
> +
> + EXPECT_EQ(0, sys_kcmp(getpid(), self->pid, KCMP_FILE, fd, self->remote_fd));
So most of these tests seem to take place when the child has already
called exit() - or at least it's very likely that the child has already
called exit() - and remains a zombie. That's not ideal because
that's not the common scenario/use-case. Usually the task of which we
want to get an fd will be alive. Also, if the child has already called
exit(), by the time it returns to userspace it should have already
called exit_files() and so I wonder whether this test would fail if it's
run after the child has exited. Maybe I'm missing something here... Is
there some ordering enforced by TEST_F()?
Also, what does self->pid point to? The fd of the already exited child?
^ 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