Skip to contentSkip to conversation

bpf: security: New file mode and LSM hooks for eBPF object permission control

PATCH Superseded Chenbo Feng 3 participants 10 messages Oct 11, 2017 – Oct 12, 2017

Subsystems (10)
Networking Network drivers Kselftest BPF +6 more subsystems

v3 of 7 Superseded Latest is v7 →

lore
Share & tools
lore

Chenbo Feng

author
#0 lore raw

v3 cover bpf: security: New file mode and LSM hooks for eBPF object permission control

From: Chenbo Feng <[email protected]>

Much like files and sockets, eBPF objects are accessed, controlled, and shared via a file descriptor (FD). Unlike files and sockets, the existing mechanism for eBPF object access control is very limited. Currently there are two options for granting accessing to eBPF
operations: grant access to all processes, or only CAP_SYS_ADMIN processes. The CAP_SYS_ADMIN-only mode is not ideal because most users do not have this capability and granting a user CAP_SYS_ADMIN grants too many other security-sensitive permissions. It also unnecessarily allows all CAP_SYS_ADMIN processes access to eBPF functionality. Allowing all processes to access to eBPF objects is also undesirable since it has potential to allow unprivileged processes to consume kernel memory, and opens up attack surface to the kernel.

Adding LSM hooks maintains the status quo for systems which do not use an LSM, preserving compatibility with userspace, while allowing security modules to choose how best to handle permissions on eBPF objects. Here is a possible use case for the lsm hooks with selinux module:

The network-control daemon (netd) creates and loads an eBPF object for network packet filtering and analysis. It passes the object FD to an unprivileged network monitor app (netmonitor), which is not allowed to create, modify or load eBPF objects, but is allowed to read the traffic stats from the map.

Selinux could use these hooks to grant the following permissions:
allow netd self:bpf_map { create read write};
allow netmonitor netd:fd use;
allow netmonitor netd:bpf_map read;

In this patch series, A file mode is added to bpf map to store the accessing mode. With this file mode flags, the map can be obtained read only, write only or read and write. With the help of this file mode, several security hooks can be added to the eBPF syscall implementations to do permissions checks. These LSM hooks are mainly focused on checking the process privileges before it obtains the fd for a specific bpf object. No matter from a file location or from a eBPF id. Besides that, a general check hook is also implemented at the start of bpf syscalls so that each security module can have their own implementation on the reset of bpf object related functionalities.

In order to store the ownership and security information about eBPF maps, a security field pointer is added to the struct bpf_map. And the last two patch set are implementation of selinux check on these hooks introduced, plus an additional check when eBPF object is passed between processes using unix socket as well as binder IPC.

Change since V1:

- Whitelist the new bpf flags in the map allocate check.
- Added bpf selftest for the new flags.
- Added two new security hooks for copying the security information from the bpf object security struct to file security struct
- Simplified the checking action when bpf fd is passed between processes.

Change since V2:

- Fixed the line break problem for map flags check
- Fixed the typo in selinux check of file mode.
- Merge bpf_map and bpf_prog into one selinux class
- Added bpf_type and bpf_sid into file security struct to store the security information when generate fd.
- Add the hook to bpf_map_new_fd and bpf_prog_new_fd.

Chenbo Feng (5):
bpf: Add file mode configuration into bpf maps
bpf: Add tests for eBPF file mode
security: bpf: Add LSM hooks for bpf object related syscall
selinux: bpf: Add selinux check for eBPF syscall operations
selinux: bpf: Add addtional check for bpf object file receive

 include/linux/bpf.h                     |  12 ++-
 include/linux/lsm_hooks.h               |  71 +++++++++++++
 include/linux/security.h                |  54 ++++++++++
 include/uapi/linux/bpf.h                |   6 ++
 kernel/bpf/arraymap.c                   |   6 +-
 kernel/bpf/devmap.c                     |   5 +-
 kernel/bpf/hashtab.c                    |   5 +-
 kernel/bpf/inode.c                      |  15 ++-
 kernel/bpf/lpm_trie.c                   |   3 +-
 kernel/bpf/sockmap.c                    |   5 +-
 kernel/bpf/stackmap.c                   |   5 +-
 kernel/bpf/syscall.c                    | 108 ++++++++++++++++++--
 security/security.c                     |  40 ++++++++
 security/selinux/hooks.c                | 174 ++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h     |   2 +
 security/selinux/include/objsec.h       |  13 +++
 tools/testing/selftests/bpf/test_maps.c |  48 +++++++++
 17 files changed, 548 insertions(+), 24 deletions(-)
Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#1 lore raw

PATCH v3 1/5 bpf: Add file mode configuration into bpf maps

From: Chenbo Feng <[email protected]>

Introduce the map read/write flags to the eBPF syscalls that returns the map fd. The flags is used to set up the file mode when construct a new file descriptor for bpf maps. To not break the backward capability, the f_flags is set to O_RDWR if the flag passed by syscall is 0. Otherwise it should be O_RDONLY or O_WRONLY. When the userspace want to modify or read the map content, it will check the file mode to see if it is allowed to make the change.

Signed-off-by Chenbo Feng <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
---
 include/linux/bpf.h      |  6 ++--
 include/uapi/linux/bpf.h |  6 ++++
 kernel/bpf/arraymap.c    |  6 +++-
 kernel/bpf/devmap.c      |  5 ++-
 kernel/bpf/hashtab.c     |  5 +--
 kernel/bpf/inode.c       | 15 ++++++---
 kernel/bpf/lpm_trie.c    |  3 +-
 kernel/bpf/sockmap.c     |  5 ++-
 kernel/bpf/stackmap.c    |  5 ++-
 kernel/bpf/syscall.c     | 80 +++++++++++++++++++++++++++++++++++++++++++-----
 10 files changed, 114 insertions(+), 22 deletions(-)
include/linux/bpf.h +4 -2
index bc7da2ddfcaf..0e9ca2555d7f 100644
@@ -308,11 +308,11 @@ void bpf_map_area_free(void *base);
 
 extern int sysctl_unprivileged_bpf_disabled;
 
-int bpf_map_new_fd(struct bpf_map *map);
+int bpf_map_new_fd(struct bpf_map *map, int flags);
 int bpf_prog_new_fd(struct bpf_prog *prog);
 
 int bpf_obj_pin_user(u32 ufd, const char __user *pathname);
-int bpf_obj_get_user(const char __user *pathname);
+int bpf_obj_get_user(const char __user *pathname, int flags);
 
 int bpf_percpu_hash_copy(struct bpf_map *map, void *key, void *value);
 int bpf_percpu_array_copy(struct bpf_map *map, void *key, void *value);

@@ -331,6 +331,8 @@ int bpf_fd_htab_map_update_elem(struct bpf_map *map, struct file *map_file,
 				void *key, void *value, u64 map_flags);
 int bpf_fd_htab_map_lookup_elem(struct bpf_map *map, void *key, u32 *value);
 
