Skip to contentSkip to conversation

Patch 1

PATCH In review Andi Kleen 2 participants 3 messages Apr 24, 2018 – Apr 25, 2018

Subsystems (6)
Networking BPF BPF core BPF networking +2 more subsystems
lore
Share & tools
lore

Andi Kleen

author
#0 lore raw

PATCH 1/1 Patch 1

In the future seccomp will have other side effects, such as disabling some CPU features. If an application does not use seccomp today, but wants to disable these CPU features, requiring PR_SET_NO_NEW_PRIVS could be a problem because it would prevent interacting with suid programs.

PR_SET_NO_NEW_PRIVS is only needed if seccomp can actually deny syscalls. Allow seccomp to set simple BPF programs consisting only of a "BPF_RET accept" even without the no_new_privs flag.

This allows any program to use such a simple seccomp rule without breaking suid use.

Signed-off-by Andi Kleen <[email protected]>
---
 include/linux/filter.h |  3 ++-
 kernel/seccomp.c       |  9 +++++++--
 net/core/filter.c      | 11 ++++++++++-
 net/packet/af_packet.c |  2 +-
 4 files changed, 20 insertions(+), 5 deletions(-)
include/linux/filter.h +2 -1
index 276932d75975..9cde89cb2841 100644
@@ -706,7 +706,8 @@ typedef int (*bpf_aux_classic_check_t)(struct sock_filter *filter,
 
 int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog);
 int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
-			      bpf_aux_classic_check_t trans, bool save_orig);
+			      bpf_aux_classic_check_t trans, bool save_orig,
+			      bool only_accept);
 void bpf_prog_destroy(struct bpf_prog *fp);
 
 int sk_attach_filter(struct sock_fprog *fprog, struct sock *sk);
kernel/seccomp.c +7 -2
index dc77548167ef..067316ee8b90 100644
@@ -361,6 +361,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 	struct seccomp_filter *sfilter;
 	int ret;
 	const bool save_orig = IS_ENABLED(CONFIG_CHECKPOINT_RESTORE);
+	bool only_accept = false;
 
 	if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
 		return ERR_PTR(-EINVAL);

@@ -372,11 +373,14 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 	 * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
 	 * This avoids scenarios where unprivileged tasks can affect the
 	 * behavior of privileged children.
+	 *
+	 * We make an exception for seccomp programs that only consist
+	 * of a single accept return.
 	 */
 	if (!task_no_new_privs(current) &&
 	    security_capable_noaudit(current_cred(), current_user_ns(),
 				     CAP_SYS_ADMIN) != 0)
-		return ERR_PTR(-EACCES);
+		only_accept = true;
 
 	/* Allocate a new seccomp_filter */
 	sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL | __GFP_NOWARN);

@@ -384,7 +388,8 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 		return ERR_PTR(-ENOMEM);
 
 	ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
-					seccomp_check_filter, save_orig);
+					seccomp_check_filter, save_orig,
+					only_accept);
 	if (ret < 0) {
 		kfree(sfilter);
 		return ERR_PTR(ret);
net/core/filter.c +10 -1
index 48aa7c7320db..e61b36db36c9 100644
@@ -1174,7 +1174,8 @@ EXPORT_SYMBOL_GPL(bpf_prog_create);
  * It also allows for passing a bpf_aux_classic_check_t handler.
  */
 int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
-			      bpf_aux_classic_check_t trans, bool save_orig)
+			      bpf_aux_classic_check_t trans, bool save_orig,
+			      bool only_accept)
 {
 	unsigned int fsize = bpf_classic_proglen(fprog);
 	struct bpf_prog *fp;

@@ -1193,6 +1194,14 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 		return -EFAULT;
 	}
 
+	/* Allow only a single BPF_STMT(BPF_RET | BPF_K, SECCOMP_RET_ALLOW) */
+	if (only_accept && (fprog->len != 1 ||
+		fprog->filter[0].code != (BPF_RET | BPF_K) ||
+		fprog->filter[0].k != SECCOMP_RET_ALLOW)) {
+		__bpf_prog_free(fp);
+		return -EACCES;
+	}
+
 	fp->len = fprog->len;
 	fp->orig_prog = NULL;
net/packet/af_packet.c +1 -1
index e0f3f4aeeb4f..b53a778010a4 100644
@@ -1567,7 +1567,7 @@ static int fanout_set_data_cbpf(struct packet_sock *po, char __user *data,
 	if (copy_from_user(&fprog, data, len))
 		return -EFAULT;
 
-	ret = bpf_prog_create_from_user(&new, &fprog, NULL, false);
+	ret = bpf_prog_create_from_user(&new, &fprog, NULL, false, false);
 	if (ret)
 		return ret;
Signature · 1 line
2.15.0
#1 lore raw
Quoted 6 lines · speck for Andi Kleen

In the future seccomp will have other side effects, such as disabling some CPU features. If an application does not use seccomp today, but wants to disable these CPU features, requiring PR_SET_NO_NEW_PRIVS could be a problem because it would prevent interacting with suid programs.

Quoted 6 lines

PR_SET_NO_NEW_PRIVS is only needed if seccomp can actually deny syscalls. Allow seccomp to set simple BPF programs consisting only of a "BPF_RET accept" even without the no_new_privs flag.

This allows any program to use such a simple seccomp rule without breaking suid use.

Extending seccomp (Tim's patch) is fair enough, since seccomp aware stuff is explicitly intended to be used for sandboxing, etc.

...but don't you think we also could need a new prctl to handle CPU features, or even just speculation features? I worry that we'll have to retro-fit seccomp to things like OpenJDK that were not using it before, in the next *week*, while a simple prctl is easily added as a one liner. Sure, you're going to say "but it's just an empty BPF_RET", but that's still more for folks who never touched it before to have to understand vs. just telling them to add a simple prctl they probably already know.

To me, it seems the idea of limiting speculation for seccomp (reasonable) was overloaded into "let's do it all there".

Jon.

Signature · 1 line
Computer Architect | Sent from my Fedora powered laptop
#2 lore raw
Quoted 25 lines · speck for Jon Masters
Quoted 6 lines · speck for Andi Kleen

In the future seccomp will have other side effects, such as disabling some CPU features. If an application does not use seccomp today, but wants to disable these CPU features, requiring PR_SET_NO_NEW_PRIVS could be a problem because it would prevent interacting with suid programs.

Quoted 6 lines

PR_SET_NO_NEW_PRIVS is only needed if seccomp can actually deny syscalls. Allow seccomp to set simple BPF programs consisting only of a "BPF_RET accept" even without the no_new_privs flag.

This allows any program to use such a simple seccomp rule without breaking suid use.

Extending seccomp (Tim's patch) is fair enough, since seccomp aware stuff is explicitly intended to be used for sandboxing, etc.

...but don't you think we also could need a new prctl to handle CPU features, or even just speculation features? I worry that we'll have to retro-fit seccomp to things like OpenJDK that were not using it before, in the next *week*, while a simple prctl is easily added as a one liner. Sure, you're going to say "but it's just an empty BPF_RET", but that's still more for folks who never touched it before to have to understand vs. just telling them to add a simple prctl they probably already know.

Quoted 2 lines

To me, it seems the idea of limiting speculation for seccomp (reasonable) was overloaded into "let's do it all there".

Andrew just confirmed what I thought above. For OpenJDK, he's not even sure there is enough time for other than the big hammer, but if there is then a prctl is pretty much the route he'd want to go.

Jon.

Signature · 1 line
Computer Architect | Sent from my Fedora powered laptop