* [PATCH 0/4] net-next: security: New file mode and LSM hooks for eBPF object permission control
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov,
Daniel Borkmann, Chenbo Feng
From: Chenbo Feng <fengc@google.com>
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.
Chenbo Feng (4):
Add file mode configuration into bpf maps
Add LSM hooks for bpf object related syscall
Add selinux check for eBPF syscall operations
Add addtional check for bpf object file receive
include/linux/bpf.h | 15 +++-
include/linux/lsm_hooks.h | 54 ++++++++++++
include/linux/security.h | 45 ++++++++++
include/uapi/linux/bpf.h | 6 ++
kernel/bpf/inode.c | 15 ++--
kernel/bpf/syscall.c | 112 +++++++++++++++++++++---
security/security.c | 32 +++++++
security/selinux/hooks.c | 168 +++++++++++++++++++++++++++++++++++-
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 +
10 files changed, 433 insertions(+), 20 deletions(-)
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH 0/4] net-next: security: New file mode and LSM hooks for eBPF object permission control
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: linux-security-module
From: Chenbo Feng <fengc@google.com>
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.
Chenbo Feng (4):
Add file mode configuration into bpf maps
Add LSM hooks for bpf object related syscall
Add selinux check for eBPF syscall operations
Add addtional check for bpf object file receive
include/linux/bpf.h | 15 +++-
include/linux/lsm_hooks.h | 54 ++++++++++++
include/linux/security.h | 45 ++++++++++
include/uapi/linux/bpf.h | 6 ++
kernel/bpf/inode.c | 15 ++--
kernel/bpf/syscall.c | 112 +++++++++++++++++++++---
security/security.c | 32 +++++++
security/selinux/hooks.c | 168 +++++++++++++++++++++++++++++++++++-
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 +
10 files changed, 433 insertions(+), 20 deletions(-)
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 18:29 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov,
Daniel Borkmann, Chenbo Feng
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 6 ++--
include/uapi/linux/bpf.h | 6 ++++
kernel/bpf/inode.c | 15 ++++++---
kernel/bpf/syscall.c | 80 +++++++++++++++++++++++++++++++++++++++++++-----
4 files changed, 92 insertions(+), 15 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 252f4bc9eb25..d826be859589 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -273,11 +273,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);
@@ -296,6 +296,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
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6d2137b4cf38..e812be5946a6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -177,6 +177,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 */
@@ -219,6 +223,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 */
@@ -246,6 +251,7 @@ union bpf_attr {
__u32 map_id;
};
__u32 next_id;
+ __u32 open_flags;
};
struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index e833ed914358..7d8c6dd8dd5d 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -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;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b927da66f653..7c8a9964a41d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -294,17 +294,48 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
+static ssize_t bpf_dummy_read(struct file *filp, char __user *buf, size_t siz,
+ loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_READ.
+ */
+ return -EINVAL;
+}
+
+static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
+ size_t siz, loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_WRITE.
+ */
+ return -EINVAL;
+}
+
static const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
.release = bpf_map_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
-int bpf_map_new_fd(struct bpf_map *map)
+int bpf_map_new_fd(struct bpf_map *map, int flags)
{
return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
- O_RDWR | O_CLOEXEC);
+ flags | O_CLOEXEC);
+}
+
+int bpf_get_file_flag(int flags)
+{
+ if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
+ return -EINVAL;
+ if (flags & BPF_F_RDONLY)
+ return O_RDONLY;
+ if (flags & BPF_F_WRONLY)
+ return O_WRONLY;
+ return O_RDWR;
}
/* helper macro to check that unused fields 'union bpf_attr' are zero */
@@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
{
int numa_node = bpf_map_attr_numa_node(attr);
struct bpf_map *map;
+ int f_flags;
int err;
err = CHECK_ATTR(BPF_MAP_CREATE);
if (err)
return -EINVAL;
+ f_flags = bpf_get_file_flag(attr->map_flags);
+ if (f_flags < 0)
+ return f_flags;
+
if (numa_node != NUMA_NO_NODE &&
((unsigned int)numa_node >= nr_node_ids ||
!node_online(numa_node)))
@@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
if (err)
goto free_map;
- err = bpf_map_new_fd(map);
+ err = bpf_map_new_fd(map, f_flags);
if (err < 0) {
/* failed to allocate fd.
* bpf_map_put() is needed because the above
@@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -571,6 +612,11 @@ static int map_update_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -654,6 +700,11 @@ static int map_delete_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -697,6 +748,11 @@ static int map_get_next_key(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
if (ukey) {
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
@@ -903,6 +959,8 @@ static const struct file_operations bpf_prog_fops = {
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
.release = bpf_prog_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
int bpf_prog_new_fd(struct bpf_prog *prog)
@@ -1112,11 +1170,11 @@ static int bpf_prog_load(union bpf_attr *attr)
return err;
}
-#define BPF_OBJ_LAST_FIELD bpf_fd
+#define BPF_OBJ_LAST_FIELD file_flags
static int bpf_obj_pin(const union bpf_attr *attr)
{
- if (CHECK_ATTR(BPF_OBJ))
+ if (CHECK_ATTR(BPF_OBJ) || attr->file_flags != 0)
return -EINVAL;
return bpf_obj_pin_user(attr->bpf_fd, u64_to_user_ptr(attr->pathname));
@@ -1127,7 +1185,8 @@ static int bpf_obj_get(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0)
return -EINVAL;
- return bpf_obj_get_user(u64_to_user_ptr(attr->pathname));
+ return bpf_obj_get_user(u64_to_user_ptr(attr->pathname),
+ attr->file_flags);
}
#ifdef CONFIG_CGROUP_BPF
@@ -1341,12 +1400,13 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
-#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD map_id
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD open_flags
static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
{
struct bpf_map *map;
u32 id = attr->map_id;
+ int f_flags;
int fd;
if (CHECK_ATTR(BPF_MAP_GET_FD_BY_ID))
@@ -1355,6 +1415,10 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
+ f_flags = bpf_get_file_flag(attr->open_flags);
+ if (f_flags < 0)
+ return f_flags;
+
spin_lock_bh(&map_idr_lock);
map = idr_find(&map_idr, id);
if (map)
@@ -1366,7 +1430,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
- fd = bpf_map_new_fd(map);
+ fd = bpf_map_new_fd(map, f_flags);
if (fd < 0)
bpf_map_put(map);
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: linux-security-module
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Daniel Borkmann <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
include/linux/bpf.h | 6 ++--
include/uapi/linux/bpf.h | 6 ++++
kernel/bpf/inode.c | 15 ++++++---
kernel/bpf/syscall.c | 80 +++++++++++++++++++++++++++++++++++++++++++-----
4 files changed, 92 insertions(+), 15 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index 252f4bc9eb25..d826be859589 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -273,11 +273,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);
@@ -296,6 +296,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
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 6d2137b4cf38..e812be5946a6 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -177,6 +177,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 */
@@ -219,6 +223,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 */
@@ -246,6 +251,7 @@ union bpf_attr {
__u32 map_id;
};
__u32 next_id;
+ __u32 open_flags;
};
struct { /* anonymous struct used by BPF_OBJ_GET_INFO_BY_FD */
diff --git a/kernel/bpf/inode.c b/kernel/bpf/inode.c
index e833ed914358..7d8c6dd8dd5d 100644
--- a/kernel/bpf/inode.c
+++ b/kernel/bpf/inode.c
@@ -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;
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index b927da66f653..7c8a9964a41d 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -294,17 +294,48 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
+static ssize_t bpf_dummy_read(struct file *filp, char __user *buf, size_t siz,
+ loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_READ.
+ */
+ return -EINVAL;
+}
+
+static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
+ size_t siz, loff_t *ppos)
+{
+ /* We need this handler such that alloc_file() enables
+ * f_mode with FMODE_CAN_WRITE.
+ */
+ return -EINVAL;
+}
+
static const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
.release = bpf_map_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
-int bpf_map_new_fd(struct bpf_map *map)
+int bpf_map_new_fd(struct bpf_map *map, int flags)
{
return anon_inode_getfd("bpf-map", &bpf_map_fops, map,
- O_RDWR | O_CLOEXEC);
+ flags | O_CLOEXEC);
+}
+
+int bpf_get_file_flag(int flags)
+{
+ if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
+ return -EINVAL;
+ if (flags & BPF_F_RDONLY)
+ return O_RDONLY;
+ if (flags & BPF_F_WRONLY)
+ return O_WRONLY;
+ return O_RDWR;
}
/* helper macro to check that unused fields 'union bpf_attr' are zero */
@@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
{
int numa_node = bpf_map_attr_numa_node(attr);
struct bpf_map *map;
+ int f_flags;
int err;
err = CHECK_ATTR(BPF_MAP_CREATE);
if (err)
return -EINVAL;
+ f_flags = bpf_get_file_flag(attr->map_flags);
+ if (f_flags < 0)
+ return f_flags;
+
if (numa_node != NUMA_NO_NODE &&
((unsigned int)numa_node >= nr_node_ids ||
!node_online(numa_node)))
@@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
if (err)
goto free_map;
- err = bpf_map_new_fd(map);
+ err = bpf_map_new_fd(map, f_flags);
if (err < 0) {
/* failed to allocate fd.
* bpf_map_put() is needed because the above
@@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -571,6 +612,11 @@ static int map_update_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -654,6 +700,11 @@ static int map_delete_elem(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_WRITE)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
err = PTR_ERR(key);
@@ -697,6 +748,11 @@ static int map_get_next_key(union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
+ if (!(f.file->f_mode & FMODE_CAN_READ)) {
+ err = -EPERM;
+ goto err_put;
+ }
+
if (ukey) {
key = memdup_user(ukey, map->key_size);
if (IS_ERR(key)) {
@@ -903,6 +959,8 @@ static const struct file_operations bpf_prog_fops = {
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
.release = bpf_prog_release,
+ .read = bpf_dummy_read,
+ .write = bpf_dummy_write,
};
int bpf_prog_new_fd(struct bpf_prog *prog)
@@ -1112,11 +1170,11 @@ static int bpf_prog_load(union bpf_attr *attr)
return err;
}
-#define BPF_OBJ_LAST_FIELD bpf_fd
+#define BPF_OBJ_LAST_FIELD file_flags
static int bpf_obj_pin(const union bpf_attr *attr)
{
- if (CHECK_ATTR(BPF_OBJ))
+ if (CHECK_ATTR(BPF_OBJ) || attr->file_flags != 0)
return -EINVAL;
return bpf_obj_pin_user(attr->bpf_fd, u64_to_user_ptr(attr->pathname));
@@ -1127,7 +1185,8 @@ static int bpf_obj_get(const union bpf_attr *attr)
if (CHECK_ATTR(BPF_OBJ) || attr->bpf_fd != 0)
return -EINVAL;
- return bpf_obj_get_user(u64_to_user_ptr(attr->pathname));
+ return bpf_obj_get_user(u64_to_user_ptr(attr->pathname),
+ attr->file_flags);
}
#ifdef CONFIG_CGROUP_BPF
@@ -1341,12 +1400,13 @@ static int bpf_prog_get_fd_by_id(const union bpf_attr *attr)
return fd;
}
-#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD map_id
+#define BPF_MAP_GET_FD_BY_ID_LAST_FIELD open_flags
static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
{
struct bpf_map *map;
u32 id = attr->map_id;
+ int f_flags;
int fd;
if (CHECK_ATTR(BPF_MAP_GET_FD_BY_ID))
@@ -1355,6 +1415,10 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (!capable(CAP_SYS_ADMIN))
return -EPERM;
+ f_flags = bpf_get_file_flag(attr->open_flags);
+ if (f_flags < 0)
+ return f_flags;
+
spin_lock_bh(&map_idr_lock);
map = idr_find(&map_idr, id);
if (map)
@@ -1366,7 +1430,7 @@ static int bpf_map_get_fd_by_id(const union bpf_attr *attr)
if (IS_ERR(map))
return PTR_ERR(map);
- fd = bpf_map_new_fd(map);
+ fd = bpf_map_new_fd(map, f_flags);
if (fd < 0)
bpf_map_put(map);
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 2/4] security: bpf: Add LSM hooks for bpf object related syscall
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 18:29 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov,
Daniel Borkmann, Chenbo Feng
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
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(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d826be859589..d757ea3f2228 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -57,6 +57,9 @@ struct bpf_map {
atomic_t usercnt;
struct bpf_map *inner_map_meta;
u8 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 */
u8 name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+ void *security;
+#endif
union {
struct work_struct work;
struct rcu_head rcu;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..7161d8e7ee79 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1351,6 +1351,40 @@
* @inode we wish to get the security context of.
* @ctx is a pointer in which to place the allocated security context.
* @ctxlen points to the place to put the length of @ctx.
+ *
+ * Security hooks for using the eBPF maps and programs functionalities through
+ * eBPF syscalls.
+ *
+ * @bpf:
+ * Do a initial check for all bpf syscalls after the attribute is copied
+ * into the kernel. The actual security module can implement their own
+ * rules to check the specific cmd they need.
+ *
+ * @bpf_map:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF maps.
+ *
+ * @map: bpf map that we want to access
+ * @mask: the access flags
+ *
+ * @bpf_prog:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF programs.
+ *
+ * @prog: bpf prog that userspace want to use.
+ *
+ * @bpf_map_alloc_security:
+ * Initialize the security field inside bpf map.
+ *
+ * @bpf_map_free_security:
+ * Clean up the security information stored inside bpf map.
+ *
+ * @bpf_prog_alloc_security:
+ * Initialize the security field inside bpf program.
+ *
+ * @bpf_prog_free_security:
+ * Clean up the security information stored inside bpf prog.
+ *
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1682,6 +1716,17 @@ union security_list_options {
struct audit_context *actx);
void (*audit_rule_free)(void *lsmrule);
#endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+ int (*bpf)(int cmd, union bpf_attr *attr,
+ unsigned int size);
+ int (*bpf_map)(struct bpf_map *map, fmode_t fmode);
+ int (*bpf_prog)(struct bpf_prog *prog);
+ int (*bpf_map_alloc_security)(struct bpf_map *map);
+ 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);
+#endif /* CONFIG_BPF_SYSCALL */
};
struct security_hook_heads {
@@ -1901,6 +1946,15 @@ struct security_hook_heads {
struct list_head audit_rule_match;
struct list_head audit_rule_free;
#endif /* CONFIG_AUDIT */
+#ifdef CONFIG_BPF_SYSCALL
+ struct list_head bpf;
+ struct list_head bpf_map;
+ struct list_head bpf_prog;
+ struct list_head bpf_map_alloc_security;
+ struct list_head bpf_map_free_security;
+ struct list_head bpf_prog_alloc_security;
+ struct list_head bpf_prog_free_security;
+#endif /* CONFIG_BPF_SYSCALL */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..18800b0911e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -31,6 +31,7 @@
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/fs.h>
+#include <linux/bpf.h>
struct linux_binprm;
struct cred;
@@ -1730,6 +1731,50 @@ static inline void securityfs_remove(struct dentry *dentry)
#endif
+#ifdef CONFIG_BPF_SYSCALL
+#ifdef CONFIG_SECURITY
+extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size);
+extern int security_bpf_map(struct bpf_map *map, fmode_t fmode);
+extern int security_bpf_prog(struct bpf_prog *prog);
+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);
+#else
+static inline int security_bpf(int cmd, union bpf_attr *attr,
+ unsigned int size)
+{
+ return 0;
+}
+
+static inline int security_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+ return 0;
+}
+
+static inline int security_bpf_prog(struct bpf_prog *prog)
+{
+ return 0;
+}
+
+static inline int security_bpf_map_alloc(struct bpf_map *map)
+{
+ return 0;
+}
+
+static inline void security_bpf_map_free(struct bpf_map *map)
+{ }
+
+static inline int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+ return 0;
+}
+
+static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
+{ }
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_BPF_SYSCALL */
+
#ifdef CONFIG_SECURITY
static inline char *alloc_secdata(void)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7c8a9964a41d..58ff769d58ab 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -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);
}
@@ -404,10 +408,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;
@@ -429,6 +437,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;
@@ -907,6 +917,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);
}
@@ -965,6 +976,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);
}
@@ -1104,10 +1118,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;
@@ -1165,6 +1183,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;
@@ -1585,6 +1605,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);
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..1cd8526cb0b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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 */
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 2/4] security: bpf: Add LSM hooks for bpf object related syscall
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: linux-security-module
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
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(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d826be859589..d757ea3f2228 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -57,6 +57,9 @@ struct bpf_map {
atomic_t usercnt;
struct bpf_map *inner_map_meta;
u8 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 */
u8 name[BPF_OBJ_NAME_LEN];
+#ifdef CONFIG_SECURITY
+ void *security;
+#endif
union {
struct work_struct work;
struct rcu_head rcu;
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index c9258124e417..7161d8e7ee79 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1351,6 +1351,40 @@
* @inode we wish to get the security context of.
* @ctx is a pointer in which to place the allocated security context.
* @ctxlen points to the place to put the length of @ctx.
+ *
+ * Security hooks for using the eBPF maps and programs functionalities through
+ * eBPF syscalls.
+ *
+ * @bpf:
+ * Do a initial check for all bpf syscalls after the attribute is copied
+ * into the kernel. The actual security module can implement their own
+ * rules to check the specific cmd they need.
+ *
+ * @bpf_map:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF maps.
+ *
+ * @map: bpf map that we want to access
+ * @mask: the access flags
+ *
+ * @bpf_prog:
+ * Do a check when the kernel generate and return a file descriptor for
+ * eBPF programs.
+ *
+ * @prog: bpf prog that userspace want to use.
+ *
+ * @bpf_map_alloc_security:
+ * Initialize the security field inside bpf map.
+ *
+ * @bpf_map_free_security:
+ * Clean up the security information stored inside bpf map.
+ *
+ * @bpf_prog_alloc_security:
+ * Initialize the security field inside bpf program.
+ *
+ * @bpf_prog_free_security:
+ * Clean up the security information stored inside bpf prog.
+ *
*/
union security_list_options {
int (*binder_set_context_mgr)(struct task_struct *mgr);
@@ -1682,6 +1716,17 @@ union security_list_options {
struct audit_context *actx);
void (*audit_rule_free)(void *lsmrule);
#endif /* CONFIG_AUDIT */
+
+#ifdef CONFIG_BPF_SYSCALL
+ int (*bpf)(int cmd, union bpf_attr *attr,
+ unsigned int size);
+ int (*bpf_map)(struct bpf_map *map, fmode_t fmode);
+ int (*bpf_prog)(struct bpf_prog *prog);
+ int (*bpf_map_alloc_security)(struct bpf_map *map);
+ 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);
+#endif /* CONFIG_BPF_SYSCALL */
};
struct security_hook_heads {
@@ -1901,6 +1946,15 @@ struct security_hook_heads {
struct list_head audit_rule_match;
struct list_head audit_rule_free;
#endif /* CONFIG_AUDIT */
+#ifdef CONFIG_BPF_SYSCALL
+ struct list_head bpf;
+ struct list_head bpf_map;
+ struct list_head bpf_prog;
+ struct list_head bpf_map_alloc_security;
+ struct list_head bpf_map_free_security;
+ struct list_head bpf_prog_alloc_security;
+ struct list_head bpf_prog_free_security;
+#endif /* CONFIG_BPF_SYSCALL */
} __randomize_layout;
/*
diff --git a/include/linux/security.h b/include/linux/security.h
index ce6265960d6c..18800b0911e5 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -31,6 +31,7 @@
#include <linux/string.h>
#include <linux/mm.h>
#include <linux/fs.h>
+#include <linux/bpf.h>
struct linux_binprm;
struct cred;
@@ -1730,6 +1731,50 @@ static inline void securityfs_remove(struct dentry *dentry)
#endif
+#ifdef CONFIG_BPF_SYSCALL
+#ifdef CONFIG_SECURITY
+extern int security_bpf(int cmd, union bpf_attr *attr, unsigned int size);
+extern int security_bpf_map(struct bpf_map *map, fmode_t fmode);
+extern int security_bpf_prog(struct bpf_prog *prog);
+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);
+#else
+static inline int security_bpf(int cmd, union bpf_attr *attr,
+ unsigned int size)
+{
+ return 0;
+}
+
+static inline int security_bpf_map(struct bpf_map *map, fmode_t fmode)
+{
+ return 0;
+}
+
+static inline int security_bpf_prog(struct bpf_prog *prog)
+{
+ return 0;
+}
+
+static inline int security_bpf_map_alloc(struct bpf_map *map)
+{
+ return 0;
+}
+
+static inline void security_bpf_map_free(struct bpf_map *map)
+{ }
+
+static inline int security_bpf_prog_alloc(struct bpf_prog_aux *aux)
+{
+ return 0;
+}
+
+static inline void security_bpf_prog_free(struct bpf_prog_aux *aux)
+{ }
+#endif /* CONFIG_SECURITY */
+#endif /* CONFIG_BPF_SYSCALL */
+
#ifdef CONFIG_SECURITY
static inline char *alloc_secdata(void)
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 7c8a9964a41d..58ff769d58ab 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -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);
}
@@ -404,10 +408,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;
@@ -429,6 +437,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;
@@ -907,6 +917,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);
}
@@ -965,6 +976,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);
}
@@ -1104,10 +1118,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;
@@ -1165,6 +1183,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;
@@ -1585,6 +1605,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);
diff --git a/security/security.c b/security/security.c
index 4bf0f571b4ef..1cd8526cb0b7 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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 */
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 18:29 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov,
Daniel Borkmann, Chenbo Feng
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
security/selinux/hooks.c | 111 ++++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 ++
3 files changed, 117 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..41aba4e3d57c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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_MAP, BPF_MAP__CREATE,
+ NULL);
+ break;
+ case BPF_PROG_LOAD:
+ ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG, 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 (f_mode & FMODE_READ)
+ av |= BPF_MAP__READ;
+ if (f_mode & 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_MAP,
+ 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_PROG,
+ 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)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35ffb29a69cb..7253c5eea59c 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
{ "access", NULL } },
{ "infiniband_endport",
{ "manage_subnet", NULL } },
+ { "bpf_map", {"create", "read", "write"} },
+ { "bpf_prog", {"load", "use"} },
{ NULL }
};
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 1649cd18eb0b..3d54468ce334 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -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_ */
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: linux-security-module
From: Chenbo Feng <fengc@google.com>
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 <fengc@google.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
security/selinux/hooks.c | 111 ++++++++++++++++++++++++++++++++++++
security/selinux/include/classmap.h | 2 +
security/selinux/include/objsec.h | 4 ++
3 files changed, 117 insertions(+)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index f5d304736852..41aba4e3d57c 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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_MAP, BPF_MAP__CREATE,
+ NULL);
+ break;
+ case BPF_PROG_LOAD:
+ ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG, 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 (f_mode & FMODE_READ)
+ av |= BPF_MAP__READ;
+ if (f_mode & 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_MAP,
+ 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_PROG,
+ 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)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35ffb29a69cb..7253c5eea59c 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
{ "access", NULL } },
{ "infiniband_endport",
{ "manage_subnet", NULL } },
+ { "bpf_map", {"create", "read", "write"} },
+ { "bpf_prog", {"load", "use"} },
{ NULL }
};
diff --git a/security/selinux/include/objsec.h b/security/selinux/include/objsec.h
index 1649cd18eb0b..3d54468ce334 100644
--- a/security/selinux/include/objsec.h
+++ b/security/selinux/include/objsec.h
@@ -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_ */
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 18:29 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov,
Daniel Borkmann, Chenbo Feng
From: Chenbo Feng <fengc@google.com>
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.
Signed-off-by: Chenbo Feng <fengc@google.com>
---
include/linux/bpf.h | 3 +++
kernel/bpf/syscall.c | 4 ++--
security/selinux/hooks.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d757ea3f2228..ac8428a36d56 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
#ifdef CONFIG_BPF_SYSCALL
DECLARE_PER_CPU(int, bpf_prog_active);
+extern const struct file_operations bpf_map_fops;
+extern const struct file_operations bpf_prog_fops;
+
#define BPF_PROG_TYPE(_id, _ops) \
extern const struct bpf_verifier_ops _ops;
#define BPF_MAP_TYPE(_id, _ops) \
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 58ff769d58ab..5789a5359f0a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
return -EINVAL;
}
-static const struct file_operations bpf_map_fops = {
+const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
@@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
-static const struct file_operations bpf_prog_fops = {
+const struct file_operations bpf_prog_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 41aba4e3d57c..381474ce3216 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred *cred,
/* av is zero if only checking access to the descriptor. */
rc = 0;
+
if (av)
rc = inode_has_perm(cred, inode, av, &ad);
@@ -2142,6 +2143,10 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
NULL);
}
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_fd_pass(struct file *file, u32 sid);
+#endif
+
static int selinux_binder_transfer_file(struct task_struct *from,
struct task_struct *to,
struct file *file)
@@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct task_struct *from,
return rc;
}
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, sid);
+ if (rc)
+ return rc;
+#endif
+
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
@@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
static int selinux_file_receive(struct file *file)
{
const struct cred *cred = current_cred();
+ int rc;
+
+ rc = file_has_perm(cred, file, file_to_av(file));
+ if (rc)
+ goto out;
+
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, cred_sid(sid));
+#endif
- return file_has_perm(cred, file, file_to_av(file));
+out:
+ return rc;
}
static int selinux_file_open(struct file *file, const struct cred *cred)
@@ -6288,6 +6309,40 @@ 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.
+ */
+static int bpf_fd_pass(struct file *file, u32 sid)
+{
+ struct bpf_security_struct *bpfsec;
+ u32 sid = cred_sid(cred);
+ struct bpf_prog *prog;
+ struct bpf_map *map;
+ int ret;
+
+ if (file->f_op == &bpf_map_fops) {
+ map = file->private_data;
+ bpfsec = map->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
+ bpf_map_fmode_to_av(file->f_mode), NULL);
+ if (ret)
+ return ret;
+ } else if (file->f_op == &bpf_prog_fops) {
+ prog = file->private_data;
+ bpfsec = prog->aux->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
+ 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();
--
2.14.2.920.gcf0c67979c-goog
^ permalink raw reply related [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-04 18:29 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 18:29 UTC (permalink / raw)
To: linux-security-module
From: Chenbo Feng <fengc@google.com>
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.
Signed-off-by: Chenbo Feng <fengc@google.com>
---
include/linux/bpf.h | 3 +++
kernel/bpf/syscall.c | 4 ++--
security/selinux/hooks.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-
3 files changed, 61 insertions(+), 3 deletions(-)
diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index d757ea3f2228..ac8428a36d56 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog, const union bpf_attr *kattr,
#ifdef CONFIG_BPF_SYSCALL
DECLARE_PER_CPU(int, bpf_prog_active);
+extern const struct file_operations bpf_map_fops;
+extern const struct file_operations bpf_prog_fops;
+
#define BPF_PROG_TYPE(_id, _ops) \
extern const struct bpf_verifier_ops _ops;
#define BPF_MAP_TYPE(_id, _ops) \
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index 58ff769d58ab..5789a5359f0a 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp, const char __user *buf,
return -EINVAL;
}
-static const struct file_operations bpf_map_fops = {
+const struct file_operations bpf_map_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_map_show_fdinfo,
#endif
@@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file *m, struct file *filp)
}
#endif
-static const struct file_operations bpf_prog_fops = {
+const struct file_operations bpf_prog_fops = {
#ifdef CONFIG_PROC_FS
.show_fdinfo = bpf_prog_show_fdinfo,
#endif
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 41aba4e3d57c..381474ce3216 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred *cred,
/* av is zero if only checking access to the descriptor. */
rc = 0;
+
if (av)
rc = inode_has_perm(cred, inode, av, &ad);
@@ -2142,6 +2143,10 @@ static int selinux_binder_transfer_binder(struct task_struct *from,
NULL);
}
+#ifdef CONFIG_BPF_SYSCALL
+static int bpf_fd_pass(struct file *file, u32 sid);
+#endif
+
static int selinux_binder_transfer_file(struct task_struct *from,
struct task_struct *to,
struct file *file)
@@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct task_struct *from,
return rc;
}
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, sid);
+ if (rc)
+ return rc;
+#endif
+
if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
return 0;
@@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct task_struct *tsk,
static int selinux_file_receive(struct file *file)
{
const struct cred *cred = current_cred();
+ int rc;
+
+ rc = file_has_perm(cred, file, file_to_av(file));
+ if (rc)
+ goto out;
+
+#ifdef CONFIG_BPF_SYSCALL
+ rc = bpf_fd_pass(file, cred_sid(sid));
+#endif
- return file_has_perm(cred, file, file_to_av(file));
+out:
+ return rc;
}
static int selinux_file_open(struct file *file, const struct cred *cred)
@@ -6288,6 +6309,40 @@ 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.
+ */
+static int bpf_fd_pass(struct file *file, u32 sid)
+{
+ struct bpf_security_struct *bpfsec;
+ u32 sid = cred_sid(cred);
+ struct bpf_prog *prog;
+ struct bpf_map *map;
+ int ret;
+
+ if (file->f_op == &bpf_map_fops) {
+ map = file->private_data;
+ bpfsec = map->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_MAP,
+ bpf_map_fmode_to_av(file->f_mode), NULL);
+ if (ret)
+ return ret;
+ } else if (file->f_op == &bpf_prog_fops) {
+ prog = file->private_data;
+ bpfsec = prog->aux->security;
+ ret = avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
+ 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();
--
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
^ permalink raw reply related [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 23:29 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:29 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov, Chenbo Feng
On 10/04/2017 08:29 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
[...]
> +int bpf_get_file_flag(int flags)
> +{
> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
> + return -EINVAL;
> + if (flags & BPF_F_RDONLY)
> + return O_RDONLY;
> + if (flags & BPF_F_WRONLY)
> + return O_WRONLY;
> + return O_RDWR;
> }
>
> /* helper macro to check that unused fields 'union bpf_attr' are zero */
> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
> {
> int numa_node = bpf_map_attr_numa_node(attr);
> struct bpf_map *map;
> + int f_flags;
> int err;
>
> err = CHECK_ATTR(BPF_MAP_CREATE);
> if (err)
> return -EINVAL;
>
> + f_flags = bpf_get_file_flag(attr->map_flags);
> + if (f_flags < 0)
> + return f_flags;
Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
to attr->map_flags, and later go into find_and_alloc_map(),
for map alloc, which is e.g. array_map_alloc(). There, we
bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
would have expected that the entire code was tested properly;
what was tested exactly in the set?
> if (numa_node != NUMA_NO_NODE &&
> ((unsigned int)numa_node >= nr_node_ids ||
> !node_online(numa_node)))
> @@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
> if (err)
> goto free_map;
>
> - err = bpf_map_new_fd(map);
> + err = bpf_map_new_fd(map, f_flags);
> if (err < 0) {
> /* failed to allocate fd.
> * bpf_map_put() is needed because the above
> @@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
[...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
@ 2017-10-04 23:29 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:29 UTC (permalink / raw)
To: linux-security-module
On 10/04/2017 08:29 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
[...]
> +int bpf_get_file_flag(int flags)
> +{
> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
> + return -EINVAL;
> + if (flags & BPF_F_RDONLY)
> + return O_RDONLY;
> + if (flags & BPF_F_WRONLY)
> + return O_WRONLY;
> + return O_RDWR;
> }
>
> /* helper macro to check that unused fields 'union bpf_attr' are zero */
> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
> {
> int numa_node = bpf_map_attr_numa_node(attr);
> struct bpf_map *map;
> + int f_flags;
> int err;
>
> err = CHECK_ATTR(BPF_MAP_CREATE);
> if (err)
> return -EINVAL;
>
> + f_flags = bpf_get_file_flag(attr->map_flags);
> + if (f_flags < 0)
> + return f_flags;
Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
to attr->map_flags, and later go into find_and_alloc_map(),
for map alloc, which is e.g. array_map_alloc(). There, we
bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
would have expected that the entire code was tested properly;
what was tested exactly in the set?
> if (numa_node != NUMA_NO_NODE &&
> ((unsigned int)numa_node >= nr_node_ids ||
> !node_online(numa_node)))
> @@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
> if (err)
> goto free_map;
>
> - err = bpf_map_new_fd(map);
> + err = bpf_map_new_fd(map, f_flags);
> if (err < 0) {
> /* failed to allocate fd.
> * bpf_map_put() is needed because the above
> @@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
[...]
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-04 23:44 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:44 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov, Chenbo Feng
On 10/04/2017 08:29 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
[...]
> +/* 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.
> + */
[...]
If selinux/lsm folks have some input on this one in particular, would
be great. The issue is not really tied to bpf specifically, but to the
use of anon-inodes wrt fd passing. Maybe some generic resolution can
be found to tackle this ...
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-04 23:44 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:44 UTC (permalink / raw)
To: linux-security-module
On 10/04/2017 08:29 PM, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
[...]
> +/* 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.
> + */
[...]
If selinux/lsm folks have some input on this one in particular, would
be great. The issue is not really tied to bpf specifically, but to the
use of anon-inodes wrt fd passing. Maybe some generic resolution can
be found to tackle this ...
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-04 23:44 ` Daniel Borkmann
@ 2017-10-04 23:52 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:52 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov, Chenbo Feng
On 10/05/2017 01:44 AM, Daniel Borkmann wrote:
> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>
> [...]
>> +/* 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.
>> + */
> [...]
>
> If selinux/lsm folks have some input on this one in particular, would
> be great. The issue is not really tied to bpf specifically, but to the
> use of anon-inodes wrt fd passing. Maybe some generic resolution can
> be found to tackle this ...
Perhaps even just a generic callback in struct file_operations might be
better in order to just retrieve the secctx from the underlying object
in case of anon-inodes and then have a generic check in selinux_file_receive()
for the case when such callback is set, such that we don't need to put
specific bpf logic there.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-04 23:52 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-04 23:52 UTC (permalink / raw)
To: linux-security-module
On 10/05/2017 01:44 AM, Daniel Borkmann wrote:
> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>
> [...]
>> +/* 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.
>> + */
> [...]
>
> If selinux/lsm folks have some input on this one in particular, would
> be great. The issue is not really tied to bpf specifically, but to the
> use of anon-inodes wrt fd passing. Maybe some generic resolution can
> be found to tackle this ...
Perhaps even just a generic callback in struct file_operations might be
better in order to just retrieve the secctx from the underlying object
in case of anon-inodes and then have a generic check in selinux_file_receive()
for the case when such callback is set, such that we don't need to put
specific bpf logic there.
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
2017-10-04 23:29 ` Daniel Borkmann
@ 2017-10-04 23:58 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 23:58 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Chenbo Feng, netdev, SELinux, linux-security-module,
Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov
On Wed, Oct 4, 2017 at 4:29 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>>
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>
> [...]
>>
>> +int bpf_get_file_flag(int flags)
>> +{
>> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
>> + return -EINVAL;
>> + if (flags & BPF_F_RDONLY)
>> + return O_RDONLY;
>> + if (flags & BPF_F_WRONLY)
>> + return O_WRONLY;
>> + return O_RDWR;
>> }
>>
>> /* helper macro to check that unused fields 'union bpf_attr' are zero */
>> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
>> {
>> int numa_node = bpf_map_attr_numa_node(attr);
>> struct bpf_map *map;
>> + int f_flags;
>> int err;
>>
>> err = CHECK_ATTR(BPF_MAP_CREATE);
>> if (err)
>> return -EINVAL;
>>
>> + f_flags = bpf_get_file_flag(attr->map_flags);
>> + if (f_flags < 0)
>> + return f_flags;
>
>
> Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
> to attr->map_flags, and later go into find_and_alloc_map(),
> for map alloc, which is e.g. array_map_alloc(). There, we
> bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
> which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
> would have expected that the entire code was tested properly;
> what was tested exactly in the set?
>
Thanks for pointing out this, my test for the patch create the map
with RD/WR flag which is 0.... that's why I didn't catch this. And
bpf_obj_get do not have similar checks for map_flags.
>> if (numa_node != NUMA_NO_NODE &&
>> ((unsigned int)numa_node >= nr_node_ids ||
>> !node_online(numa_node)))
>> @@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
>> if (err)
>> goto free_map;
>>
>> - err = bpf_map_new_fd(map);
>> + err = bpf_map_new_fd(map, f_flags);
>> if (err < 0) {
>> /* failed to allocate fd.
>> * bpf_map_put() is needed because the above
>> @@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
>
> [...]
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
@ 2017-10-04 23:58 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-04 23:58 UTC (permalink / raw)
To: linux-security-module
On Wed, Oct 4, 2017 at 4:29 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>>
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>
> [...]
>>
>> +int bpf_get_file_flag(int flags)
>> +{
>> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
>> + return -EINVAL;
>> + if (flags & BPF_F_RDONLY)
>> + return O_RDONLY;
>> + if (flags & BPF_F_WRONLY)
>> + return O_WRONLY;
>> + return O_RDWR;
>> }
>>
>> /* helper macro to check that unused fields 'union bpf_attr' are zero */
>> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
>> {
>> int numa_node = bpf_map_attr_numa_node(attr);
>> struct bpf_map *map;
>> + int f_flags;
>> int err;
>>
>> err = CHECK_ATTR(BPF_MAP_CREATE);
>> if (err)
>> return -EINVAL;
>>
>> + f_flags = bpf_get_file_flag(attr->map_flags);
>> + if (f_flags < 0)
>> + return f_flags;
>
>
> Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
> to attr->map_flags, and later go into find_and_alloc_map(),
> for map alloc, which is e.g. array_map_alloc(). There, we
> bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
> which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
> would have expected that the entire code was tested properly;
> what was tested exactly in the set?
>
Thanks for pointing out this, my test for the patch create the map
with RD/WR flag which is 0.... that's why I didn't catch this. And
bpf_obj_get do not have similar checks for map_flags.
>> if (numa_node != NUMA_NO_NODE &&
>> ((unsigned int)numa_node >= nr_node_ids ||
>> !node_online(numa_node)))
>> @@ -376,7 +412,7 @@ static int map_create(union bpf_attr *attr)
>> if (err)
>> goto free_map;
>>
>> - err = bpf_map_new_fd(map);
>> + err = bpf_map_new_fd(map, f_flags);
>> if (err < 0) {
>> /* failed to allocate fd.
>> * bpf_map_put() is needed because the above
>> @@ -491,6 +527,11 @@ static int map_lookup_elem(union bpf_attr *attr)
>
> [...]
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
2017-10-04 23:58 ` Chenbo Feng
@ 2017-10-05 0:51 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-05 0:51 UTC (permalink / raw)
To: Chenbo Feng
Cc: Chenbo Feng, netdev, SELinux, linux-security-module,
Jeffrey Vander Stoep, Lorenzo Colitti, Alexei Starovoitov
On 10/05/2017 01:58 AM, Chenbo Feng wrote:
> On Wed, Oct 4, 2017 at 4:29 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>>> From: Chenbo Feng <fengc@google.com>
>>>
>>> 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.
>>
>> [...]
>>>
>>> +int bpf_get_file_flag(int flags)
>>> +{
>>> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
>>> + return -EINVAL;
>>> + if (flags & BPF_F_RDONLY)
>>> + return O_RDONLY;
>>> + if (flags & BPF_F_WRONLY)
>>> + return O_WRONLY;
>>> + return O_RDWR;
>>> }
>>>
>>> /* helper macro to check that unused fields 'union bpf_attr' are zero */
>>> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
>>> {
>>> int numa_node = bpf_map_attr_numa_node(attr);
>>> struct bpf_map *map;
>>> + int f_flags;
>>> int err;
>>>
>>> err = CHECK_ATTR(BPF_MAP_CREATE);
>>> if (err)
>>> return -EINVAL;
>>>
>>> + f_flags = bpf_get_file_flag(attr->map_flags);
>>> + if (f_flags < 0)
>>> + return f_flags;
>>
>> Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
>> to attr->map_flags, and later go into find_and_alloc_map(),
>> for map alloc, which is e.g. array_map_alloc(). There, we
>> bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
>> which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
>> would have expected that the entire code was tested properly;
>> what was tested exactly in the set?
>
> Thanks for pointing out this, my test for the patch create the map
> with RD/WR flag which is 0.... that's why I didn't catch this. And
> bpf_obj_get do not have similar checks for map_flags.
Ok, please make sure to extend tools/testing/selftests/bpf/test_maps.c
regarding the two added flags, should be really straight forward to
integrate there and it would also help tracking potential regressions
in future as it's run by various ci bots (like 0day bot).
Thanks,
Daniel
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps
@ 2017-10-05 0:51 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-05 0:51 UTC (permalink / raw)
To: linux-security-module
On 10/05/2017 01:58 AM, Chenbo Feng wrote:
> On Wed, Oct 4, 2017 at 4:29 PM, Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 10/04/2017 08:29 PM, Chenbo Feng wrote:
>>> From: Chenbo Feng <fengc@google.com>
>>>
>>> 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.
>>
>> [...]
>>>
>>> +int bpf_get_file_flag(int flags)
>>> +{
>>> + if ((flags & BPF_F_RDONLY) && (flags & BPF_F_WRONLY))
>>> + return -EINVAL;
>>> + if (flags & BPF_F_RDONLY)
>>> + return O_RDONLY;
>>> + if (flags & BPF_F_WRONLY)
>>> + return O_WRONLY;
>>> + return O_RDWR;
>>> }
>>>
>>> /* helper macro to check that unused fields 'union bpf_attr' are zero */
>>> @@ -345,12 +376,17 @@ static int map_create(union bpf_attr *attr)
>>> {
>>> int numa_node = bpf_map_attr_numa_node(attr);
>>> struct bpf_map *map;
>>> + int f_flags;
>>> int err;
>>>
>>> err = CHECK_ATTR(BPF_MAP_CREATE);
>>> if (err)
>>> return -EINVAL;
>>>
>>> + f_flags = bpf_get_file_flag(attr->map_flags);
>>> + if (f_flags < 0)
>>> + return f_flags;
>>
>> Wait, I just noticed, given you add BPF_F_RDONLY/BPF_F_WRONLY
>> to attr->map_flags, and later go into find_and_alloc_map(),
>> for map alloc, which is e.g. array_map_alloc(). There, we
>> bail out with EINVAL on attr->map_flags & ~BPF_F_NUMA_NODE,
>> which is the case for both BPF_F_RDONLY/BPF_F_WRONLY ... I
>> would have expected that the entire code was tested properly;
>> what was tested exactly in the set?
>
> Thanks for pointing out this, my test for the patch create the map
> with RD/WR flag which is 0.... that's why I didn't catch this. And
> bpf_obj_get do not have similar checks for map_flags.
Ok, please make sure to extend tools/testing/selftests/bpf/test_maps.c
regarding the two added flags, should be really straight forward to
integrate there and it would also help tracking potential regressions
in future as it's run by various ci bots (like 0day bot).
Thanks,
Daniel
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-05 13:28 ` Stephen Smalley
-1 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 13:28 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Chenbo Feng, Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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 <fengc@google.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
> security/selinux/hooks.c | 111
> ++++++++++++++++++++++++++++++++++++
> security/selinux/include/classmap.h | 2 +
> security/selinux/include/objsec.h | 4 ++
> 3 files changed, 117 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..41aba4e3d57c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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_MAP,
> BPF_MAP__CREATE,
> + NULL);
> + break;
> + case BPF_PROG_LOAD:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> 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 (f_mode & FMODE_READ)
> + av |= BPF_MAP__READ;
> + if (f_mode & 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_MAP,
> + 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;
I haven't looked closely at the bpf code, but is it guaranteed that
prog->aux cannot be NULL here? What's the difference in lifecycle for
bpf_prog vs bpf_prog_aux? Could the aux field be shared across progs
created by different processes?
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> + 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)
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 35ffb29a69cb..7253c5eea59c 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
> { "access", NULL } },
> { "infiniband_endport",
> { "manage_subnet", NULL } },
> + { "bpf_map", {"create", "read", "write"} },
> + { "bpf_prog", {"load", "use"} },
Alternatively, assuming that one usually allows access to bpf_map and
bpf_prog together, these could be coalesced into a single class and
only distinguish by permission, e.g.
{ "bpf", { "create_map", "read_map", "write_map", "prog_load",
"prog_use" } },
and then allow A self:bpf { create_map read_map write_map prog_load
prog_use }; would be stored in a single policy avtab rule, and be
cached in a single AVC entry.
> { NULL }
> };
>
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 1649cd18eb0b..3d54468ce334 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -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_ */
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
@ 2017-10-05 13:28 ` Stephen Smalley
0 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 13:28 UTC (permalink / raw)
To: linux-security-module
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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 <fengc@google.com>
> Acked-by: Alexei Starovoitov <ast@kernel.org>
> ---
> ?security/selinux/hooks.c????????????| 111
> ++++++++++++++++++++++++++++++++++++
> ?security/selinux/include/classmap.h |???2 +
> ?security/selinux/include/objsec.h???|???4 ++
> ?3 files changed, 117 insertions(+)
>
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index f5d304736852..41aba4e3d57c 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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_MAP,
> BPF_MAP__CREATE,
> + ???NULL);
> + break;
> + case BPF_PROG_LOAD:
> + ret = avc_has_perm(sid, sid, SECCLASS_BPF_PROG,
> 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 (f_mode & FMODE_READ)
> + av |= BPF_MAP__READ;
> + if (f_mode & 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_MAP,
> + ????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;
I haven't looked closely at the bpf code, but is it guaranteed that
prog->aux cannot be NULL here? What's the difference in lifecycle for
bpf_prog vs bpf_prog_aux? Could the aux field be shared across progs
created by different processes?
> + return avc_has_perm(sid, bpfsec->sid, SECCLASS_BPF_PROG,
> + ????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)
> diff --git a/security/selinux/include/classmap.h
> b/security/selinux/include/classmap.h
> index 35ffb29a69cb..7253c5eea59c 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -237,6 +237,8 @@ struct security_class_mapping secclass_map[] = {
> ? ??{ "access", NULL } },
> ? { "infiniband_endport",
> ? ??{ "manage_subnet", NULL } },
> + { "bpf_map", {"create", "read", "write"} },
> + { "bpf_prog", {"load", "use"} },
Alternatively, assuming that one usually allows access to bpf_map and
bpf_prog together, these could be coalesced into a single class and
only distinguish by permission, e.g.
{ "bpf", { "create_map", "read_map", "write_map", "prog_load",
"prog_use" } },
and then allow A self:bpf { create_map read_map write_map prog_load
prog_use }; would be stored in a single policy avtab rule, and be
cached in a single AVC entry.
> ? { NULL }
> ???};
> ?
> diff --git a/security/selinux/include/objsec.h
> b/security/selinux/include/objsec.h
> index 1649cd18eb0b..3d54468ce334 100644
> --- a/security/selinux/include/objsec.h
> +++ b/security/selinux/include/objsec.h
> @@ -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_ */
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-05 13:37 ` Stephen Smalley
-1 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 13:37 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Chenbo Feng, Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
> ---
> include/linux/bpf.h | 3 +++
> kernel/bpf/syscall.c | 4 ++--
> security/selinux/hooks.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++-
> 3 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d757ea3f2228..ac8428a36d56 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> #ifdef CONFIG_BPF_SYSCALL
> DECLARE_PER_CPU(int, bpf_prog_active);
>
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> #define BPF_PROG_TYPE(_id, _ops) \
> extern const struct bpf_verifier_ops _ops;
> #define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 58ff769d58ab..5789a5359f0a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> return -EINVAL;
> }
>
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_map_show_fdinfo,
> #endif
> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> }
> #endif
>
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> #ifdef CONFIG_PROC_FS
> .show_fdinfo = bpf_prog_show_fdinfo,
> #endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 41aba4e3d57c..381474ce3216 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> *cred,
>
> /* av is zero if only checking access to the descriptor. */
> rc = 0;
> +
> if (av)
> rc = inode_has_perm(cred, inode, av, &ad);
>
> @@ -2142,6 +2143,10 @@ static int
> selinux_binder_transfer_binder(struct task_struct *from,
> NULL);
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> static int selinux_binder_transfer_file(struct task_struct *from,
> struct task_struct *to,
> struct file *file)
> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> return rc;
> }
>
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> return 0;
>
> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
> task_struct *tsk,
> static int selinux_file_receive(struct file *file)
> {
> const struct cred *cred = current_cred();
> + int rc;
> +
> + rc = file_has_perm(cred, file, file_to_av(file));
> + if (rc)
> + goto out;
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(sid));
> +#endif
>
> - return file_has_perm(cred, file, file_to_av(file));
> +out:
> + return rc;
> }
>
> static int selinux_file_open(struct file *file, const struct cred
> *cred)
> @@ -6288,6 +6309,40 @@ 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.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + u32 sid = cred_sid(cred);
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_MAP,
> + bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (file->f_op == &bpf_prog_fops) {
> + prog = file->private_data;
> + bpfsec = prog->aux->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_PROG,
> + BPF_PROG__USE, NULL);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
When the struct file is allocated for the bpf map and/or prog, you
could call a hook at that time passing both, and note the fact that it
is a bpf map/prog in the file_security_struct. Then, on
file_receive/binder_transfer_file, you could apply the appropriate
checking. Further, if we know that the file is always allocated at the
same point as the bpf map/prog, then they should have the same SID (i.e
fsec->sid should be the same as bpfsec->sid), so we shouldn't even need
to dereference the bpf map/prog. Unless I'm missing something.
Also, are we concerned about doing the same in
flush_unauthorized_files(), for inheriting descriptors across a
context-changing execve? Should this checking actually go into
file_has_perm() itself so it is always applied on any use of the struct
file?
Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
FD__USE in the case where sid == fsec->sid, for example.
> +
> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> {
> u32 sid = current_sid();
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-05 13:37 ` Stephen Smalley
0 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 13:37 UTC (permalink / raw)
To: linux-security-module
On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> From: Chenbo Feng <fengc@google.com>
>
> 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.
>
> Signed-off-by: Chenbo Feng <fengc@google.com>
> ---
> ?include/linux/bpf.h??????|??3 +++
> ?kernel/bpf/syscall.c?????|??4 ++--
> ?security/selinux/hooks.c | 57
> +++++++++++++++++++++++++++++++++++++++++++++++-
> ?3 files changed, 61 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> index d757ea3f2228..ac8428a36d56 100644
> --- a/include/linux/bpf.h
> +++ b/include/linux/bpf.h
> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
> const union bpf_attr *kattr,
> ?#ifdef CONFIG_BPF_SYSCALL
> ?DECLARE_PER_CPU(int, bpf_prog_active);
> ?
> +extern const struct file_operations bpf_map_fops;
> +extern const struct file_operations bpf_prog_fops;
> +
> ?#define BPF_PROG_TYPE(_id, _ops) \
> ? extern const struct bpf_verifier_ops _ops;
> ?#define BPF_MAP_TYPE(_id, _ops) \
> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> index 58ff769d58ab..5789a5359f0a 100644
> --- a/kernel/bpf/syscall.c
> +++ b/kernel/bpf/syscall.c
> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
> const char __user *buf,
> ? return -EINVAL;
> ?}
> ?
> -static const struct file_operations bpf_map_fops = {
> +const struct file_operations bpf_map_fops = {
> ?#ifdef CONFIG_PROC_FS
> ? .show_fdinfo = bpf_map_show_fdinfo,
> ?#endif
> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
> *m, struct file *filp)
> ?}
> ?#endif
> ?
> -static const struct file_operations bpf_prog_fops = {
> +const struct file_operations bpf_prog_fops = {
> ?#ifdef CONFIG_PROC_FS
> ? .show_fdinfo = bpf_prog_show_fdinfo,
> ?#endif
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 41aba4e3d57c..381474ce3216 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> *cred,
> ?
> ? /* av is zero if only checking access to the descriptor. */
> ? rc = 0;
> +
> ? if (av)
> ? rc = inode_has_perm(cred, inode, av, &ad);
> ?
> @@ -2142,6 +2143,10 @@ static int
> selinux_binder_transfer_binder(struct task_struct *from,
> ? ????NULL);
> ?}
> ?
> +#ifdef CONFIG_BPF_SYSCALL
> +static int bpf_fd_pass(struct file *file, u32 sid);
> +#endif
> +
> ?static int selinux_binder_transfer_file(struct task_struct *from,
> ? struct task_struct *to,
> ? struct file *file)
> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
> task_struct *from,
> ? return rc;
> ? }
> ?
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, sid);
> + if (rc)
> + return rc;
> +#endif
> +
> ? if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> ? return 0;
> ?
> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
> task_struct *tsk,
> ?static int selinux_file_receive(struct file *file)
> ?{
> ? const struct cred *cred = current_cred();
> + int rc;
> +
> + rc = file_has_perm(cred, file, file_to_av(file));
> + if (rc)
> + goto out;
> +
> +#ifdef CONFIG_BPF_SYSCALL
> + rc = bpf_fd_pass(file, cred_sid(sid));
> +#endif
> ?
> - return file_has_perm(cred, file, file_to_av(file));
> +out:
> + return rc;
> ?}
> ?
> ?static int selinux_file_open(struct file *file, const struct cred
> *cred)
> @@ -6288,6 +6309,40 @@ 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.
> + */
> +static int bpf_fd_pass(struct file *file, u32 sid)
> +{
> + struct bpf_security_struct *bpfsec;
> + u32 sid = cred_sid(cred);
> + struct bpf_prog *prog;
> + struct bpf_map *map;
> + int ret;
> +
> + if (file->f_op == &bpf_map_fops) {
> + map = file->private_data;
> + bpfsec = map->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_MAP,
> + ???bpf_map_fmode_to_av(file-
> >f_mode), NULL);
> + if (ret)
> + return ret;
> + } else if (file->f_op == &bpf_prog_fops) {
> + prog = file->private_data;
> + bpfsec = prog->aux->security;
> + ret = avc_has_perm(sid, bpfsec->sid,
> SECCLASS_BPF_PROG,
> + ???BPF_PROG__USE, NULL);
> + if (ret)
> + return ret;
> + }
> + return 0;
> +}
When the struct file is allocated for the bpf map and/or prog, you
could call a hook at that time passing both, and note the fact that it
is a bpf map/prog in the file_security_struct. Then, on
file_receive/binder_transfer_file, you could apply the appropriate
checking. Further, if we know that the file is always allocated at the
same point as the bpf map/prog, then they should have the same SID (i.e
fsec->sid should be the same as bpfsec->sid), so we shouldn't even need
to dereference the bpf map/prog. Unless I'm missing something.
Also, are we concerned about doing the same in
flush_unauthorized_files(), for inheriting descriptors across a
context-changing execve? Should this checking actually go into
file_has_perm() itself so it is always applied on any use of the struct
file?
Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
FD__USE in the case where sid == fsec->sid, for example.
> +
> ?static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> ?{
> ? u32 sid = current_sid();
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
2017-10-05 13:28 ` Stephen Smalley
@ 2017-10-05 16:12 ` Daniel Borkmann
-1 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-05 16:12 UTC (permalink / raw)
To: Stephen Smalley, Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Chenbo Feng, Alexei Starovoitov, Lorenzo Colitti
On 10/05/2017 03:28 PM, Stephen Smalley wrote:
[...]
>> +static int selinux_bpf_prog(struct bpf_prog *prog)
>> +{
>> + u32 sid = current_sid();
>> + struct bpf_security_struct *bpfsec;
>> +
>> + bpfsec = prog->aux->security;
>
> I haven't looked closely at the bpf code, but is it guaranteed that
> prog->aux cannot be NULL here? What's the difference in lifecycle for
> bpf_prog vs bpf_prog_aux? Could the aux field be shared across progs
> created by different processes?
prog->aux cannot be NULL here, its lifetime is 1:1 with prog,
it holds slow-path auxiliary data unlike prog itself which is
additionally set to read-only after initial setup until teardown;
aux is also never shared across BPF progs, so always 1:1 relation
to prog itself.
Things that can be shared across multiple BPF programs and user
space processes are BPF maps themselves. From user space side
they can be passed via fd or pinned/retrieved from bpf fs, so
as currently implemented the void *security member you'd hold
in struct bpf_map would refer to the initial creator process of
the map here that doesn't strictly need to be alive anymore;
similarly for prog itself, when its shared between multiple
user space processes, *security ctx would point to the one that
installed it into the kernel initially.
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations
@ 2017-10-05 16:12 ` Daniel Borkmann
0 siblings, 0 replies; 34+ messages in thread
From: Daniel Borkmann @ 2017-10-05 16:12 UTC (permalink / raw)
To: linux-security-module
On 10/05/2017 03:28 PM, Stephen Smalley wrote:
[...]
>> +static int selinux_bpf_prog(struct bpf_prog *prog)
>> +{
>> + u32 sid = current_sid();
>> + struct bpf_security_struct *bpfsec;
>> +
>> + bpfsec = prog->aux->security;
>
> I haven't looked closely at the bpf code, but is it guaranteed that
> prog->aux cannot be NULL here? What's the difference in lifecycle for
> bpf_prog vs bpf_prog_aux? Could the aux field be shared across progs
> created by different processes?
prog->aux cannot be NULL here, its lifetime is 1:1 with prog,
it holds slow-path auxiliary data unlike prog itself which is
additionally set to read-only after initial setup until teardown;
aux is also never shared across BPF progs, so always 1:1 relation
to prog itself.
Things that can be shared across multiple BPF programs and user
space processes are BPF maps themselves. From user space side
they can be passed via fd or pinned/retrieved from bpf fs, so
as currently implemented the void *security member you'd hold
in struct bpf_map would refer to the initial creator process of
the map here that doesn't strictly need to be alive anymore;
similarly for prog itself, when its shared between multiple
user space processes, *security ctx would point to the one that
installed it into the kernel initially.
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-05 13:37 ` Stephen Smalley
@ 2017-10-05 18:26 ` Stephen Smalley
-1 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 18:26 UTC (permalink / raw)
To: Chenbo Feng, netdev, SELinux, linux-security-module
Cc: Chenbo Feng, Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> > From: Chenbo Feng <fengc@google.com>
> >
> > 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.
> >
> > Signed-off-by: Chenbo Feng <fengc@google.com>
> > ---
> > include/linux/bpf.h | 3 +++
> > kernel/bpf/syscall.c | 4 ++--
> > security/selinux/hooks.c | 57
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d757ea3f2228..ac8428a36d56 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog
> > *prog,
> > const union bpf_attr *kattr,
> > #ifdef CONFIG_BPF_SYSCALL
> > DECLARE_PER_CPU(int, bpf_prog_active);
> >
> > +extern const struct file_operations bpf_map_fops;
> > +extern const struct file_operations bpf_prog_fops;
> > +
> > #define BPF_PROG_TYPE(_id, _ops) \
> > extern const struct bpf_verifier_ops _ops;
> > #define BPF_MAP_TYPE(_id, _ops) \
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 58ff769d58ab..5789a5359f0a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
> > *filp,
> > const char __user *buf,
> > return -EINVAL;
> > }
> >
> > -static const struct file_operations bpf_map_fops = {
> > +const struct file_operations bpf_map_fops = {
> > #ifdef CONFIG_PROC_FS
> > .show_fdinfo = bpf_map_show_fdinfo,
> > #endif
> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct
> > seq_file
> > *m, struct file *filp)
> > }
> > #endif
> >
> > -static const struct file_operations bpf_prog_fops = {
> > +const struct file_operations bpf_prog_fops = {
> > #ifdef CONFIG_PROC_FS
> > .show_fdinfo = bpf_prog_show_fdinfo,
> > #endif
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 41aba4e3d57c..381474ce3216 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> > *cred,
> >
> > /* av is zero if only checking access to the descriptor.
> > */
> > rc = 0;
> > +
> > if (av)
> > rc = inode_has_perm(cred, inode, av, &ad);
> >
> > @@ -2142,6 +2143,10 @@ static int
> > selinux_binder_transfer_binder(struct task_struct *from,
> > NULL);
> > }
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int bpf_fd_pass(struct file *file, u32 sid);
> > +#endif
> > +
> > static int selinux_binder_transfer_file(struct task_struct *from,
> > struct task_struct *to,
> > struct file *file)
> > @@ -2165,6 +2170,12 @@ static int
> > selinux_binder_transfer_file(struct
> > task_struct *from,
> > return rc;
> > }
> >
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, sid);
> > + if (rc)
> > + return rc;
> > +#endif
> > +
> > if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > return 0;
> >
> > @@ -3735,8 +3746,18 @@ static int
> > selinux_file_send_sigiotask(struct
> > task_struct *tsk,
> > static int selinux_file_receive(struct file *file)
> > {
> > const struct cred *cred = current_cred();
> > + int rc;
> > +
> > + rc = file_has_perm(cred, file, file_to_av(file));
> > + if (rc)
> > + goto out;
> > +
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, cred_sid(sid));
> > +#endif
> >
> > - return file_has_perm(cred, file, file_to_av(file));
> > +out:
> > + return rc;
> > }
> >
> > static int selinux_file_open(struct file *file, const struct cred
> > *cred)
> > @@ -6288,6 +6309,40 @@ 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.
> > + */
> > +static int bpf_fd_pass(struct file *file, u32 sid)
> > +{
> > + struct bpf_security_struct *bpfsec;
> > + u32 sid = cred_sid(cred);
> > + struct bpf_prog *prog;
> > + struct bpf_map *map;
> > + int ret;
> > +
> > + if (file->f_op == &bpf_map_fops) {
> > + map = file->private_data;
> > + bpfsec = map->security;
> > + ret = avc_has_perm(sid, bpfsec->sid,
> > SECCLASS_BPF_MAP,
> > + bpf_map_fmode_to_av(file-
> > > f_mode), NULL);
> >
> > + if (ret)
> > + return ret;
> > + } else if (file->f_op == &bpf_prog_fops) {
> > + prog = file->private_data;
> > + bpfsec = prog->aux->security;
> > + ret = avc_has_perm(sid, bpfsec->sid,
> > SECCLASS_BPF_PROG,
> > + BPF_PROG__USE, NULL);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
>
> When the struct file is allocated for the bpf map and/or prog, you
> could call a hook at that time passing both, and note the fact that
> it
> is a bpf map/prog in the file_security_struct. Then, on
> file_receive/binder_transfer_file, you could apply the appropriate
> checking. Further, if we know that the file is always allocated at
> the
> same point as the bpf map/prog, then they should have the same SID
> (i.e
> fsec->sid should be the same as bpfsec->sid), so we shouldn't even
> need
> to dereference the bpf map/prog. Unless I'm missing something.
>
> Also, are we concerned about doing the same in
> flush_unauthorized_files(), for inheriting descriptors across a
> context-changing execve? Should this checking actually go into
> file_has_perm() itself so it is always applied on any use of the
> struct
> file?
>
> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
> FD__USE in the case where sid == fsec->sid, for example.
BTW, the prog use check seems slightly redundant in that we will
already check fd use permission. So we only really need it if you want
to allow fd use but deny prog use. The map read/write checks are more
granular than fd use, so I guess we can't get rid of those.
>
> > +
> > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> > {
> > u32 sid = current_sid();
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-05 18:26 ` Stephen Smalley
0 siblings, 0 replies; 34+ messages in thread
From: Stephen Smalley @ 2017-10-05 18:26 UTC (permalink / raw)
To: linux-security-module
On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
> > From: Chenbo Feng <fengc@google.com>
> >
> > 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.
> >
> > Signed-off-by: Chenbo Feng <fengc@google.com>
> > ---
> > ?include/linux/bpf.h??????|??3 +++
> > ?kernel/bpf/syscall.c?????|??4 ++--
> > ?security/selinux/hooks.c | 57
> > +++++++++++++++++++++++++++++++++++++++++++++++-
> > ?3 files changed, 61 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
> > index d757ea3f2228..ac8428a36d56 100644
> > --- a/include/linux/bpf.h
> > +++ b/include/linux/bpf.h
> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog
> > *prog,
> > const union bpf_attr *kattr,
> > ?#ifdef CONFIG_BPF_SYSCALL
> > ?DECLARE_PER_CPU(int, bpf_prog_active);
> > ?
> > +extern const struct file_operations bpf_map_fops;
> > +extern const struct file_operations bpf_prog_fops;
> > +
> > ?#define BPF_PROG_TYPE(_id, _ops) \
> > ? extern const struct bpf_verifier_ops _ops;
> > ?#define BPF_MAP_TYPE(_id, _ops) \
> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
> > index 58ff769d58ab..5789a5359f0a 100644
> > --- a/kernel/bpf/syscall.c
> > +++ b/kernel/bpf/syscall.c
> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
> > *filp,
> > const char __user *buf,
> > ? return -EINVAL;
> > ?}
> > ?
> > -static const struct file_operations bpf_map_fops = {
> > +const struct file_operations bpf_map_fops = {
> > ?#ifdef CONFIG_PROC_FS
> > ? .show_fdinfo = bpf_map_show_fdinfo,
> > ?#endif
> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct
> > seq_file
> > *m, struct file *filp)
> > ?}
> > ?#endif
> > ?
> > -static const struct file_operations bpf_prog_fops = {
> > +const struct file_operations bpf_prog_fops = {
> > ?#ifdef CONFIG_PROC_FS
> > ? .show_fdinfo = bpf_prog_show_fdinfo,
> > ?#endif
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 41aba4e3d57c..381474ce3216 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
> > *cred,
> > ?
> > ? /* av is zero if only checking access to the descriptor.
> > */
> > ? rc = 0;
> > +
> > ? if (av)
> > ? rc = inode_has_perm(cred, inode, av, &ad);
> > ?
> > @@ -2142,6 +2143,10 @@ static int
> > selinux_binder_transfer_binder(struct task_struct *from,
> > ? ????NULL);
> > ?}
> > ?
> > +#ifdef CONFIG_BPF_SYSCALL
> > +static int bpf_fd_pass(struct file *file, u32 sid);
> > +#endif
> > +
> > ?static int selinux_binder_transfer_file(struct task_struct *from,
> > ? struct task_struct *to,
> > ? struct file *file)
> > @@ -2165,6 +2170,12 @@ static int
> > selinux_binder_transfer_file(struct
> > task_struct *from,
> > ? return rc;
> > ? }
> > ?
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, sid);
> > + if (rc)
> > + return rc;
> > +#endif
> > +
> > ? if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
> > ? return 0;
> > ?
> > @@ -3735,8 +3746,18 @@ static int
> > selinux_file_send_sigiotask(struct
> > task_struct *tsk,
> > ?static int selinux_file_receive(struct file *file)
> > ?{
> > ? const struct cred *cred = current_cred();
> > + int rc;
> > +
> > + rc = file_has_perm(cred, file, file_to_av(file));
> > + if (rc)
> > + goto out;
> > +
> > +#ifdef CONFIG_BPF_SYSCALL
> > + rc = bpf_fd_pass(file, cred_sid(sid));
> > +#endif
> > ?
> > - return file_has_perm(cred, file, file_to_av(file));
> > +out:
> > + return rc;
> > ?}
> > ?
> > ?static int selinux_file_open(struct file *file, const struct cred
> > *cred)
> > @@ -6288,6 +6309,40 @@ 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.
> > + */
> > +static int bpf_fd_pass(struct file *file, u32 sid)
> > +{
> > + struct bpf_security_struct *bpfsec;
> > + u32 sid = cred_sid(cred);
> > + struct bpf_prog *prog;
> > + struct bpf_map *map;
> > + int ret;
> > +
> > + if (file->f_op == &bpf_map_fops) {
> > + map = file->private_data;
> > + bpfsec = map->security;
> > + ret = avc_has_perm(sid, bpfsec->sid,
> > SECCLASS_BPF_MAP,
> > + ???bpf_map_fmode_to_av(file-
> > > f_mode), NULL);
> >
> > + if (ret)
> > + return ret;
> > + } else if (file->f_op == &bpf_prog_fops) {
> > + prog = file->private_data;
> > + bpfsec = prog->aux->security;
> > + ret = avc_has_perm(sid, bpfsec->sid,
> > SECCLASS_BPF_PROG,
> > + ???BPF_PROG__USE, NULL);
> > + if (ret)
> > + return ret;
> > + }
> > + return 0;
> > +}
>
> When the struct file is allocated for the bpf map and/or prog, you
> could call a hook at that time passing both, and note the fact that
> it
> is a bpf map/prog in the file_security_struct.??Then, on
> file_receive/binder_transfer_file, you could apply the appropriate
> checking.??Further, if we know that the file is always allocated at
> the
> same point as the bpf map/prog, then they should have the same SID
> (i.e
> fsec->sid should be the same as bpfsec->sid), so we shouldn't even
> need
> to dereference the bpf map/prog.??Unless I'm missing something.
>
> Also, are we concerned about doing the same in
> flush_unauthorized_files(), for inheriting descriptors across a
> context-changing execve???Should this checking actually go into
> file_has_perm() itself so it is always applied on any use of the
> struct
> file?
>
> Lastly, do we need/want these checks if sid == bpfsec->sid???We skip
> FD__USE in the case where sid == fsec->sid, for example.
BTW, the prog use check seems slightly redundant in that we will
already check fd use permission. So we only really need it if you want
to allow fd use but deny prog use. The map read/write checks are more
granular than fd use, so I guess we can't get rid of those.
>
> > +
> > ?static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
> > ?{
> > ? u32 sid = current_sid();
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-05 13:37 ` Stephen Smalley
@ 2017-10-06 21:00 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-06 21:00 UTC (permalink / raw)
To: Stephen Smalley
Cc: Chenbo Feng, netdev, SELinux, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
On Thu, Oct 5, 2017 at 6:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>> ---
>> include/linux/bpf.h | 3 +++
>> kernel/bpf/syscall.c | 4 ++--
>> security/selinux/hooks.c | 57
>> +++++++++++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 61 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index d757ea3f2228..ac8428a36d56 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
>> const union bpf_attr *kattr,
>> #ifdef CONFIG_BPF_SYSCALL
>> DECLARE_PER_CPU(int, bpf_prog_active);
>>
>> +extern const struct file_operations bpf_map_fops;
>> +extern const struct file_operations bpf_prog_fops;
>> +
>> #define BPF_PROG_TYPE(_id, _ops) \
>> extern const struct bpf_verifier_ops _ops;
>> #define BPF_MAP_TYPE(_id, _ops) \
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 58ff769d58ab..5789a5359f0a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
>> const char __user *buf,
>> return -EINVAL;
>> }
>>
>> -static const struct file_operations bpf_map_fops = {
>> +const struct file_operations bpf_map_fops = {
>> #ifdef CONFIG_PROC_FS
>> .show_fdinfo = bpf_map_show_fdinfo,
>> #endif
>> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
>> *m, struct file *filp)
>> }
>> #endif
>>
>> -static const struct file_operations bpf_prog_fops = {
>> +const struct file_operations bpf_prog_fops = {
>> #ifdef CONFIG_PROC_FS
>> .show_fdinfo = bpf_prog_show_fdinfo,
>> #endif
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 41aba4e3d57c..381474ce3216 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
>> *cred,
>>
>> /* av is zero if only checking access to the descriptor. */
>> rc = 0;
>> +
>> if (av)
>> rc = inode_has_perm(cred, inode, av, &ad);
>>
>> @@ -2142,6 +2143,10 @@ static int
>> selinux_binder_transfer_binder(struct task_struct *from,
>> NULL);
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> +static int bpf_fd_pass(struct file *file, u32 sid);
>> +#endif
>> +
>> static int selinux_binder_transfer_file(struct task_struct *from,
>> struct task_struct *to,
>> struct file *file)
>> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
>> task_struct *from,
>> return rc;
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> + rc = bpf_fd_pass(file, sid);
>> + if (rc)
>> + return rc;
>> +#endif
>> +
>> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>> return 0;
>>
>> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
>> task_struct *tsk,
>> static int selinux_file_receive(struct file *file)
>> {
>> const struct cred *cred = current_cred();
>> + int rc;
>> +
>> + rc = file_has_perm(cred, file, file_to_av(file));
>> + if (rc)
>> + goto out;
>> +
>> +#ifdef CONFIG_BPF_SYSCALL
>> + rc = bpf_fd_pass(file, cred_sid(sid));
>> +#endif
>>
>> - return file_has_perm(cred, file, file_to_av(file));
>> +out:
>> + return rc;
>> }
>>
>> static int selinux_file_open(struct file *file, const struct cred
>> *cred)
>> @@ -6288,6 +6309,40 @@ 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.
>> + */
>> +static int bpf_fd_pass(struct file *file, u32 sid)
>> +{
>> + struct bpf_security_struct *bpfsec;
>> + u32 sid = cred_sid(cred);
>> + struct bpf_prog *prog;
>> + struct bpf_map *map;
>> + int ret;
>> +
>> + if (file->f_op == &bpf_map_fops) {
>> + map = file->private_data;
>> + bpfsec = map->security;
>> + ret = avc_has_perm(sid, bpfsec->sid,
>> SECCLASS_BPF_MAP,
>> + bpf_map_fmode_to_av(file-
>> >f_mode), NULL);
>> + if (ret)
>> + return ret;
>> + } else if (file->f_op == &bpf_prog_fops) {
>> + prog = file->private_data;
>> + bpfsec = prog->aux->security;
>> + ret = avc_has_perm(sid, bpfsec->sid,
>> SECCLASS_BPF_PROG,
>> + BPF_PROG__USE, NULL);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> +}
>
> When the struct file is allocated for the bpf map and/or prog, you
> could call a hook at that time passing both, and note the fact that it
> is a bpf map/prog in the file_security_struct. Then, on
> file_receive/binder_transfer_file, you could apply the appropriate
> checking. Further, if we know that the file is always allocated at the
> same point as the bpf map/prog, then they should have the same SID (i.e
> fsec->sid should be the same as bpfsec->sid), so we shouldn't even need
> to dereference the bpf map/prog. Unless I'm missing something.
>
Thanks for the feedback, but I am a little confused about the proposed
implementation. Do we need to add an additional field inside
file_security_struct to identify a file as bpf map/prog? Or we just
copy the sid stored inside bpf map/prog security field into
file_security_struct when we allocate the file and do checks like
following:
if (file->f_op == &bpf_map_fops) {
ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF,
bpf_map_fmode_to_av(file->f_mode), NULL);
} else if (file->f_op == &bpf_prog_fops) {
ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF,
BPF_PROG__USE, NULL);
}
> Also, are we concerned about doing the same in
> flush_unauthorized_files(), for inheriting descriptors across a
> context-changing execve? Should this checking actually go into
> file_has_perm() itself so it is always applied on any use of the struct
> file?
>
I agree moving this into file_has_perm might be a better solution.
> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
> FD__USE in the case where sid == fsec->sid, for example.
>
In this case we still have to check if the process have the right
privilege to access the map. The creator of the bpf map/prog doesn't
necessarily have all privileges to the object.
>> +
>> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> {
>> u32 sid = current_sid();
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-06 21:00 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-06 21:00 UTC (permalink / raw)
To: linux-security-module
On Thu, Oct 5, 2017 at 6:37 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
>> From: Chenbo Feng <fengc@google.com>
>>
>> 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.
>>
>> Signed-off-by: Chenbo Feng <fengc@google.com>
>> ---
>> include/linux/bpf.h | 3 +++
>> kernel/bpf/syscall.c | 4 ++--
>> security/selinux/hooks.c | 57
>> +++++++++++++++++++++++++++++++++++++++++++++++-
>> 3 files changed, 61 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> index d757ea3f2228..ac8428a36d56 100644
>> --- a/include/linux/bpf.h
>> +++ b/include/linux/bpf.h
>> @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog *prog,
>> const union bpf_attr *kattr,
>> #ifdef CONFIG_BPF_SYSCALL
>> DECLARE_PER_CPU(int, bpf_prog_active);
>>
>> +extern const struct file_operations bpf_map_fops;
>> +extern const struct file_operations bpf_prog_fops;
>> +
>> #define BPF_PROG_TYPE(_id, _ops) \
>> extern const struct bpf_verifier_ops _ops;
>> #define BPF_MAP_TYPE(_id, _ops) \
>> diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> index 58ff769d58ab..5789a5359f0a 100644
>> --- a/kernel/bpf/syscall.c
>> +++ b/kernel/bpf/syscall.c
>> @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file *filp,
>> const char __user *buf,
>> return -EINVAL;
>> }
>>
>> -static const struct file_operations bpf_map_fops = {
>> +const struct file_operations bpf_map_fops = {
>> #ifdef CONFIG_PROC_FS
>> .show_fdinfo = bpf_map_show_fdinfo,
>> #endif
>> @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct seq_file
>> *m, struct file *filp)
>> }
>> #endif
>>
>> -static const struct file_operations bpf_prog_fops = {
>> +const struct file_operations bpf_prog_fops = {
>> #ifdef CONFIG_PROC_FS
>> .show_fdinfo = bpf_prog_show_fdinfo,
>> #endif
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 41aba4e3d57c..381474ce3216 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
>> *cred,
>>
>> /* av is zero if only checking access to the descriptor. */
>> rc = 0;
>> +
>> if (av)
>> rc = inode_has_perm(cred, inode, av, &ad);
>>
>> @@ -2142,6 +2143,10 @@ static int
>> selinux_binder_transfer_binder(struct task_struct *from,
>> NULL);
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> +static int bpf_fd_pass(struct file *file, u32 sid);
>> +#endif
>> +
>> static int selinux_binder_transfer_file(struct task_struct *from,
>> struct task_struct *to,
>> struct file *file)
>> @@ -2165,6 +2170,12 @@ static int selinux_binder_transfer_file(struct
>> task_struct *from,
>> return rc;
>> }
>>
>> +#ifdef CONFIG_BPF_SYSCALL
>> + rc = bpf_fd_pass(file, sid);
>> + if (rc)
>> + return rc;
>> +#endif
>> +
>> if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>> return 0;
>>
>> @@ -3735,8 +3746,18 @@ static int selinux_file_send_sigiotask(struct
>> task_struct *tsk,
>> static int selinux_file_receive(struct file *file)
>> {
>> const struct cred *cred = current_cred();
>> + int rc;
>> +
>> + rc = file_has_perm(cred, file, file_to_av(file));
>> + if (rc)
>> + goto out;
>> +
>> +#ifdef CONFIG_BPF_SYSCALL
>> + rc = bpf_fd_pass(file, cred_sid(sid));
>> +#endif
>>
>> - return file_has_perm(cred, file, file_to_av(file));
>> +out:
>> + return rc;
>> }
>>
>> static int selinux_file_open(struct file *file, const struct cred
>> *cred)
>> @@ -6288,6 +6309,40 @@ 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.
>> + */
>> +static int bpf_fd_pass(struct file *file, u32 sid)
>> +{
>> + struct bpf_security_struct *bpfsec;
>> + u32 sid = cred_sid(cred);
>> + struct bpf_prog *prog;
>> + struct bpf_map *map;
>> + int ret;
>> +
>> + if (file->f_op == &bpf_map_fops) {
>> + map = file->private_data;
>> + bpfsec = map->security;
>> + ret = avc_has_perm(sid, bpfsec->sid,
>> SECCLASS_BPF_MAP,
>> + bpf_map_fmode_to_av(file-
>> >f_mode), NULL);
>> + if (ret)
>> + return ret;
>> + } else if (file->f_op == &bpf_prog_fops) {
>> + prog = file->private_data;
>> + bpfsec = prog->aux->security;
>> + ret = avc_has_perm(sid, bpfsec->sid,
>> SECCLASS_BPF_PROG,
>> + BPF_PROG__USE, NULL);
>> + if (ret)
>> + return ret;
>> + }
>> + return 0;
>> +}
>
> When the struct file is allocated for the bpf map and/or prog, you
> could call a hook at that time passing both, and note the fact that it
> is a bpf map/prog in the file_security_struct. Then, on
> file_receive/binder_transfer_file, you could apply the appropriate
> checking. Further, if we know that the file is always allocated at the
> same point as the bpf map/prog, then they should have the same SID (i.e
> fsec->sid should be the same as bpfsec->sid), so we shouldn't even need
> to dereference the bpf map/prog. Unless I'm missing something.
>
Thanks for the feedback, but I am a little confused about the proposed
implementation. Do we need to add an additional field inside
file_security_struct to identify a file as bpf map/prog? Or we just
copy the sid stored inside bpf map/prog security field into
file_security_struct when we allocate the file and do checks like
following:
if (file->f_op == &bpf_map_fops) {
ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF,
bpf_map_fmode_to_av(file->f_mode), NULL);
} else if (file->f_op == &bpf_prog_fops) {
ret = avc_has_perm(sid, fsec->sid, SECCLASS_BPF,
BPF_PROG__USE, NULL);
}
> Also, are we concerned about doing the same in
> flush_unauthorized_files(), for inheriting descriptors across a
> context-changing execve? Should this checking actually go into
> file_has_perm() itself so it is always applied on any use of the struct
> file?
>
I agree moving this into file_has_perm might be a better solution.
> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
> FD__USE in the case where sid == fsec->sid, for example.
>
In this case we still have to check if the process have the right
privilege to access the map. The creator of the bpf map/prog doesn't
necessarily have all privileges to the object.
>> +
>> static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> {
>> u32 sid = current_sid();
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
2017-10-05 18:26 ` Stephen Smalley
@ 2017-10-06 21:10 ` Chenbo Feng
-1 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-06 21:10 UTC (permalink / raw)
To: Stephen Smalley
Cc: Chenbo Feng, netdev, SELinux, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, Lorenzo Colitti
On Thu, Oct 5, 2017 at 11:26 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
>> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
>> > From: Chenbo Feng <fengc@google.com>
>> >
>> > 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.
>> >
>> > Signed-off-by: Chenbo Feng <fengc@google.com>
>> > ---
>> > include/linux/bpf.h | 3 +++
>> > kernel/bpf/syscall.c | 4 ++--
>> > security/selinux/hooks.c | 57
>> > +++++++++++++++++++++++++++++++++++++++++++++++-
>> > 3 files changed, 61 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> > index d757ea3f2228..ac8428a36d56 100644
>> > --- a/include/linux/bpf.h
>> > +++ b/include/linux/bpf.h
>> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog
>> > *prog,
>> > const union bpf_attr *kattr,
>> > #ifdef CONFIG_BPF_SYSCALL
>> > DECLARE_PER_CPU(int, bpf_prog_active);
>> >
>> > +extern const struct file_operations bpf_map_fops;
>> > +extern const struct file_operations bpf_prog_fops;
>> > +
>> > #define BPF_PROG_TYPE(_id, _ops) \
>> > extern const struct bpf_verifier_ops _ops;
>> > #define BPF_MAP_TYPE(_id, _ops) \
>> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> > index 58ff769d58ab..5789a5359f0a 100644
>> > --- a/kernel/bpf/syscall.c
>> > +++ b/kernel/bpf/syscall.c
>> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
>> > *filp,
>> > const char __user *buf,
>> > return -EINVAL;
>> > }
>> >
>> > -static const struct file_operations bpf_map_fops = {
>> > +const struct file_operations bpf_map_fops = {
>> > #ifdef CONFIG_PROC_FS
>> > .show_fdinfo = bpf_map_show_fdinfo,
>> > #endif
>> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct
>> > seq_file
>> > *m, struct file *filp)
>> > }
>> > #endif
>> >
>> > -static const struct file_operations bpf_prog_fops = {
>> > +const struct file_operations bpf_prog_fops = {
>> > #ifdef CONFIG_PROC_FS
>> > .show_fdinfo = bpf_prog_show_fdinfo,
>> > #endif
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index 41aba4e3d57c..381474ce3216 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
>> > *cred,
>> >
>> > /* av is zero if only checking access to the descriptor.
>> > */
>> > rc = 0;
>> > +
>> > if (av)
>> > rc = inode_has_perm(cred, inode, av, &ad);
>> >
>> > @@ -2142,6 +2143,10 @@ static int
>> > selinux_binder_transfer_binder(struct task_struct *from,
>> > NULL);
>> > }
>> >
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > +static int bpf_fd_pass(struct file *file, u32 sid);
>> > +#endif
>> > +
>> > static int selinux_binder_transfer_file(struct task_struct *from,
>> > struct task_struct *to,
>> > struct file *file)
>> > @@ -2165,6 +2170,12 @@ static int
>> > selinux_binder_transfer_file(struct
>> > task_struct *from,
>> > return rc;
>> > }
>> >
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > + rc = bpf_fd_pass(file, sid);
>> > + if (rc)
>> > + return rc;
>> > +#endif
>> > +
>> > if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>> > return 0;
>> >
>> > @@ -3735,8 +3746,18 @@ static int
>> > selinux_file_send_sigiotask(struct
>> > task_struct *tsk,
>> > static int selinux_file_receive(struct file *file)
>> > {
>> > const struct cred *cred = current_cred();
>> > + int rc;
>> > +
>> > + rc = file_has_perm(cred, file, file_to_av(file));
>> > + if (rc)
>> > + goto out;
>> > +
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > + rc = bpf_fd_pass(file, cred_sid(sid));
>> > +#endif
>> >
>> > - return file_has_perm(cred, file, file_to_av(file));
>> > +out:
>> > + return rc;
>> > }
>> >
>> > static int selinux_file_open(struct file *file, const struct cred
>> > *cred)
>> > @@ -6288,6 +6309,40 @@ 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.
>> > + */
>> > +static int bpf_fd_pass(struct file *file, u32 sid)
>> > +{
>> > + struct bpf_security_struct *bpfsec;
>> > + u32 sid = cred_sid(cred);
>> > + struct bpf_prog *prog;
>> > + struct bpf_map *map;
>> > + int ret;
>> > +
>> > + if (file->f_op == &bpf_map_fops) {
>> > + map = file->private_data;
>> > + bpfsec = map->security;
>> > + ret = avc_has_perm(sid, bpfsec->sid,
>> > SECCLASS_BPF_MAP,
>> > + bpf_map_fmode_to_av(file-
>> > > f_mode), NULL);
>> >
>> > + if (ret)
>> > + return ret;
>> > + } else if (file->f_op == &bpf_prog_fops) {
>> > + prog = file->private_data;
>> > + bpfsec = prog->aux->security;
>> > + ret = avc_has_perm(sid, bpfsec->sid,
>> > SECCLASS_BPF_PROG,
>> > + BPF_PROG__USE, NULL);
>> > + if (ret)
>> > + return ret;
>> > + }
>> > + return 0;
>> > +}
>>
>> When the struct file is allocated for the bpf map and/or prog, you
>> could call a hook at that time passing both, and note the fact that
>> it
>> is a bpf map/prog in the file_security_struct. Then, on
>> file_receive/binder_transfer_file, you could apply the appropriate
>> checking. Further, if we know that the file is always allocated at
>> the
>> same point as the bpf map/prog, then they should have the same SID
>> (i.e
>> fsec->sid should be the same as bpfsec->sid), so we shouldn't even
>> need
>> to dereference the bpf map/prog. Unless I'm missing something.
>>
>> Also, are we concerned about doing the same in
>> flush_unauthorized_files(), for inheriting descriptors across a
>> context-changing execve? Should this checking actually go into
>> file_has_perm() itself so it is always applied on any use of the
>> struct
>> file?
>>
>> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
>> FD__USE in the case where sid == fsec->sid, for example.
>
> BTW, the prog use check seems slightly redundant in that we will
> already check fd use permission. So we only really need it if you want
> to allow fd use but deny prog use. The map read/write checks are more
> granular than fd use, so I guess we can't get rid of those.
>
But it seems fd use doesn't check what object is behind that fd. So if
we don't want a process to run the eBPF program, then we still need a
additional check for it. Maybe use prog_run instead of prog_use should
be more appropriate.
>>
>> > +
>> > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> > {
>> > u32 sid = current_sid();
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive
@ 2017-10-06 21:10 ` Chenbo Feng
0 siblings, 0 replies; 34+ messages in thread
From: Chenbo Feng @ 2017-10-06 21:10 UTC (permalink / raw)
To: linux-security-module
On Thu, Oct 5, 2017 at 11:26 AM, Stephen Smalley <sds@tycho.nsa.gov> wrote:
> On Thu, 2017-10-05 at 09:37 -0400, Stephen Smalley wrote:
>> On Wed, 2017-10-04 at 11:29 -0700, Chenbo Feng wrote:
>> > From: Chenbo Feng <fengc@google.com>
>> >
>> > 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.
>> >
>> > Signed-off-by: Chenbo Feng <fengc@google.com>
>> > ---
>> > include/linux/bpf.h | 3 +++
>> > kernel/bpf/syscall.c | 4 ++--
>> > security/selinux/hooks.c | 57
>> > +++++++++++++++++++++++++++++++++++++++++++++++-
>> > 3 files changed, 61 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/include/linux/bpf.h b/include/linux/bpf.h
>> > index d757ea3f2228..ac8428a36d56 100644
>> > --- a/include/linux/bpf.h
>> > +++ b/include/linux/bpf.h
>> > @@ -250,6 +250,9 @@ int bpf_prog_test_run_skb(struct bpf_prog
>> > *prog,
>> > const union bpf_attr *kattr,
>> > #ifdef CONFIG_BPF_SYSCALL
>> > DECLARE_PER_CPU(int, bpf_prog_active);
>> >
>> > +extern const struct file_operations bpf_map_fops;
>> > +extern const struct file_operations bpf_prog_fops;
>> > +
>> > #define BPF_PROG_TYPE(_id, _ops) \
>> > extern const struct bpf_verifier_ops _ops;
>> > #define BPF_MAP_TYPE(_id, _ops) \
>> > diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
>> > index 58ff769d58ab..5789a5359f0a 100644
>> > --- a/kernel/bpf/syscall.c
>> > +++ b/kernel/bpf/syscall.c
>> > @@ -313,7 +313,7 @@ static ssize_t bpf_dummy_write(struct file
>> > *filp,
>> > const char __user *buf,
>> > return -EINVAL;
>> > }
>> >
>> > -static const struct file_operations bpf_map_fops = {
>> > +const struct file_operations bpf_map_fops = {
>> > #ifdef CONFIG_PROC_FS
>> > .show_fdinfo = bpf_map_show_fdinfo,
>> > #endif
>> > @@ -965,7 +965,7 @@ static void bpf_prog_show_fdinfo(struct
>> > seq_file
>> > *m, struct file *filp)
>> > }
>> > #endif
>> >
>> > -static const struct file_operations bpf_prog_fops = {
>> > +const struct file_operations bpf_prog_fops = {
>> > #ifdef CONFIG_PROC_FS
>> > .show_fdinfo = bpf_prog_show_fdinfo,
>> > #endif
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index 41aba4e3d57c..381474ce3216 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -1847,6 +1847,7 @@ static int file_has_perm(const struct cred
>> > *cred,
>> >
>> > /* av is zero if only checking access to the descriptor.
>> > */
>> > rc = 0;
>> > +
>> > if (av)
>> > rc = inode_has_perm(cred, inode, av, &ad);
>> >
>> > @@ -2142,6 +2143,10 @@ static int
>> > selinux_binder_transfer_binder(struct task_struct *from,
>> > NULL);
>> > }
>> >
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > +static int bpf_fd_pass(struct file *file, u32 sid);
>> > +#endif
>> > +
>> > static int selinux_binder_transfer_file(struct task_struct *from,
>> > struct task_struct *to,
>> > struct file *file)
>> > @@ -2165,6 +2170,12 @@ static int
>> > selinux_binder_transfer_file(struct
>> > task_struct *from,
>> > return rc;
>> > }
>> >
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > + rc = bpf_fd_pass(file, sid);
>> > + if (rc)
>> > + return rc;
>> > +#endif
>> > +
>> > if (unlikely(IS_PRIVATE(d_backing_inode(dentry))))
>> > return 0;
>> >
>> > @@ -3735,8 +3746,18 @@ static int
>> > selinux_file_send_sigiotask(struct
>> > task_struct *tsk,
>> > static int selinux_file_receive(struct file *file)
>> > {
>> > const struct cred *cred = current_cred();
>> > + int rc;
>> > +
>> > + rc = file_has_perm(cred, file, file_to_av(file));
>> > + if (rc)
>> > + goto out;
>> > +
>> > +#ifdef CONFIG_BPF_SYSCALL
>> > + rc = bpf_fd_pass(file, cred_sid(sid));
>> > +#endif
>> >
>> > - return file_has_perm(cred, file, file_to_av(file));
>> > +out:
>> > + return rc;
>> > }
>> >
>> > static int selinux_file_open(struct file *file, const struct cred
>> > *cred)
>> > @@ -6288,6 +6309,40 @@ 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.
>> > + */
>> > +static int bpf_fd_pass(struct file *file, u32 sid)
>> > +{
>> > + struct bpf_security_struct *bpfsec;
>> > + u32 sid = cred_sid(cred);
>> > + struct bpf_prog *prog;
>> > + struct bpf_map *map;
>> > + int ret;
>> > +
>> > + if (file->f_op == &bpf_map_fops) {
>> > + map = file->private_data;
>> > + bpfsec = map->security;
>> > + ret = avc_has_perm(sid, bpfsec->sid,
>> > SECCLASS_BPF_MAP,
>> > + bpf_map_fmode_to_av(file-
>> > > f_mode), NULL);
>> >
>> > + if (ret)
>> > + return ret;
>> > + } else if (file->f_op == &bpf_prog_fops) {
>> > + prog = file->private_data;
>> > + bpfsec = prog->aux->security;
>> > + ret = avc_has_perm(sid, bpfsec->sid,
>> > SECCLASS_BPF_PROG,
>> > + BPF_PROG__USE, NULL);
>> > + if (ret)
>> > + return ret;
>> > + }
>> > + return 0;
>> > +}
>>
>> When the struct file is allocated for the bpf map and/or prog, you
>> could call a hook at that time passing both, and note the fact that
>> it
>> is a bpf map/prog in the file_security_struct. Then, on
>> file_receive/binder_transfer_file, you could apply the appropriate
>> checking. Further, if we know that the file is always allocated at
>> the
>> same point as the bpf map/prog, then they should have the same SID
>> (i.e
>> fsec->sid should be the same as bpfsec->sid), so we shouldn't even
>> need
>> to dereference the bpf map/prog. Unless I'm missing something.
>>
>> Also, are we concerned about doing the same in
>> flush_unauthorized_files(), for inheriting descriptors across a
>> context-changing execve? Should this checking actually go into
>> file_has_perm() itself so it is always applied on any use of the
>> struct
>> file?
>>
>> Lastly, do we need/want these checks if sid == bpfsec->sid? We skip
>> FD__USE in the case where sid == fsec->sid, for example.
>
> BTW, the prog use check seems slightly redundant in that we will
> already check fd use permission. So we only really need it if you want
> to allow fd use but deny prog use. The map read/write checks are more
> granular than fd use, so I guess we can't get rid of those.
>
But it seems fd use doesn't check what object is behind that fd. So if
we don't want a process to run the eBPF program, then we still need a
additional check for it. Maybe use prog_run instead of prog_use should
be more appropriate.
>>
>> > +
>> > static int selinux_bpf_map(struct bpf_map *map, fmode_t fmode)
>> > {
>> > u32 sid = current_sid();
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
* Re: [PATCH net-next 2/4] security: bpf: Add LSM hooks for bpf object related syscall
2017-10-04 18:29 ` Chenbo Feng
@ 2017-10-12 0:31 ` James Morris
-1 siblings, 0 replies; 34+ messages in thread
From: James Morris @ 2017-10-12 0:31 UTC (permalink / raw)
To: Chenbo Feng
Cc: netdev, SELinux, linux-security-module, Jeffrey Vander Stoep,
Lorenzo Colitti, Alexei Starovoitov, Daniel Borkmann,
Chenbo Feng
On Wed, 4 Oct 2017, Chenbo Feng wrote:
> int bpf_map_new_fd(struct bpf_map *map, int flags)
> {
> + if (security_bpf_map(map, OPEN_FMODE(flags)))
> + return -EPERM;
> +
Don't hardcode -EPERM here, return the actual error from
security_bpf_map().
> + if (security_bpf_prog(prog))
> + return -EPERM;
> +
Same.
> + err = security_bpf(cmd, &attr, size);
> + if (err)
> + return -EPERM;
Same.
- James
^ permalink raw reply [flat|nested] 34+ messages in thread
* [PATCH net-next 2/4] security: bpf: Add LSM hooks for bpf object related syscall
@ 2017-10-12 0:31 ` James Morris
0 siblings, 0 replies; 34+ messages in thread
From: James Morris @ 2017-10-12 0:31 UTC (permalink / raw)
To: linux-security-module
On Wed, 4 Oct 2017, Chenbo Feng wrote:
> int bpf_map_new_fd(struct bpf_map *map, int flags)
> {
> + if (security_bpf_map(map, OPEN_FMODE(flags)))
> + return -EPERM;
> +
Don't hardcode -EPERM here, return the actual error from
security_bpf_map().
> + if (security_bpf_prog(prog))
> + return -EPERM;
> +
Same.
> + err = security_bpf(cmd, &attr, size);
> + if (err)
> + return -EPERM;
Same.
- James
--
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
^ permalink raw reply [flat|nested] 34+ messages in thread
end of thread, other threads:[~2017-10-12 0:31 UTC | newest]
Thread overview: 34+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-04 18:29 [PATCH 0/4] net-next: security: New file mode and LSM hooks for eBPF object permission control Chenbo Feng
2017-10-04 18:29 ` Chenbo Feng
2017-10-04 18:29 ` [PATCH net-next 1/4] bpf: Add file mode configuration into bpf maps Chenbo Feng
2017-10-04 18:29 ` Chenbo Feng
2017-10-04 23:29 ` Daniel Borkmann
2017-10-04 23:29 ` Daniel Borkmann
2017-10-04 23:58 ` Chenbo Feng
2017-10-04 23:58 ` Chenbo Feng
2017-10-05 0:51 ` Daniel Borkmann
2017-10-05 0:51 ` Daniel Borkmann
2017-10-04 18:29 ` [PATCH net-next 2/4] security: bpf: Add LSM hooks for bpf object related syscall Chenbo Feng
2017-10-04 18:29 ` Chenbo Feng
2017-10-12 0:31 ` James Morris
2017-10-12 0:31 ` James Morris
2017-10-04 18:29 ` [PATCH net-next 3/4] selinux: bpf: Add selinux check for eBPF syscall operations Chenbo Feng
2017-10-04 18:29 ` Chenbo Feng
2017-10-05 13:28 ` Stephen Smalley
2017-10-05 13:28 ` Stephen Smalley
2017-10-05 16:12 ` Daniel Borkmann
2017-10-05 16:12 ` Daniel Borkmann
2017-10-04 18:29 ` [PATCH net-next 4/4] selinux: bpf: Add addtional check for bpf object file receive Chenbo Feng
2017-10-04 18:29 ` Chenbo Feng
2017-10-04 23:44 ` Daniel Borkmann
2017-10-04 23:44 ` Daniel Borkmann
2017-10-04 23:52 ` Daniel Borkmann
2017-10-04 23:52 ` Daniel Borkmann
2017-10-05 13:37 ` Stephen Smalley
2017-10-05 13:37 ` Stephen Smalley
2017-10-05 18:26 ` Stephen Smalley
2017-10-05 18:26 ` Stephen Smalley
2017-10-06 21:10 ` Chenbo Feng
2017-10-06 21:10 ` Chenbo Feng
2017-10-06 21:00 ` Chenbo Feng
2017-10-06 21:00 ` Chenbo Feng
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.