+int bpf_get_file_flag(int flags);
+
 /* memcpy that is used with 8-byte aligned pointers, power-of-8 size and
  * forced to use 'long' read/writes to try to atomically copy long counters.
  * Best-effort only.  No barriers here, since it _will_ race with concurrent
include/uapi/linux/bpf.h +6 -0
index 6db9e1d679cd..9cb50a228c39 100644
@@ -217,6 +217,10 @@ enum bpf_attach_type {
 
 #define BPF_OBJ_NAME_LEN 16U
 
+/* Flags for accessing BPF object */
+#define BPF_F_RDONLY		(1U << 3)
+#define BPF_F_WRONLY		(1U << 4)
+
 union bpf_attr {
 	struct { /* anonymous struct used by BPF_MAP_CREATE command */
 		__u32	map_type;	/* one of enum bpf_map_type */

@@ -259,6 +263,7 @@ union bpf_attr {
 	struct { /* anonymous struct used by BPF_OBJ_* commands */
 		__aligned_u64	pathname;
 		__u32		bpf_fd;
+		__u32		file_flags;
 	};
 
 	struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */

@@ -286,6 +291,7 @@ union bpf_attr {
 			__u32		map_id;
 		};
 		__u32		next_id;
+		__u32		open_flags;
 	};
 
 	struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
kernel/bpf/arraymap.c +5 -1
index 68d866628be0..988c04c91e10 100644
@@ -19,6 +19,9 @@
 
 #include "map_in_map.h"
 
+#define ARRAY_CREATE_FLAG_MASK \
+	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
 static void bpf_array_free_percpu(struct bpf_array *array)
 {
 	int i;

@@ -56,7 +59,8 @@ static struct bpf_map *array_map_alloc(union bpf_attr *attr)
 
 	/* check sanity of attributes */
 	if (attr->max_entries == 0 || attr->key_size != 4 ||
-	    attr->value_size == 0 || attr->map_flags & ~BPF_F_NUMA_NODE ||
+	    attr->value_size == 0 ||
+	    attr->map_flags & ~ARRAY_CREATE_FLAG_MASK ||
 	    (percpu && numa_node != NUMA_NO_NODE))
 		return ERR_PTR(-EINVAL);
kernel/bpf/devmap.c +4 -1
index e093d9a2c4dd..e5d3de7cff2e 100644
@@ -50,6 +50,9 @@
 #include <linux/bpf.h>
 #include <linux/filter.h>
 
+#define DEV_CREATE_FLAG_MASK \
+	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
 struct bpf_dtab_netdev {
 	struct net_device *dev;
 	struct bpf_dtab *dtab;

@@ -80,7 +83,7 @@ static struct bpf_map *dev_map_alloc(union bpf_attr *attr)
 
 	/* check sanity of attributes */
 	if (attr->max_entries == 0 || attr->key_size != 4 ||
-	    attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
+	    attr->value_size != 4 || attr->map_flags & ~DEV_CREATE_FLAG_MASK)
 		return ERR_PTR(-EINVAL);
 
 	dtab = kzalloc(sizeof(*dtab), GFP_USER);
kernel/bpf/hashtab.c +3 -2
index 431126f31ea3..919955236e63 100644
@@ -18,8 +18,9 @@
 #include "bpf_lru_list.h"
 #include "map_in_map.h"
 
-#define HTAB_CREATE_FLAG_MASK \
-	(BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE)
+#define HTAB_CREATE_FLAG_MASK						\
+	(BPF_F_NO_PREALLOC | BPF_F_NO_COMMON_LRU | BPF_F_NUMA_NODE |	\
+	 BPF_F_RDONLY | BPF_F_WRONLY)
 
 struct bucket {
 	struct hlist_nulls_head head;
kernel/bpf/inode.c +10 -5
index e833ed914358..7d8c6dd8dd5d 100644
@@ -295,7 +295,7 @@ int bpf_obj_pin_user(u32 ufd, const char __user *pathname)
 }
 
 static void *bpf_obj_do_get(const struct filename *pathname,
-			    enum bpf_type *type)
+			    enum bpf_type *type, int flags)
 {
 	struct inode *inode;
 	struct path path;

@@ -307,7 +307,7 @@ static void *bpf_obj_do_get(const struct filename *pathname,
 		return ERR_PTR(ret);
 
 	inode = d_backing_inode(path.dentry);
-	ret = inode_permission(inode, MAY_WRITE);
+	ret = inode_permission(inode, ACC_MODE(flags));
 	if (ret)
 		goto out;
 

@@ -326,18 +326,23 @@ static void *bpf_obj_do_get(const struct filename *pathname,
 	return ERR_PTR(ret);
 }
 
-int bpf_obj_get_user(const char __user *pathname)
+int bpf_obj_get_user(const char __user *pathname, int flags)
 {
 	enum bpf_type type = BPF_TYPE_UNSPEC;
 	struct filename *pname;
 	int ret = -ENOENT;
+	int f_flags;
 	void *raw;
 
+	f_flags = bpf_get_file_flag(flags);
+	if (f_flags < 0)
+		return f_flags;
+
 	pname = getname(pathname);
 	if (IS_ERR(pname))
 		return PTR_ERR(pname);
 
-	raw = bpf_obj_do_get(pname, &type);
+	raw = bpf_obj_do_get(pname, &type, f_flags);
 	if (IS_ERR(raw)) {
 		ret = PTR_ERR(raw);
 		goto out;

@@ -346,7 +351,7 @@ int bpf_obj_get_user(const char __user *pathname)
 	if (type == BPF_TYPE_PROG)
 		ret = bpf_prog_new_fd(raw);
 	else if (type == BPF_TYPE_MAP)
-		ret = bpf_map_new_fd(raw);
+		ret = bpf_map_new_fd(raw, f_flags);
 	else
 		goto out;
kernel/bpf/lpm_trie.c +2 -1
index 34d8a690ea05..885e45479680 100644
@@ -495,7 +495,8 @@ static int trie_delete_elem(struct bpf_map *map, void *_key)
 #define LPM_KEY_SIZE_MAX	LPM_KEY_SIZE(LPM_DATA_SIZE_MAX)
 #define LPM_KEY_SIZE_MIN	LPM_KEY_SIZE(LPM_DATA_SIZE_MIN)
 
-#define LPM_CREATE_FLAG_MASK	(BPF_F_NO_PREALLOC | BPF_F_NUMA_NODE)
+#define LPM_CREATE_FLAG_MASK	(BPF_F_NO_PREALLOC | BPF_F_NUMA_NODE |	\
+				 BPF_F_RDONLY | BPF_F_WRONLY)
 
 static struct bpf_map *trie_alloc(union bpf_attr *attr)
 {
kernel/bpf/sockmap.c +4 -1
index a298d6666698..86ec846f2d5e 100644
@@ -40,6 +40,9 @@
 #include <linux/list.h>
 #include <net/strparser.h>
 
+#define SOCK_CREATE_FLAG_MASK \
+	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
 struct bpf_stab {
 	struct bpf_map map;
 	struct sock **sock_map;

@@ -489,7 +492,7 @@ static struct bpf_map *sock_map_alloc(union bpf_attr *attr)
 
 	/* check sanity of attributes */
 	if (attr->max_entries == 0 || attr->key_size != 4 ||
-	    attr->value_size != 4 || attr->map_flags & ~BPF_F_NUMA_NODE)
+	    attr->value_size != 4 || attr->map_flags & ~SOCK_CREATE_FLAG_MASK)
 		return ERR_PTR(-EINVAL);
 
 	if (attr->value_size > KMALLOC_MAX_SIZE)
kernel/bpf/stackmap.c +4 -1
index 135be433e9a0..a15bc636cc98 100644
@@ -11,6 +11,9 @@
 #include <linux/perf_event.h>
 #include "percpu_freelist.h"
 
+#define STACK_CREATE_FLAG_MASK \
+	(BPF_F_NUMA_NODE | BPF_F_RDONLY | BPF_F_WRONLY)
+
 struct stack_map_bucket {
 	struct pcpu_freelist_node fnode;
 	u32 hash;

@@ -60,7 +63,7 @@ static struct bpf_map *stack_map_alloc(union bpf_attr *attr)
 	if (!capable(CAP_SYS_ADMIN))
 		return ERR_PTR(-EPERM);
 
-	if (attr->map_flags & ~BPF_F_NUMA_NODE)
+	if (attr->map_flags & ~STACK_CREATE_FLAG_MASK)
 		return ERR_PTR(-EINVAL);
 
 	/* check sanity of attributes */
kernel/bpf/syscall.c +72 -8

181 lines not on this page · raw message

Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html
author
#2 lore raw

PATCH v3 2/5 bpf: Add tests for eBPF file mode

In reply to Chenbo Feng

From: Chenbo Feng <[email protected]>

Two related tests are added into bpf selftest to test read only map and write only map. The tests verified the read only and write only flags are working on hash maps.

Signed-off-by Chenbo Feng <[email protected]>
---
 tools/testing/selftests/bpf/test_maps.c | 48 +++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)
tools/testing/selftests/bpf/test_maps.c +48 -0
index fe3a443a1102..896f23cfe918 100644
@@ -1033,6 +1033,51 @@ static void test_map_parallel(void)
 	assert(bpf_map_get_next_key(fd, &key, &key) == -1 && errno == ENOENT);
 }
 
+static void test_map_rdonly(void)
+{
+	int i, fd, key = 0, value = 0;
+
+	fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+			    MAP_SIZE, map_flags | BPF_F_RDONLY);
+	if (fd < 0) {
+		printf("Failed to create map for read only test '%s'!\n",
+		       strerror(errno));
+		exit(1);
+	}
+
+	key = 1;
+	value = 1234;
+	/* Insert key=1 element. */
+	assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == -1 &&
+	       errno == EPERM);
+
+	/* Check that key=2 is not found. */
+	assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == ENOENT);
+	assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == ENOENT);
+}
+
+static void test_map_wronly(void)
+{
+	int i, fd, key = 0, value = 0;
+
+	fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value),
+			    MAP_SIZE, map_flags | BPF_F_WRONLY);
+	if (fd < 0) {
+		printf("Failed to create map for read only test '%s'!\n",
+		       strerror(errno));
+		exit(1);
+	}
+
+	key = 1;
+	value = 1234;
+	/* Insert key=1 element. */
+	assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == 0)
+
+	/* Check that key=2 is not found. */
+	assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == EPERM);
+	assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == EPERM);
+}
+
 static void run_all_tests(void)
 {
 	test_hashmap(0, NULL);

@@ -1050,6 +1095,9 @@ static void run_all_tests(void)
 	test_map_large();
 	test_map_parallel();
 	test_map_stress();
+
+	test_map_rdonly();
+	test_map_wronly();
 }
 
 int main(void)
Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info@ http://vger.kernel.org/majordomo-info.html
author
#3 lore raw

PATCH v3 3/5 security: bpf: Add LSM hooks for bpf object related syscall

In reply to Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce several LSM hooks for the syscalls that will allow the userspace to access to eBPF object such as eBPF programs and eBPF maps. The security check is aimed to enforce a per object security protection for eBPF object so only processes with the right priviliges can read/write to a specific map or use a specific eBPF program. Besides that, a general security hook is added before the multiplexer of bpf syscall to check the cmd and the attribute used for the command. The actual security module can decide which command need to be checked and how the cmd should be checked.

Signed-off-by Chenbo Feng <[email protected]>
---
 include/linux/bpf.h       |  6 ++++++
 include/linux/lsm_hooks.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/security.h  | 45 +++++++++++++++++++++++++++++++++++++++
 kernel/bpf/syscall.c      | 28 ++++++++++++++++++++++--
 security/security.c       | 32 ++++++++++++++++++++++++++++
 5 files changed, 163 insertions(+), 2 deletions(-)
include/linux/bpf.h +6 -0
index 0e9ca2555d7f..225740688ab7 100644
@@ -57,6 +57,9 @@ struct bpf_map {
 	atomic_t usercnt;
 	struct bpf_map *inner_map_meta;
 	char name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+	void *security;
+#endif
 };
 
 /* function argument constraints */

@@ -190,6 +193,9 @@ struct bpf_prog_aux {
 	struct user_struct *user;
 	u64 load_time; /* ns since boottime */
 	char name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+	void *security;
+#endif
 	union {
 		struct work_struct work;
 		struct rcu_head	rcu;
include/linux/lsm_hooks.h +54 -0

72 lines not on this page · raw message

include/linux/security.h +45 -0

57 lines not on this page · raw message

kernel/bpf/syscall.c +26 -2
index b02582ead9a4..1cf31ddd7616 100644
@@ -210,6 +210,7 @@ static void bpf_map_free_deferred(struct work_struct *work)
 	struct bpf_map *map = container_of(work, struct bpf_map, work);
 
 	bpf_map_uncharge_memlock(map);
+	security_bpf_map_free(map);
 	/* implementation dependent freeing */
 	map->ops->map_free(map);
 }

@@ -323,6 +324,9 @@ static const struct file_operations bpf_map_fops = {
 
 int bpf_map_new_fd(struct bpf_map *map, int flags)
 {
+	if (security_bpf_map(map, OPEN_FMODE(flags)))
+		return -EPERM;
+
 	return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
 				flags | O_CLOEXEC);
 }

@@ -403,10 +407,14 @@ static int map_create(union bpf_attr *attr)
 	atomic_set(&map->refcnt, 1);
 	atomic_set(&map->usercnt, 1);
 
-	err = bpf_map_charge_memlock(map);
+	err = security_bpf_map_alloc(map);
 	if (err)
 		goto free_map_nouncharge;
 
+	err = bpf_map_charge_memlock(map);
+	if (err)
+		goto free_map_sec;
+
 	err = bpf_map_alloc_id(map);
 	if (err)
 		goto free_map;

@@ -428,6 +436,8 @@ static int map_create(union bpf_attr *attr)
 
 free_map:
 	bpf_map_uncharge_memlock(map);
+free_map_sec:
+	security_bpf_map_free(map);
 free_map_nouncharge:
 	map->ops->map_free(map);
 	return err;

@@ -906,6 +916,7 @@ static void __bpf_prog_put_rcu(struct rcu_head *rcu)
 
 	free_used_maps(aux);
 	bpf_prog_uncharge_memlock(aux->prog);
+	security_bpf_prog_free(aux);
 	bpf_prog_free(aux->prog);
 }
 

@@ -964,6 +975,9 @@ static const struct file_operations bpf_prog_fops = {
 
 int bpf_prog_new_fd(struct bpf_prog *prog)
 {
+	if (security_bpf_prog(prog))
+		return -EPERM;
+
 	return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
 				O_RDWR | O_CLOEXEC);
 }

@@ -1103,10 +1117,14 @@ static int bpf_prog_load(union bpf_attr *attr)
 	if (!prog)
 		return -ENOMEM;
 
-	err = bpf_prog_charge_memlock(prog);
+	err = security_bpf_prog_alloc(prog->aux);
 	if (err)
 		goto free_prog_nouncharge;
 
+	err = bpf_prog_charge_memlock(prog);
+	if (err)
+		goto free_prog_sec;
+
 	prog->len = attr->insn_cnt;
 
 	err = -EFAULT;

@@ -1164,6 +1182,8 @@ static int bpf_prog_load(union bpf_attr *attr)
 	free_used_maps(prog->aux);
 free_prog:
 	bpf_prog_uncharge_memlock(prog);
+free_prog_sec:
+	security_bpf_prog_free(prog->aux);
 free_prog_nouncharge:
 	bpf_prog_free(prog);
 	return err;

@@ -1630,6 +1650,10 @@ SYSCALL_DEFINE3(bpf, int, cmd, union bpf_attr __user *, uattr, unsigned int, siz
 	if (copy_from_user(&attr, uattr, size) != 0)
 		return -EFAULT;
 
+	err = security_bpf(cmd, &attr, size);
+	if (err)
+		return -EPERM;
+
 	switch (cmd) {
 	case BPF_MAP_CREATE:
 		err = map_create(&attr);
security/security.c +32 -0
index 4bf0f571b4ef..1cd8526cb0b7 100644
@@ -12,6 +12,7 @@
  *	(at your option) any later version.
  */
 
+#include <linux/bpf.h>
 #include <linux/capability.h>
 #include <linux/dcache.h>
 #include <linux/module.h>

@@ -1703,3 +1704,34 @@ int security_audit_rule_match(u32 secid, u32 field, u32 op, void *lsmrule,
 				actx);
 }
 #endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)
+{
+	return call_int_hook(bpf, 0, cmd, attr, size);
+}
+int security_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+	return call_int_hook(bpf_map, 0, map, fmode);
+}
+int security_bpf_prog(struct bpf_prog *prog)
+{
+	return call_int_hook(bpf_prog, 0, prog);
+}
+int security_bpf_map_alloc(struct bpf_map *map)
+{
+	return call_int_hook(bpf_map_alloc_security, 0, map);
+}
+int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+	return call_int_hook(bpf_prog_alloc_security, 0, aux);
+}
+void security_bpf_map_free(struct bpf_map *map)
+{
+	call_void_hook(bpf_map_free_security, map);
+}
+void security_bpf_prog_free(struct bpf_prog_aux *aux)
+{
+	call_void_hook(bpf_prog_free_security, aux);
+}
+#endif /* CONFIG_BPF_SYSCALL */
Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#4 lore raw

PATCH v3 4/5 selinux: bpf: Add selinux check for eBPF syscall operations

In reply to Chenbo Feng

From: Chenbo Feng <[email protected]>

Implement the actual checks introduced to eBPF related syscalls. This implementation use the security field inside bpf object to store a sid that identify the bpf object. And when processes try to access the object, selinux will check if processes have the right privileges. The creation of eBPF object are also checked at the general bpf check hook and new cmd introduced to eBPF domain can also be checked there.

Signed-off-by Chenbo Feng <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
---
 security/selinux/hooks.c            | 111 ++++++++++++++++++++++++++++++++++++
 security/selinux/include/classmap.h |   2 +
 security/selinux/include/objsec.h   |   4 ++
 3 files changed, 117 insertions(+)
security/selinux/hooks.c +111 -0
index f5d304736852..94e473b9c884 100644
@@ -85,6 +85,7 @@
 #include <linux/export.h>
 #include <linux/msg.h>
 #include <linux/shm.h>
+#include <linux/bpf.h>
 
 #include "avc.h"
 #include "objsec.h"

@@ -6252,6 +6253,106 @@ static void selinux_ib_free_security(void *ib_sec)
 }
 #endif
 
+#ifdef CONFIG_BPF_SYSCALL
+static int selinux_bpf(int cmd, union bpf_attr *attr,
+				     unsigned int size)
+{
+	u32 sid = current_sid();
+	int ret;
+
+	switch (cmd) {
+	case BPF_MAP_CREATE:
+		ret = avc_has_perm(sid, sid, SECCLASS_BPF, BPF__MAP_CREATE,
+				   NULL);
+		break;
+	case BPF_PROG_LOAD:
+		ret = avc_has_perm(sid, sid, SECCLASS_BPF, BPF__PROG_LOAD,
+				   NULL);
+		break;
+	default:
+		ret = 0;
+		break;
+	}
+
+	return ret;
+}
+
+static u32 bpf_map_fmode_to_av(fmode_t fmode)
+{
+	u32 av = 0;
+
+	if (fmode & FMODE_READ)
+		av |= BPF__MAP_READ;
+	if (fmode & FMODE_WRITE)
+		av |= BPF__MAP_WRITE;
+	return av;
+}
+
+static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+	u32 sid = current_sid();
+	struct bpf_security_struct *bpfsec;
+
+	bpfsec = map->security;
+	return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
+			    bpf_map_fmode_to_av(fmode), NULL);
+}
+
+static int selinux_bpf_prog(struct bpf_prog *prog)
+{
+	u32 sid = current_sid();
+	struct bpf_security_struct *bpfsec;
+
+	bpfsec = prog->aux->security;
+	return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF,
+			    BPF__PROG_USE, NULL);
+}
+
+static int selinux_bpf_map_alloc(struct bpf_map *map)
+{
+	struct bpf_security_struct *bpfsec;
+
+	bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
+	if (!bpfsec)
+		return -ENOMEM;
+
+	bpfsec->sid = current_sid();
+	map->security = bpfsec;
+
+	return 0;
+}
+
+static void selinux_bpf_map_free(struct bpf_map *map)
+{
+	struct bpf_security_struct *bpfsec = map->security;
+
+	map->security = NULL;
+	kfree(bpfsec);
+}
+
+static int selinux_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+	struct bpf_security_struct *bpfsec;
+
+	bpfsec = kzalloc(sizeof(*bpfsec), GFP_KERNEL);
+	if (!bpfsec)
+		return -ENOMEM;
+
+	bpfsec->sid = current_sid();
+	aux->security = bpfsec;
+
+	return 0;
+}
+
+static void selinux_bpf_prog_free(struct bpf_prog_aux *aux)
+{
+	struct bpf_security_struct *bpfsec = aux->security;
+
+	aux->security = NULL;
+	kfree(bpfsec);
+}
+#endif
+
 static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(binder_set_context_mgr, selinux_binder_set_context_mgr),
 	LSM_HOOK_INIT(binder_transaction, selinux_binder_transaction),

@@ -6471,6 +6572,16 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(audit_rule_match, selinux_audit_rule_match),
 	LSM_HOOK_INIT(audit_rule_free, selinux_audit_rule_free),
 #endif
+
+#ifdef CONFIG_BPF_SYSCALL
+	LSM_HOOK_INIT(bpf, selinux_bpf),
+	LSM_HOOK_INIT(bpf_map, selinux_bpf_map),
+	LSM_HOOK_INIT(bpf_prog, selinux_bpf_prog),
+	LSM_HOOK_INIT(bpf_map_alloc_security, selinux_bpf_map_alloc),
+	LSM_HOOK_INIT(bpf_prog_alloc_security, selinux_bpf_prog_alloc),
+	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
+	LSM_HOOK_INIT(bpf_prog_free_security, selinux_bpf_prog_free),
+#endif
 };
 
 static __init int selinux_init(void)
security/selinux/include/classmap.h +2 -0
index 35ffb29a69cb..a91fa46a789f 100644
@@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
 	  { "access", NULL } },
 	{ "infiniband_endport",
 	  { "manage_subnet", NULL } },
+	{ "bpf",
+	  {"map_create", "map_read", "map_write", "prog_load", "prog_use"} },
 	{ NULL }
   };
security/selinux/include/objsec.h +4 -0
index 1649cd18eb0b..3d54468ce334 100644
@@ -150,6 +150,10 @@ struct pkey_security_struct {
 	u32	sid;	/* SID of pkey */
 };
 
+struct bpf_security_struct {
+	u32 sid;  /*SID of bpf obj creater*/
+};
+
 extern unsigned int selinux_checkreqprot;
 
 #endif /* _SELINUX_OBJSEC_H_ */
Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#5 lore raw

PATCH v3 5/5 selinux: bpf: Add addtional check for bpf object file receive

In reply to Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce a bpf object related check when sending and receiving files through unix domain socket as well as binder. It checks if the receiving process have privilege to read/write the bpf map or use the bpf program. This check is necessary because the bpf maps and programs are using a anonymous inode as their shared inode so the normal way of checking the files and sockets when passing between processes cannot work properly on eBPF object. This check only works when the BPF_SYSCALL is configured. The information stored inside the file security struct is the same as the information in bpf object security struct.

Signed-off-by Chenbo Feng <[email protected]>
---
 include/linux/lsm_hooks.h         | 17 ++++++++++
 include/linux/security.h          |  9 ++++++
 kernel/bpf/syscall.c              | 27 ++++++++++++++--
 security/security.c               |  8 +++++
 security/selinux/hooks.c          | 67 +++++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |  9 ++++++
 6 files changed, 135 insertions(+), 2 deletions(-)
include/linux/lsm_hooks.h +17 -0
index 7161d8e7ee79..517dea60b87b 100644
@@ -1385,6 +1385,19 @@
  * @bpf_prog_free_security:
  *	Clean up the security information stored inside bpf prog.
  *
+ * @bpf_map_file:
+ *	When creating a bpf map fd, set up the file security information with
+ *	the bpf security information stored in the map struct. So when the map
+ *	fd is passed between processes, the security module can directly read
+ *	the security information from file security struct rather than the bpf
+ *	security struct.
+ *
+ * @bpf_prog_file:
+ *	When creating a bpf prog fd, set up the file security information with
+ *	the bpf security information stored in the prog struct. So when the prog
+ *	fd is passed between processes, the security module can directly read
+ *	the security information from file security struct rather than the bpf
+ *	security struct.
  */
 union security_list_options {
 	int (*binder_set_context_mgr)(struct task_struct *mgr);

@@ -1726,6 +1739,8 @@ union security_list_options {
 	void (*bpf_map_free_security)(struct bpf_map *map);
 	int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
 	void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+	void (*bpf_map_file)(struct bpf_map *map, struct file *file);
+	void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file *file);
 #endif /* CONFIG_BPF_SYSCALL */
 };
 

@@ -1954,6 +1969,8 @@ struct security_hook_heads {
 	struct list_head bpf_map_free_security;
 	struct list_head bpf_prog_alloc_security;
 	struct list_head bpf_prog_free_security;
+	struct list_head bpf_map_file;
+	struct list_head bpf_prog_file;
 #endif /* CONFIG_BPF_SYSCALL */
 } __randomize_layout;
include/linux/security.h +9 -0
index 18800b0911e5..57573b794e2d 100644
@@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct bpf_map *map);
 extern void security_bpf_map_free(struct bpf_map *map);
 extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
 extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+extern void security_bpf_map_file(struct bpf_map *map, struct file *file);
+extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct file *file);
 #else
 static inline int security_bpf(int cmd, union bpf_attr *attr,
 					     unsigned int size)

@@ -1772,6 +1774,13 @@ static inline int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
 
 static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 { }
+
+static inline void security_bpf_map_file(struct bpf_map *map, struct file *file)
+{ }
+
+static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
+					  struct file *file)
+{ }
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_BPF_SYSCALL */
kernel/bpf/syscall.c +25 -2
index 1cf31ddd7616..aee69e564c50 100644
@@ -324,11 +324,22 @@ static const struct file_operations bpf_map_fops = {
 
 int bpf_map_new_fd(struct bpf_map *map, int flags)
 {
+	int fd;
+	struct fd f;
 	if (security_bpf_map(map, OPEN_FMODE(flags)))
 		return -EPERM;
 
-	return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
+	fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
 				flags | O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+	security_bpf_map_file(map, f.file);
+	fdput(f);
+	return fd;
 }
 
 int bpf_get_file_flag(int flags)

@@ -975,11 +986,23 @@ static const struct file_operations bpf_prog_fops = {
 
 int bpf_prog_new_fd(struct bpf_prog *prog)
 {
+	int fd;
+	struct fd f;
+
 	if (security_bpf_prog(prog))
 		return -EPERM;
 
-	return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
+	fd =  anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
 				O_RDWR | O_CLOEXEC);
+	if (fd < 0)
+		return fd;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+	security_bpf_prog_file(prog->aux, f.file);
+	fdput(f);
+	return fd;
 }
 
 static struct bpf_prog *____bpf_prog_get(struct fd f)
security/security.c +8 -0
index 1cd8526cb0b7..dacf649b8cfa 100644
@@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct bpf_prog_aux *aux)
 {
 	call_void_hook(bpf_prog_free_security, aux);
 }
+void security_bpf_map_file(struct bpf_map *map, struct file *file)
+{
+	call_void_hook(bpf_map_file, map, file);
+}
+void security_bpf_prog_file(struct bpf_prog_aux *aux, struct file *file)
+{
+	call_void_hook(bpf_prog_file, aux, file);
+}
 #endif /* CONFIG_BPF_SYSCALL */
security/selinux/hooks.c +67 -0

103 lines not on this page · raw message

security/selinux/include/objsec.h +9 -0
index 3d54468ce334..0162648761f9 100644
@@ -67,11 +67,20 @@ struct inode_security_struct {
 	spinlock_t lock;
 };
 
+enum bpf_obj_type {
+	BPF_MAP = 1,
+	BPF_PROG,
+};
+
 struct file_security_struct {
 	u32 sid;		/* SID of open file description */
 	u32 fown_sid;		/* SID of file owner (for SIGIO) */
 	u32 isid;		/* SID of inode at the time of file open */
 	u32 pseqno;		/* Policy seqno at the time of file open */
+#ifdef CONFIG_BPF_SYSCALL
+	unsigned char bpf_type;
+	u32 bpf_sid;
+#endif
 };
 
 struct superblock_security_struct {
Signature · 6 lines
2.14.2.920.gcf0c67979c-goog

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#6 lore raw

Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive

Quoted 129 lines · Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce a bpf object related check when sending and receiving files through unix domain socket as well as binder. It checks if the receiving
process have privilege to read/write the bpf map or use the bpf program.
This check is necessary because the bpf maps and programs are using a anonymous inode as their shared inode so the normal way of checking the
files and sockets when passing between processes cannot work properly on
eBPF object. This check only works when the BPF_SYSCALL is configured.
The information stored inside the file security struct is the same as the information in bpf object security struct.

Signed-off-by Chenbo Feng <[email protected]>

---

?include/linux/lsm_hooks.h?????????| 17 ++++++++++
?include/linux/security.h??????????|??9 ++++++
?kernel/bpf/syscall.c??????????????| 27 ++++++++++++++--
?security/security.c???????????????|??8 +++++
?security/selinux/hooks.c??????????| 67

+++++++++++++++++++++++++++++++++++++++
?security/selinux/include/objsec.h |??9 ++++++ ?6 files changed, 135 insertions(+), 2 deletions(-)

include/linux/lsm_hooks.h +0 -0
index 7161d8e7ee79..517dea60b87b 100644
@@ -1385,6 +1385,19 @@

? * @bpf_prog_free_security:
? * Clean up the security information stored inside bpf prog.
? *
+ * @bpf_map_file:
+ * When creating a bpf map fd, set up the file security
information with
+ * the bpf security information stored in the map struct. So
when the map
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
+ *
+ * @bpf_prog_file:
+ * When creating a bpf prog fd, set up the file security
information with
+ * the bpf security information stored in the prog struct. So
when the prog
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
? */
?union security_list_options {
? int (*binder_set_context_mgr)(struct task_struct *mgr);

include/linux/lsm_hooks.h +2 -0
@@ -1726,6 +1739,8 @@ union security_list_options {
 ?	void (*bpf_map_free_security)(struct bpf_map *map);
 ?	int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
 ?	void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+	void (*bpf_map_file)(struct bpf_map *map, struct file
 *file);
+	void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
 *file);

?#endif /* CONFIG_BPF_SYSCALL */
?};
?

include/linux/lsm_hooks.h +2 -0
@@ -1954,6 +1969,8 @@ struct security_hook_heads {
 ?	struct list_head bpf_map_free_security;
 ?	struct list_head bpf_prog_alloc_security;
 ?	struct list_head bpf_prog_free_security;
+	struct list_head bpf_map_file;
+	struct list_head bpf_prog_file;

?#endif /* CONFIG_BPF_SYSCALL */
?} __randomize_layout;
?

include/linux/security.h +2 -0
index 18800b0911e5..57573b794e2d 100644
@@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
 bpf_map *map);
 ?extern void security_bpf_map_free(struct bpf_map *map);
 ?extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
 ?extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+extern void security_bpf_map_file(struct bpf_map *map, struct file
 *file);
+extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
 file *file);

?#else
?static inline int security_bpf(int cmd, union bpf_attr *attr,
? ?????unsigned int size)

include/linux/security.h +0 -0
@@ -1772,6 +1774,13 @@ static inline int
 security_bpf_prog_alloc(struct bpf_prog_aux *aux)

?
?static inline void security_bpf_prog_free(struct bpf_prog_aux *aux) ?{ }

include/linux/security.h +2 -0
+
+static inline void security_bpf_map_file(struct bpf_map *map, struct

file *file)

include/linux/security.h +5 -0
+{ }
+
+static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
+					??struct file *file)
+{ }

?#endif /* CONFIG_SECURITY */
?#endif /* CONFIG_BPF_SYSCALL */
?

kernel/bpf/syscall.c +0 -0
index 1cf31ddd7616..aee69e564c50 100644
@@ -324,11 +324,22 @@ static const struct file_operations
 bpf_map_fops = {

?
?int bpf_map_new_fd(struct bpf_map *map, int flags) ?{

kernel/bpf/syscall.c +2 -0
+	int fd;
+	struct fd f;

? if (security_bpf_map(map, OPEN_FMODE(flags)))
? return -EPERM;
?

kernel/bpf/syscall.c +1 -1
-	return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
+	fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,

? flags | O_CLOEXEC);

kernel/bpf/syscall.c static const struct file_operations bpf_map_fops = { · line 339

Quoted 6 lines
kernel/bpf/syscall.c +6 -0
+	if (fd < 0)
+		return fd;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;

This seems convoluted and unnecessarily inefficient, since anon_inode_getfd() has the struct file and could have directly returned it instead of having to go through fdget() on a fd we just installed. Also, couldn't the fd->file mapping have changed underneath us between fd_install() and fdget()?
I would think it would be safer and more efficient to create an anon_inode_getfdandfile() or similar interface and use that, so that we can just pass the file it set up to the hook. Obviously that would need to be reviewed by the vfs folks.

Quoted 207 lines
kernel/bpf/syscall.c +3 -0
+	security_bpf_map_file(map, f.file);
+	fdput(f);
+	return fd;

?}
?
?int bpf_get_file_flag(int flags)

kernel/bpf/syscall.c +0 -0
@@ -975,11 +986,23 @@ static const struct file_operations
 bpf_prog_fops = {

?
?int bpf_prog_new_fd(struct bpf_prog *prog)
?{

kernel/bpf/syscall.c +3 -0
+	int fd;
+	struct fd f;
+

? if (security_bpf_prog(prog))
? return -EPERM;
?

kernel/bpf/syscall.c +1 -1
-	return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
+	fd =??anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,

? O_RDWR | O_CLOEXEC);

kernel/bpf/syscall.c +9 -0
+	if (fd < 0)
+		return fd;
+
+	f = fdget(fd);
+	if (!f.file)
+		return -EBADF;
+	security_bpf_prog_file(prog->aux, f.file);
+	fdput(f);
+	return fd;

?}
?
?static struct bpf_prog *____bpf_prog_get(struct fd f)

security/security.c +5 -0
index 1cd8526cb0b7..dacf649b8cfa 100644
@@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
 bpf_prog_aux *aux)
 ?{
 ?	call_void_hook(bpf_prog_free_security, aux);
 ?}
+void security_bpf_map_file(struct bpf_map *map, struct file *file)
+{
+	call_void_hook(bpf_map_file, map, file);
+}
+void security_bpf_prog_file(struct bpf_prog_aux *aux, struct file

*file)

security/security.c +3 -0
+{
+	call_void_hook(bpf_prog_file, aux, file);
+}

?#endif /* CONFIG_BPF_SYSCALL */

security/selinux/hooks.c +0 -0
index 94e473b9c884..0a6ef20513b0 100644
@@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
 struct cred *cred,
 ?	return inode_has_perm(cred, file_inode(file), av, &ad);
 ?}

?

security/selinux/hooks.c +4 -0
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_file_check(struct file *file, u32 sid);
+#endif
+

?/* Check whether a task can use an open file descriptor to ????access an inode in a given way.??Check access to the ????descriptor itself, and then use dentry_has_perm to

security/selinux/hooks.c +0 -0
@@ -1845,6 +1849,14 @@ static int file_has_perm(const struct cred
 *cred,
 ?			goto out;
 ?	}

?

security/selinux/hooks.c +8 -0
+#ifdef CONFIG_BPF_SYSCALL
+	if (fsec->bpf_type) {
+		rc = bpf_file_check(file, cred_sid(cred));
+		if (rc)
+			goto out;
+	}
+#endif
+

? /* av is zero if only checking access to the descriptor. */
? rc = 0;
? if (av)

security/selinux/hooks.c +0 -0
@@ -2165,6 +2177,14 @@ static int selinux_binder_transfer_file(struct
 task_struct *from,
 ?			return rc;
 ?	}

?

security/selinux/hooks.c +8 -0
+#ifdef CONFIG_BPF_SYSCALL
+	if (fsec->bpf_type) {
+		rc = bpf_file_check(file, sid);
+		if (rc)
+			return rc;
+	}
+#endif
+

? if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
? return 0;
?

security/selinux/hooks.c +0 -0
@@ -6288,6 +6308,33 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
 ?	return av;
 ?}

?
+/* This function will check the file pass through unix socket or binder to see
+ * if it is a bpf related object. And apply correspinding checks on the bpf
+ * object based on the type. The bpf maps and programs, not like other files and
+ * socket, are using a shared anonymous inode inside the kernel as their inode.
+ * So checking that inode cannot identify if the process have privilege to
+ * access the bpf object and that's why we have to add this additional check in
+ * selinux_file_receive and selinux_binder_transfer_files.
+ */

security/selinux/hooks.c +8 -0
+static int bpf_file_check(struct file *file, u32 sid)
+{
+	struct file_security_struct *fsec = file->f_security;
+	int ret;
+
+	if (fsec->bpf_type == BPF_MAP) {
+		ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
+				???bpf_map_fmode_to_av(file-
Quoted 1 line

f_mode), NULL);

security/selinux/hooks.c +11 -0
+		if (ret)
+			return ret;
+	} else if (fsec->bpf_type == BPF_PROG) {
+		ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
+				???BPF__PROG_USE, NULL);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+

?static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) ?{
? u32 sid = current_sid();

security/selinux/hooks.c +18 -0
@@ -6351,6 +6398,24 @@ static void selinux_bpf_prog_free(struct
 bpf_prog_aux *aux)
 ?	aux->security = NULL;
 ?	kfree(bpfsec);
 ?}
+
+static void selinux_bpf_map_file(struct bpf_map *map, struct file
 *file)
+{
+	struct bpf_security_struct *bpfsec = map->security;
+	struct file_security_struct *fsec = file->f_security;
+
+	fsec->bpf_type = BPF_MAP;
+	fsec->bpf_sid = bpfsec->sid;
+}
+
+static void selinux_bpf_prog_file(struct bpf_prog_aux *aux, struct
 file *file)
+{
+	struct bpf_security_struct *bpfsec = aux->security;
+	struct file_security_struct *fsec = file->f_security;
+
+	fsec->bpf_type = BPF_PROG;
+	fsec->bpf_sid = bpfsec->sid;
+}

?#endif
?
?static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {

security/selinux/hooks.c +2 -0
@@ -6581,6 +6646,8 @@ static struct security_hook_list
 selinux_hooks[] __lsm_ro_after_init = {
 ?	LSM_HOOK_INIT(bpf_prog_alloc_security,
 selinux_bpf_prog_alloc),
 ?	LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
 ?	LSM_HOOK_INIT(bpf_prog_free_security,
 selinux_bpf_prog_free),
+	LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
+	LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),

?#endif
?};
?

security/selinux/include/objsec.h +0 -0
index 3d54468ce334..0162648761f9 100644
@@ -67,11 +67,20 @@ struct inode_security_struct {
 ?	spinlock_t lock;
 ?};

?

security/selinux/include/objsec.h +5 -0
+enum bpf_obj_type {
+	BPF_MAP = 1,
+	BPF_PROG,
+};
+

?struct file_security_struct {
? u32 sid; /* SID of open file description */
? u32 fown_sid; /* SID of file owner (for
SIGIO) */
? u32 isid; /* SID of inode at the time of file
open */
? u32 pseqno; /* Policy seqno at the time of
file open */

security/selinux/include/objsec.h +4 -0
+#ifdef CONFIG_BPF_SYSCALL
+	unsigned char bpf_type;
+	u32 bpf_sid;
+#endif
Quoted 1 line

?};

Can you check how this impacts the size of the file_security_cache objects, and thus the memory overhead imposed on all open files?

If it is significant, do we need to cache the bpf_sid here or could we just store the bpf_type and then dereference the bpfsec if it is a map or prog?

Quoted 2 lines

?
?struct superblock_security_struct {

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

#7 lore raw

Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive

Quoted 138 lines · Stephen Smalley
Quoted 125 lines · Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce a bpf object related check when sending and receiving files through unix domain socket as well as binder. It checks if the receiving
process have privilege to read/write the bpf map or use the bpf program.
This check is necessary because the bpf maps and programs are using a anonymous inode as their shared inode so the normal way of checking the
files and sockets when passing between processes cannot work properly on
eBPF object. This check only works when the BPF_SYSCALL is configured.
The information stored inside the file security struct is the same as the information in bpf object security struct.

Signed-off-by Chenbo Feng <[email protected]>
---
 include/linux/lsm_hooks.h         | 17 ++++++++++
 include/linux/security.h          |  9 ++++++
 kernel/bpf/syscall.c              | 27 ++++++++++++++--
 security/security.c               |  8 +++++
 security/selinux/hooks.c          | 67
+++++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |  9 ++++++
 6 files changed, 135 insertions(+), 2 deletions(-)
include/linux/lsm_hooks.h +2 -0
index 7161d8e7ee79..517dea60b87b 100644
@@ -1385,6 +1385,19 @@
  * @bpf_prog_free_security:
  *   Clean up the security information stored inside bpf prog.
  *
+ * @bpf_map_file:
+ *   When creating a bpf map fd, set up the file security

information with
+ * the bpf security information stored in the map struct. So
when the map
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
+ *
+ * @bpf_prog_file:
+ * When creating a bpf prog fd, set up the file security
information with
+ * the bpf security information stored in the prog struct. So
when the prog
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
*/

 union security_list_options {
      int (*binder_set_context_mgr)(struct task_struct *mgr);
include/linux/lsm_hooks.h +2 -0
@@ -1726,6 +1739,8 @@ union security_list_options {
      void (*bpf_map_free_security)(struct bpf_map *map);
      int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
      void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+     void (*bpf_map_file)(struct bpf_map *map, struct file
 *file);
+     void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
 *file);
 #endif /* CONFIG_BPF_SYSCALL */

};

include/linux/lsm_hooks.h +2 -0
@@ -1954,6 +1969,8 @@ struct security_hook_heads {
      struct list_head bpf_map_free_security;
      struct list_head bpf_prog_alloc_security;
      struct list_head bpf_prog_free_security;
+     struct list_head bpf_map_file;
+     struct list_head bpf_prog_file;
 #endif /* CONFIG_BPF_SYSCALL */
 } __randomize_layout;
include/linux/security.h +2 -0
index 18800b0911e5..57573b794e2d 100644
@@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
 bpf_map *map);
 extern void security_bpf_map_free(struct bpf_map *map);
 extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
 extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+extern void security_bpf_map_file(struct bpf_map *map, struct file
 *file);
+extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
 file *file);

#else
static inline int security_bpf(int cmd, union bpf_attr *attr,
unsigned int size)

include/linux/security.h +7 -0
@@ -1772,6 +1774,13 @@ static inline int
 security_bpf_prog_alloc(struct bpf_prog_aux *aux)
 
 static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 { }
+
+static inline void security_bpf_map_file(struct bpf_map *map, struct
 file *file)
+{ }
+
+static inline void security_bpf_prog_file(struct bpf_prog_aux *aux,
+                                       struct file *file)
+{ }
 #endif /* CONFIG_SECURITY */

#endif /* CONFIG_BPF_SYSCALL */

kernel/bpf/syscall.c +2 -0
index 1cf31ddd7616..aee69e564c50 100644
@@ -324,11 +324,22 @@ static const struct file_operations
 bpf_map_fops = {
 
 int bpf_map_new_fd(struct bpf_map *map, int flags)
 {
+     int fd;
+     struct fd f;
      if (security_bpf_map(map, OPEN_FMODE(flags)))
              return -EPERM;

kernel/bpf/syscall.c static const struct file_operations · line 340

Quoted 10 lines
kernel/bpf/syscall.c +7 -1
 
-     return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
+     fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
                              flags | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;

This seems convoluted and unnecessarily inefficient, since

Quoted 9 lines

anon_inode_getfd() has the struct file and could have directly returned it instead of having to go through fdget() on a fd we just installed. Also, couldn't the fd->file mapping have changed underneath us between fd_install() and fdget()?
I would think it would be safer and more efficient to create an anon_inode_getfdandfile() or similar interface and use that, so that we can just pass the file it set up to the hook. Obviously that would need to be reviewed by the vfs folks.

Do you mean create a anonymous inode interface specifically for eBPF object? Is it okay that we add the hooks inside anon_inode_getfd and pass the file to the hook before fd install.

Quoted 212 lines
Quoted 203 lines
kernel/bpf/syscall.c +3 -0
+     security_bpf_map_file(map, f.file);
+     fdput(f);
+     return fd;
 }

int bpf_get_file_flag(int flags)

kernel/bpf/syscall.c +13 -1
@@ -975,11 +986,23 @@ static const struct file_operations
 bpf_prog_fops = {
 
 int bpf_prog_new_fd(struct bpf_prog *prog)
 {
+     int fd;
+     struct fd f;
+
      if (security_bpf_prog(prog))
              return -EPERM;
 
-     return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
+     fd =  anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
                              O_RDWR | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;
+     security_bpf_prog_file(prog->aux, f.file);
+     fdput(f);
+     return fd;
 }

static struct bpf_prog *____bpf_prog_get(struct fd f)

security/security.c +5 -0
index 1cd8526cb0b7..dacf649b8cfa 100644
@@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
 bpf_prog_aux *aux)
 {
      call_void_hook(bpf_prog_free_security, aux);
 }
+void security_bpf_map_file(struct bpf_map *map, struct file *file)
+{
+     call_void_hook(bpf_map_file, map, file);
+}
+void security_bpf_prog_file(struct bpf_prog_aux *aux, struct file

*file)

security/security.c +3 -0
+{
+     call_void_hook(bpf_prog_file, aux, file);
+}
 #endif /* CONFIG_BPF_SYSCALL */
security/selinux/hooks.c +4 -0
index 94e473b9c884..0a6ef20513b0 100644
@@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
 struct cred *cred,
      return inode_has_perm(cred, file_inode(file), av, &ad);
 }
 
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_file_check(struct file *file, u32 sid);
+#endif
+
 /* Check whether a task can use an open file descriptor to
    access an inode in a given way.  Check access to the

descriptor itself, and then use dentry_has_perm to

security/selinux/hooks.c +8 -0
@@ -1845,6 +1849,14 @@ static int file_has_perm(const struct cred
 *cred,
                      goto out;
      }
 
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, cred_sid(cred));
+             if (rc)
+                     goto out;
+     }
+#endif
+
      /* av is zero if only checking access to the descriptor. */
      rc = 0;

      if (av)
security/selinux/hooks.c +9 -0
@@ -2165,6 +2177,14 @@ static int selinux_binder_transfer_file(struct
 task_struct *from,
                      return rc;
      }
 
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, sid);
+             if (rc)
+                     return rc;
+     }
+#endif
+
      if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
              return 0;

@@ -6288,6 +6308,33 @@ static u32 bpf_map_fmode_to_av(fmode_t fmode)
      return av;
 }
 
+/* This function will check the file pass through unix socket or

binder to see
+ * if it is a bpf related object. And apply correspinding checks on the bpf
+ * object based on the type. The bpf maps and programs, not like other files and
+ * socket, are using a shared anonymous inode inside the kernel as their inode.
+ * So checking that inode cannot identify if the process have privilege to
+ * access the bpf object and that's why we have to add this additional check in
+ * selinux_file_receive and selinux_binder_transfer_files.
+ */

security/selinux/hooks.c +8 -0
+static int bpf_file_check(struct file *file, u32 sid)
+{
+     struct file_security_struct *fsec = file->f_security;
+     int ret;
+
+     if (fsec->bpf_type == BPF_MAP) {
+             ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
+                                bpf_map_fmode_to_av(file-
Quoted 1 line

f_mode), NULL);

security/selinux/hooks.c +29 -0
+             if (ret)
+                     return ret;
+     } else if (fsec->bpf_type == BPF_PROG) {
+             ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
+                                BPF__PROG_USE, NULL);
+             if (ret)
+                     return ret;
+     }
+     return 0;
+}
+
 static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
 {
      u32 sid = current_sid();

@@ -6351,6 +6398,24 @@ static void selinux_bpf_prog_free(struct
 bpf_prog_aux *aux)
      aux->security = NULL;
      kfree(bpfsec);
 }
+
+static void selinux_bpf_map_file(struct bpf_map *map, struct file
 *file)
+{
+     struct bpf_security_struct *bpfsec = map->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_MAP;
+     fsec->bpf_sid = bpfsec->sid;
+}
+
+static void selinux_bpf_prog_file(struct bpf_prog_aux *aux, struct
 file *file)
+{
+     struct bpf_security_struct *bpfsec = aux->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_PROG;
+     fsec->bpf_sid = bpfsec->sid;
+}

#endif

static struct security_hook_list selinux_hooks[] __lsm_ro_after_init
= {

security/selinux/hooks.c +2 -0
@@ -6581,6 +6646,8 @@ static struct security_hook_list
 selinux_hooks[] __lsm_ro_after_init = {
      LSM_HOOK_INIT(bpf_prog_alloc_security,
 selinux_bpf_prog_alloc),
      LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
      LSM_HOOK_INIT(bpf_prog_free_security,
 selinux_bpf_prog_free),
+     LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
+     LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),

#endif
};

security/selinux/include/objsec.h +5 -0
index 3d54468ce334..0162648761f9 100644
@@ -67,11 +67,20 @@ struct inode_security_struct {
      spinlock_t lock;
 };
 
+enum bpf_obj_type {
+     BPF_MAP = 1,
+     BPF_PROG,
+};
+
 struct file_security_struct {
      u32 sid;                /* SID of open file description */
      u32 fown_sid;           /* SID of file owner (for

SIGIO) */
u32 isid; /* SID of inode at the time of file
open */
u32 pseqno; /* Policy seqno at the time of
file open */

security/selinux/include/objsec.h struct inode_security_struct { · line 84

Quoted 5 lines
security/selinux/include/objsec.h +4 -0
+#ifdef CONFIG_BPF_SYSCALL
+     unsigned char bpf_type;
+     u32 bpf_sid;
+#endif
 };

Can you check how this impacts the size of the file_security_cache objects, and thus the memory overhead imposed on all open files?

Quoted 5 lines

If it is significant, do we need to cache the bpf_sid here or could we just store the bpf_type and then dereference the bpfsec if it is a map or prog?

From proc/slabinfo I find the number of object and the object size

grows a lot after adding this two field. I will try to dereference the bpfsec instead to see if it helps.

Quoted 2 lines
Quoted 2 lines

struct superblock_security_struct {

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

#8 lore raw

Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive

Quoted 165 lines · Chenbo Feng via Selinux
Quoted 153 lines · Stephen Smalley
Quoted 136 lines · Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce a bpf object related check when sending and receiving files
through unix domain socket as well as binder. It checks if the receiving
process have privilege to read/write the bpf map or use the bpf program.
This check is necessary because the bpf maps and programs are using a
anonymous inode as their shared inode so the normal way of checking
the
files and sockets when passing between processes cannot work properly
on
eBPF object. This check only works when the BPF_SYSCALL is configured.
The information stored inside the file security struct is the same as
the information in bpf object security struct.

Signed-off-by Chenbo Feng <[email protected]>

---
 include/linux/lsm_hooks.h         | 17 ++++++++++ include/linux/security.h          |  9 ++++++ kernel/bpf/syscall.c              | 27 ++++++++++++++-- security/security.c               |  8 +++++ security/selinux/hooks.c          | 67
+++++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |  9 ++++++

 6 files changed, 135 insertions(+), 2 deletions(-)
include/linux/lsm_hooks.h +2 -0
index 7161d8e7ee79..517dea60b87b 100644
@@ -1385,6 +1385,19 @@
   * @bpf_prog_free_security:
   *   Clean up the security information stored inside bpf prog.
   *
+ * @bpf_map_file:
+ *   When creating a bpf map fd, set up the file security

information with
+ *   the bpf security information stored in the map struct. So when the map
+ *   fd is passed between processes, the security module can directly read
+ *   the security information from file security struct rather than the bpf
+ *   security struct.
+ *
+ * @bpf_prog_file:
+ *   When creating a bpf prog fd, set up the file security information with
+ *   the bpf security information stored in the prog struct. So when the prog
+ *   fd is passed between processes, the security module can directly read
+ *   the security information from file security struct rather than the bpf
+ *   security struct.
  */
 union security_list_options {
      int (*binder_set_context_mgr)(struct task_struct *mgr);

include/linux/lsm_hooks.h +2 -0
@@ -1726,6 +1739,8 @@ union security_list_options {
       void (*bpf_map_free_security)(struct bpf_map *map);
       int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
       void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+     void (*bpf_map_file)(struct bpf_map *map, struct file
 *file);
+     void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
 *file);
  #endif /* CONFIG_BPF_SYSCALL */

 };

include/linux/lsm_hooks.h +2 -0
@@ -1954,6 +1969,8 @@ struct security_hook_heads {
       struct list_head bpf_map_free_security;
       struct list_head bpf_prog_alloc_security;
       struct list_head bpf_prog_free_security;
+     struct list_head bpf_map_file;
+     struct list_head bpf_prog_file;
  #endif /* CONFIG_BPF_SYSCALL */
  } __randomize_layout;
include/linux/security.h +1 -0
index 18800b0911e5..57573b794e2d 100644
@@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
 bpf_map *map);
  extern void security_bpf_map_free(struct bpf_map *map);
  extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
  extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+extern void security_bpf_map_file(struct bpf_map *map, struct

file
*file);
+extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
file *file);
 #else
 static inline int security_bpf(int cmd, union bpf_attr *attr, unsigned int size)

include/linux/security.h +0 -0
@@ -1772,6 +1774,13 @@ static inline int
 security_bpf_prog_alloc(struct bpf_prog_aux *aux)

 static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
 { }

include/linux/security.h +2 -0
+
+static inline void security_bpf_map_file(struct bpf_map *map,

struct
file *file)

include/linux/security.h +3 -0
+{ }
+
+static inline void security_bpf_prog_file(struct bpf_prog_aux

*aux,
+                                       struct file *file) +{ }
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_BPF_SYSCALL */

kernel/bpf/syscall.c +2 -0
index 1cf31ddd7616..aee69e564c50 100644
@@ -324,11 +324,22 @@ static const struct file_operations
 bpf_map_fops = {
 
  int bpf_map_new_fd(struct bpf_map *map, int flags)
  {
+     int fd;
+     struct fd f;
       if (security_bpf_map(map, OPEN_FMODE(flags)))
               return -EPERM;

kernel/bpf/syscall.c static const struct file_operations · line 340

Quoted 10 lines
kernel/bpf/syscall.c +7 -1
 
-     return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
+     fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
                               flags | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;

This seems convoluted and unnecessarily inefficient, since anon_inode_getfd() has the struct file and could have directly returned
it instead of having to go through fdget() on a fd we just installed.

Quoted 9 lines

Also, couldn't the fd->file mapping have changed underneath us between
fd_install() and fdget()?
I would think it would be safer and more efficient to create an anon_inode_getfdandfile() or similar interface and use that, so that we
can just pass the file it set up to the hook.  Obviously that would need to be reviewed by the vfs folks.

Quoted 3 lines

Do you mean create a anonymous inode interface specifically for eBPF object? Is it okay that we add the hooks inside anon_inode_getfd and pass the file to the hook before fd install.

No, I meant to create a general helper, anon_inode_getfile(), that returns the file and the fd to the caller, and then the BPF-specific logic can stay in the BPF code.

However, if storing the bpf type in the file_security_struct is in fact having a significant impact on per-file memory usage, then perhaps your original approach of exporting and testing the fops was the right one, albeit ugly.

Quoted 237 lines
Quoted 224 lines
Quoted 219 lines
kernel/bpf/syscall.c +3 -0
+     security_bpf_map_file(map, f.file);
+     fdput(f);
+     return fd;
  }

 int bpf_get_file_flag(int flags)

kernel/bpf/syscall.c +13 -1
@@ -975,11 +986,23 @@ static const struct file_operations
 bpf_prog_fops = {
 
  int bpf_prog_new_fd(struct bpf_prog *prog)
  {
+     int fd;
+     struct fd f;
+
       if (security_bpf_prog(prog))
               return -EPERM;
 
-     return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
+     fd =  anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
                               O_RDWR | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;
+     security_bpf_prog_file(prog->aux, f.file);
+     fdput(f);
+     return fd;
  }

 static struct bpf_prog *____bpf_prog_get(struct fd f)

security/security.c +1 -0
index 1cd8526cb0b7..dacf649b8cfa 100644
@@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
 bpf_prog_aux *aux)
  {
       call_void_hook(bpf_prog_free_security, aux);
  }
+void security_bpf_map_file(struct bpf_map *map, struct file

*file)

security/security.c +4 -0
+{
+     call_void_hook(bpf_map_file, map, file);
+}
+void security_bpf_prog_file(struct bpf_prog_aux *aux, struct

file
*file)

security/security.c +3 -0
+{
+     call_void_hook(bpf_prog_file, aux, file);
+}

 #endif /* CONFIG_BPF_SYSCALL */

security/selinux/hooks.c +4 -0
index 94e473b9c884..0a6ef20513b0 100644
@@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
 struct cred *cred,
       return inode_has_perm(cred, file_inode(file), av, &ad);
  }
 
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_file_check(struct file *file, u32 sid);
+#endif
+
  /* Check whether a task can use an open file descriptor to

    access an inode in a given way.  Check access to the descriptor itself, and then use dentry_has_perm to

security/selinux/hooks.c +8 -0
@@ -1845,6 +1849,14 @@ static int file_has_perm(const struct cred
 *cred,
                       goto out;
       }
 
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, cred_sid(cred));
+             if (rc)
+                     goto out;
+     }
+#endif
+
       /* av is zero if only checking access to the descriptor. */
       rc = 0;

      if (av)

security/selinux/hooks.c +0 -0
@@ -2165,6 +2177,14 @@ static int

selinux_binder_transfer_file(struct
task_struct *from,
                      return rc;
      }

security/selinux/hooks.c +8 -0
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, sid);
+             if (rc)
+                     return rc;
+     }
+#endif
+

      if (unlikely(IS_PRIVATE(d_backing_inode(dentry)))) return 0;

security/selinux/hooks.c +1 -0
@@ -6288,6 +6308,33 @@ static u32 bpf_map_fmode_to_av(fmode_t
 fmode)
       return av;
  }
 
+/* This function will check the file pass through unix socket or

binder to see
+ * if it is a bpf related object. And apply correspinding checks on
the bpf
+ * object based on the type. The bpf maps and programs, not like other files and
+ * socket, are using a shared anonymous inode inside the kernel as
their inode.
+ * So checking that inode cannot identify if the process have privilege to
+ * access the bpf object and that's why we have to add this additional check in
+ * selinux_file_receive and selinux_binder_transfer_files.
+ */

security/selinux/hooks.c +7 -0
+static int bpf_file_check(struct file *file, u32 sid)
+{
+     struct file_security_struct *fsec = file->f_security;
+     int ret;
+
+     if (fsec->bpf_type == BPF_MAP) {
+             ret = avc_has_perm(sid, fsec->bpf_sid,

SECCLASS_BPF,
+                                bpf_map_fmode_to_av(file-

Quoted 1 line

f_mode), NULL);

+             if (ret)
+                     return ret;
+     } else if (fsec->bpf_type == BPF_PROG) { +             ret = avc_has_perm(sid, fsec->bpf_sid, SECCLASS_BPF,
+                                BPF__PROG_USE, NULL); +             if (ret)
+                     return ret;
+     }
+     return 0;

security/selinux/hooks.c +2 -0
+}
+

 static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode) {
      u32 sid = current_sid();

security/selinux/hooks.c +2 -0
@@ -6351,6 +6398,24 @@ static void selinux_bpf_prog_free(struct
 bpf_prog_aux *aux)
       aux->security = NULL;
       kfree(bpfsec);
  }
+
+static void selinux_bpf_map_file(struct bpf_map *map, struct

file
*file)

security/selinux/hooks.c +9 -0
+{
+     struct bpf_security_struct *bpfsec = map->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_MAP;
+     fsec->bpf_sid = bpfsec->sid;
+}
+
+static void selinux_bpf_prog_file(struct bpf_prog_aux *aux,

struct
file *file)

security/selinux/hooks.c +7 -0
+{
+     struct bpf_security_struct *bpfsec = aux->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_PROG;
+     fsec->bpf_sid = bpfsec->sid;
+}

 #endif

 static struct security_hook_list selinux_hooks[]
__lsm_ro_after_init
= {

security/selinux/hooks.c +2 -0
@@ -6581,6 +6646,8 @@ static struct security_hook_list
 selinux_hooks[] __lsm_ro_after_init = {
       LSM_HOOK_INIT(bpf_prog_alloc_security,
 selinux_bpf_prog_alloc),
       LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
       LSM_HOOK_INIT(bpf_prog_free_security,
 selinux_bpf_prog_free),
+     LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
+     LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),

 #endif
 };

security/selinux/include/objsec.h +5 -0
index 3d54468ce334..0162648761f9 100644
@@ -67,11 +67,20 @@ struct inode_security_struct {
       spinlock_t lock;
  };
 
+enum bpf_obj_type {
+     BPF_MAP = 1,
+     BPF_PROG,
+};
+
  struct file_security_struct {

      u32 sid;                /* SID of open file description */ u32 fown_sid;           /* SID of file owner (for SIGIO) */
      u32 isid;               /* SID of inode at the time of file open */
      u32 pseqno;             /* Policy seqno at the time of file open */

security/selinux/include/objsec.h +4 -0
+#ifdef CONFIG_BPF_SYSCALL
+     unsigned char bpf_type;
+     u32 bpf_sid;
+#endif
Quoted 1 line

 };

Can you check how this impacts the size of the file_security_cache objects, and thus the memory overhead imposed on all open files?

Quoted 7 lines

If it is significant, do we need to cache the bpf_sid here or could we
just store the bpf_type and then dereference the bpfsec if it is a map
or prog?

From proc/slabinfo I find the number of object and the object size

grows a lot after adding this two field. I will try to dereference the
bpfsec instead to see if it helps.

Quoted 2 lines
Quoted 2 lines

 struct superblock_security_struct {

#9 lore raw

Re: [PATCH net-next v3 5/5] selinux: bpf: Add addtional check for bpf object file receive

Quoted 170 lines · Stephen Smalley
Quoted 165 lines · Chenbo Feng via Selinux
Quoted 153 lines · Stephen Smalley
Quoted 136 lines · Chenbo Feng

From: Chenbo Feng <[email protected]>

Introduce a bpf object related check when sending and receiving files
through unix domain socket as well as binder. It checks if the receiving
process have privilege to read/write the bpf map or use the bpf program.
This check is necessary because the bpf maps and programs are using a
anonymous inode as their shared inode so the normal way of checking
the
files and sockets when passing between processes cannot work properly
on
eBPF object. This check only works when the BPF_SYSCALL is configured.
The information stored inside the file security struct is the same as
the information in bpf object security struct.

Signed-off-by Chenbo Feng <[email protected]>
---
 include/linux/lsm_hooks.h         | 17 ++++++++++
 include/linux/security.h          |  9 ++++++
 kernel/bpf/syscall.c              | 27 ++++++++++++++--
 security/security.c               |  8 +++++
 security/selinux/hooks.c          | 67
+++++++++++++++++++++++++++++++++++++++
 security/selinux/include/objsec.h |  9 ++++++
 6 files changed, 135 insertions(+), 2 deletions(-)
include/linux/lsm_hooks.h +2 -0
index 7161d8e7ee79..517dea60b87b 100644
@@ -1385,6 +1385,19 @@
  * @bpf_prog_free_security:
  *   Clean up the security information stored inside bpf prog.
  *
+ * @bpf_map_file:
+ *   When creating a bpf map fd, set up the file security

information with
+ * the bpf security information stored in the map struct. So
when the map
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
+ *
+ * @bpf_prog_file:
+ * When creating a bpf prog fd, set up the file security
information with
+ * the bpf security information stored in the prog struct. So
when the prog
+ * fd is passed between processes, the security module can
directly read
+ * the security information from file security struct rather
than the bpf
+ * security struct.
*/

 union security_list_options {
      int (*binder_set_context_mgr)(struct task_struct *mgr);
include/linux/lsm_hooks.h +2 -0
@@ -1726,6 +1739,8 @@ union security_list_options {
      void (*bpf_map_free_security)(struct bpf_map *map);
      int (*bpf_prog_alloc_security)(struct bpf_prog_aux *aux);
      void (*bpf_prog_free_security)(struct bpf_prog_aux *aux);
+     void (*bpf_map_file)(struct bpf_map *map, struct file
 *file);
+     void (*bpf_prog_file)(struct bpf_prog_aux *aux, struct file
 *file);
 #endif /* CONFIG_BPF_SYSCALL */

};

include/linux/lsm_hooks.h +2 -0
@@ -1954,6 +1969,8 @@ struct security_hook_heads {
      struct list_head bpf_map_free_security;
      struct list_head bpf_prog_alloc_security;
      struct list_head bpf_prog_free_security;
+     struct list_head bpf_map_file;
+     struct list_head bpf_prog_file;
 #endif /* CONFIG_BPF_SYSCALL */
 } __randomize_layout;
include/linux/security.h +1 -0
index 18800b0911e5..57573b794e2d 100644
@@ -1740,6 +1740,8 @@ extern int security_bpf_map_alloc(struct
 bpf_map *map);
 extern void security_bpf_map_free(struct bpf_map *map);
 extern int security_bpf_prog_alloc(struct bpf_prog_aux *aux);
 extern void security_bpf_prog_free(struct bpf_prog_aux *aux);
+extern void security_bpf_map_file(struct bpf_map *map, struct

file
*file);
+extern void security_bpf_prog_file(struct bpf_prog_aux *aux, struct
file *file);
#else
static inline int security_bpf(int cmd, union bpf_attr *attr,
unsigned int size)

include/linux/security.h +2 -0
@@ -1772,6 +1774,13 @@ static inline int
 security_bpf_prog_alloc(struct bpf_prog_aux *aux)
 
 static inline void security_bpf_prog_free(struct bpf_prog_aux
 *aux)
 { }
+
+static inline void security_bpf_map_file(struct bpf_map *map,

struct
file *file)

include/linux/security.h +3 -0
+{ }
+
+static inline void security_bpf_prog_file(struct bpf_prog_aux

*aux,

include/linux/security.h +2 -0
+                                       struct file *file)
+{ }
 #endif /* CONFIG_SECURITY */
 #endif /* CONFIG_BPF_SYSCALL */
kernel/bpf/syscall.c +2 -0
index 1cf31ddd7616..aee69e564c50 100644
@@ -324,11 +324,22 @@ static const struct file_operations
 bpf_map_fops = {
 
 int bpf_map_new_fd(struct bpf_map *map, int flags)
 {
+     int fd;
+     struct fd f;
      if (security_bpf_map(map, OPEN_FMODE(flags)))
              return -EPERM;

kernel/bpf/syscall.c static const struct file_operations · line 340

Quoted 10 lines
kernel/bpf/syscall.c +7 -1
 
-     return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
+     fd = anon_inode_getfd("bpf-map", &bpf_map_fops, map,
                              flags | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;

This seems convoluted and unnecessarily inefficient, since anon_inode_getfd() has the struct file and could have directly returned
it instead of having to go through fdget() on a fd we just installed.

Quoted 9 lines

Also, couldn't the fd->file mapping have changed underneath us between
fd_install() and fdget()?
I would think it would be safer and more efficient to create an anon_inode_getfdandfile() or similar interface and use that, so that we
can just pass the file it set up to the hook. Obviously that would need to be reviewed by the vfs folks.

Quoted 3 lines

Do you mean create a anonymous inode interface specifically for eBPF object? Is it okay that we add the hooks inside anon_inode_getfd and pass the file to the hook before fd install.

Quoted 9 lines

No, I meant to create a general helper, anon_inode_getfile(), that returns the file and the fd to the caller, and then the BPF-specific logic can stay in the BPF code.

However, if storing the bpf type in the file_security_struct is in fact having a significant impact on per-file memory usage, then perhaps your original approach of exporting and testing the fops was the right one, albeit ugly.

It seems to me adding a bpf_type field inside the file_security_struct have the same impact on selinux_file_security cache size as adding both bpf_type and bpf_sid. Both implementation will cause a significant increase on memory size. I am testing on a ARM 64bit device and it might be different on x86 devices. I will return to the original way to implement the bpf_ops check instead of adding new field.

Quoted 230 lines
Quoted 230 lines
Quoted 230 lines
Quoted 215 lines
kernel/bpf/syscall.c +3 -0
+     security_bpf_map_file(map, f.file);
+     fdput(f);
+     return fd;
 }

int bpf_get_file_flag(int flags)

kernel/bpf/syscall.c +13 -1
@@ -975,11 +986,23 @@ static const struct file_operations
 bpf_prog_fops = {
 
 int bpf_prog_new_fd(struct bpf_prog *prog)
 {
+     int fd;
+     struct fd f;
+
      if (security_bpf_prog(prog))
              return -EPERM;
 
-     return anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
+     fd =  anon_inode_getfd("bpf-prog", &bpf_prog_fops, prog,
                              O_RDWR | O_CLOEXEC);
+     if (fd < 0)
+             return fd;
+
+     f = fdget(fd);
+     if (!f.file)
+             return -EBADF;
+     security_bpf_prog_file(prog->aux, f.file);
+     fdput(f);
+     return fd;
 }

static struct bpf_prog *____bpf_prog_get(struct fd f)

security/security.c +1 -0
index 1cd8526cb0b7..dacf649b8cfa 100644
@@ -1734,4 +1734,12 @@ void security_bpf_prog_free(struct
 bpf_prog_aux *aux)
 {
      call_void_hook(bpf_prog_free_security, aux);
 }
+void security_bpf_map_file(struct bpf_map *map, struct file

*file)

security/security.c +4 -0
+{
+     call_void_hook(bpf_map_file, map, file);
+}
+void security_bpf_prog_file(struct bpf_prog_aux *aux, struct

file
*file)

security/security.c +3 -0
+{
+     call_void_hook(bpf_prog_file, aux, file);
+}
 #endif /* CONFIG_BPF_SYSCALL */
security/selinux/hooks.c +4 -0
index 94e473b9c884..0a6ef20513b0 100644
@@ -1815,6 +1815,10 @@ static inline int file_path_has_perm(const
 struct cred *cred,
      return inode_has_perm(cred, file_inode(file), av, &ad);
 }
 
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_file_check(struct file *file, u32 sid);
+#endif
+
 /* Check whether a task can use an open file descriptor to
    access an inode in a given way.  Check access to the

descriptor itself, and then use dentry_has_perm to

security/selinux/hooks.c +8 -0
@@ -1845,6 +1849,14 @@ static int file_has_perm(const struct cred
 *cred,
                      goto out;
      }
 
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, cred_sid(cred));
+             if (rc)
+                     goto out;
+     }
+#endif
+
      /* av is zero if only checking access to the descriptor. */
      rc = 0;

      if (av)
security/selinux/hooks.c +0 -0
@@ -2165,6 +2177,14 @@ static int

selinux_binder_transfer_file(struct
task_struct *from,

                      return rc;
      }
security/selinux/hooks.c +9 -0
+#ifdef CONFIG_BPF_SYSCALL
+     if (fsec->bpf_type) {
+             rc = bpf_file_check(file, sid);
+             if (rc)
+                     return rc;
+     }
+#endif
+
      if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
              return 0;

@@ -6288,6 +6308,33 @@ static u32 bpf_map_fmode_to_av(fmode_t
 fmode)
      return av;
 }
 
+/* This function will check the file pass through unix socket or

binder to see
+ * if it is a bpf related object. And apply correspinding checks on
the bpf
+ * object based on the type. The bpf maps and programs, not like other files and
+ * socket, are using a shared anonymous inode inside the kernel as
their inode.
+ * So checking that inode cannot identify if the process have privilege to
+ * access the bpf object and that's why we have to add this additional check in
+ * selinux_file_receive and selinux_binder_transfer_files.
+ */

security/selinux/hooks.c +7 -0
+static int bpf_file_check(struct file *file, u32 sid)
+{
+     struct file_security_struct *fsec = file->f_security;
+     int ret;
+
+     if (fsec->bpf_type == BPF_MAP) {
+             ret = avc_has_perm(sid, fsec->bpf_sid,

SECCLASS_BPF,
+ bpf_map_fmode_to_av(file-

Quoted 1 line

f_mode), NULL);

security/selinux/hooks.c +4 -0
+             if (ret)
+                     return ret;
+     } else if (fsec->bpf_type == BPF_PROG) {
+             ret = avc_has_perm(sid, fsec->bpf_sid,

SECCLASS_BPF,

security/selinux/hooks.c +9 -0
+                                BPF__PROG_USE, NULL);
+             if (ret)
+                     return ret;
+     }
+     return 0;
+}
+
 static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
 {
      u32 sid = current_sid();

@@ -6351,6 +6398,24 @@ static void selinux_bpf_prog_free(struct
 bpf_prog_aux *aux)
      aux->security = NULL;
      kfree(bpfsec);
 }
+
+static void selinux_bpf_map_file(struct bpf_map *map, struct

file
*file)

security/selinux/hooks.c +9 -0
+{
+     struct bpf_security_struct *bpfsec = map->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_MAP;
+     fsec->bpf_sid = bpfsec->sid;
+}
+
+static void selinux_bpf_prog_file(struct bpf_prog_aux *aux,

struct
file *file)

security/selinux/hooks.c +7 -0
+{
+     struct bpf_security_struct *bpfsec = aux->security;
+     struct file_security_struct *fsec = file->f_security;
+
+     fsec->bpf_type = BPF_PROG;
+     fsec->bpf_sid = bpfsec->sid;
+}
 #endif
 
 static struct security_hook_list selinux_hooks[]

__lsm_ro_after_init
= {

security/selinux/hooks.c +2 -0
@@ -6581,6 +6646,8 @@ static struct security_hook_list
 selinux_hooks[] __lsm_ro_after_init = {
      LSM_HOOK_INIT(bpf_prog_alloc_security,
 selinux_bpf_prog_alloc),
      LSM_HOOK_INIT(bpf_map_free_security, selinux_bpf_map_free),
      LSM_HOOK_INIT(bpf_prog_free_security,
 selinux_bpf_prog_free),
+     LSM_HOOK_INIT(bpf_map_file, selinux_bpf_map_file),
+     LSM_HOOK_INIT(bpf_prog_file, selinux_bpf_prog_file),

#endif
};

security/selinux/include/objsec.h +5 -0
index 3d54468ce334..0162648761f9 100644
@@ -67,11 +67,20 @@ struct inode_security_struct {
      spinlock_t lock;
 };
 
+enum bpf_obj_type {
+     BPF_MAP = 1,
+     BPF_PROG,
+};
+
 struct file_security_struct {
      u32 sid;                /* SID of open file description */
      u32 fown_sid;           /* SID of file owner (for

SIGIO) */
u32 isid; /* SID of inode at the time of file
open */
u32 pseqno; /* Policy seqno at the time of
file open */

security/selinux/include/objsec.h struct inode_security_struct { · line 84

Quoted 5 lines
security/selinux/include/objsec.h +4 -0
+#ifdef CONFIG_BPF_SYSCALL
+     unsigned char bpf_type;
+     u32 bpf_sid;
+#endif
 };

Can you check how this impacts the size of the file_security_cache objects, and thus the memory overhead imposed on all open files?

If it is significant, do we need to cache the bpf_sid here or could we
just store the bpf_type and then dereference the bpfsec if it is a map
or prog?

Quoted 7 lines
Quoted 7 lines
Quoted 1 line

From proc/slabinfo I find the number of object and the object size

grows a lot after adding this two field. I will try to dereference the
bpfsec instead to see if it helps.

Quoted 2 lines
Quoted 2 lines

struct superblock_security_struct {

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html