bpf: introduce BPF_CGROUP_FILE_OPEN

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

Quoted 12 lines · Alexei Starovoitov

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

Quoted 10 lines

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

It's probably nice to have file uid/gid as well.

Quoted 25 lines · Roman Gushchin

Quoted 22 lines · Alexei Starovoitov

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

Quoted 1 line

It's probably nice to have file uid/gid as well.

And an indication of which mount namespace we're looking at.

kernel/bpf/cgroup.c line 18

Quoted 5 lines · Alexei Starovoitov

kernel/bpf/cgroup.c +1 -0

@@ -15,6 +15,7 @@
 #include <linux/bpf.h>
 #include <linux/bpf-cgroup.h>
 #include <net/sock.h>
+#include <../fs/mount.h>

No.

kernel/bpf/cgroup.c const struct bpf_verifier_ops cg_dev_verifier_ops = { · line 867

Quoted 4 lines

kernel/bpf/cgroup.c +4 -0

+	struct file *file = NULL;
+	struct inode *inode;
+	struct super_block *sb;
+	struct mount *mnt;

Fuck, no.

kernel/bpf/cgroup.c const struct bpf_verifier_ops cg_dev_verifier_ops = { · line 912

Quoted 4 lines

kernel/bpf/cgroup.c +4 -0

+	case offsetof(struct bpf_file_info, mnt_id):
+		/* dst = real_mount(file->f_path.mnt)->mnt_id */
+		mnt = real_mount(LD_1(file->f_path.mnt));
+		LD_n(mnt->mnt_id);

NAK. Anything in struct mount is private to just a couple of files in fs/*.c. Don't do that. And keep in mind that internal details can and will be changed at zero notice, so be careful with adding such stuff.

Another problem is your direct poking in ->i_ino. It's not something directly exposed to userland at the moment and it should not become such.

Filesystem has every right to have ->getattr() set ->ino (== ->st_ino value) in whichever way it likes;

the same
goes for ->dev.

Quoted 3 lines · Al Viro

Another problem is your direct poking in ->i_ino. It's not something directly exposed to userland at the moment and it should not become such.

The patch is not making i_ino directly exposed.
Only 'struct bpf_file_info' is exposed to user space / bpf programs.

Quoted 7 lines · Alexei Starovoitov

Quoted 3 lines · Al Viro

Another problem is your direct poking in ->i_ino. It's not something directly exposed to userland at the moment and it should not become such.

Quoted 2 lines

The patch is not making i_ino directly exposed.
Only 'struct bpf_file_info' is exposed to user space / bpf programs.

I think Al is saying that the valie of i_ino is not something that user code is permitted to know regardless of how you format it because it may or may not actually match the value returned by stat(). Another way of saying that is that your patch is digging into an internal data structure and is doing it wrong.

Quoted 9 lines · Andy Lutomirski

Quoted 7 lines · Alexei Starovoitov

Quoted 3 lines · Al Viro

Another problem is your direct poking in ->i_ino. It's not something directly exposed to userland at the moment and it should not become such.

Quoted 8 lines

Quoted 2 lines

The patch is not making i_ino directly exposed.
Only 'struct bpf_file_info' is exposed to user space / bpf programs.

I think Al is saying that the valie of i_ino is not something that user code is permitted to know regardless of how you format it because it may or may not actually match the value returned by stat(). Another way of saying that is that your patch is digging into an internal data structure and is doing it wrong.

several fs implementation I've looked at just do generic_fillattr() for these fields. Are you saying some FS don't use inode->i_ino at all? And it's bogus, hence shouldn't be read?

And no, it's not that each of those filesystems does not use inode->i_ino at all. There's a bunch of library helpers in fs/*.c that happen to use the value filesystem has seen fit to store there. Whether to use those helpers or not, what to store in that field, etc. is, again, entirely up to the filesystem in question. generic_fillattr() is one of those, that's all there is to it.

That's precisely why I really do not like the idea of hooks poking in the internals of kernel data structures. Especially since not even "it's not visible outside of a subsystem-internal header" appears to slow you down.

The same goes for tracepoints, etc. - turning random details of implementation into a carved in stone ABI is actively harmful.

PS: If anything, visibility to hooks should be opt-in.

Sure, we can start actively hiding
the things, but that's a winless arms race - even if we bloody went and encrypted the private stuff, you'd still be able to pull decryption key from where it would be accessed by legitimate users, etc. Нахуй нам это надо?

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

Other file attributes can be added in the future to the end of this struct without breaking bpf programs.

For debugging introduce bpf_get_file_path() helper that returns NUL-terminated full path of the file. It should never be used for sandboxing.

Use cases:
- disallow certain FS types within containers (fs_magic == CGROUP2_SUPER_MAGIC)
- restrict permissions in particular mount (mnt_id == X && (flags & O_RDWR))
- disallow access to hard linked sensitive files (nlink > 1 && mode == 0700)
- disallow access to world writeable files (mode == 0..7)
- disallow access to given set of files (dev_major == X && dev_minor == Y && inode == Z)

Signed-off-by Alexei Starovoitov <[email protected]>

---
 include/linux/bpf-cgroup.h |  10 +++
 include/linux/bpf_types.h  |   1 +
 include/uapi/linux/bpf.h   |  28 +++++-
 kernel/bpf/cgroup.c        | 171 +++++++++++++++++++++++++++++++++++++
 kernel/bpf/syscall.c       |   7 ++
 5 files changed, 216 insertions(+), 1 deletion(-)

include/linux/bpf-cgroup.h +10 -0

index 588dd5f0bd85..766f0223c222 100644
@@ -109,6 +109,8 @@ int __cgroup_bpf_run_filter_sock_ops(struct sock *sk,
 int __cgroup_bpf_check_dev_permission(short dev_type, u32 major, u32 minor,
                                      short access, enum bpf_attach_type type);
 
+int __cgroup_bpf_file_filter(struct file *file, enum bpf_attach_type type);
+
 static inline enum bpf_cgroup_storage_type cgroup_storage_type(
        struct bpf_map *map)
 {

@@ -253,6 +255,13 @@ int bpf_percpu_cgroup_storage_update(struct bpf_map *map, void *key,
                                                                              \
        __ret;                                                                \
 })
+#define BPF_CGROUP_RUN_PROG_FILE_FILTER(file)                               \
+({                                                                           \
+       int __ret = 0;                                                        \
+       if (cgroup_bpf_enabled)                                               \
+               __ret = __cgroup_bpf_file_filter(file, BPF_CGROUP_FILE_OPEN); \
+       __ret;                                                                \
+})
 int cgroup_bpf_prog_attach(const union bpf_attr *attr,
                           enum bpf_prog_type ptype, struct bpf_prog *prog);
 int cgroup_bpf_prog_detach(const union bpf_attr *attr,

@@ -321,6 +330,7 @@ static inline int bpf_percpu_cgroup_storage_update(struct bpf_map *map,
 #define BPF_CGROUP_RUN_PROG_UDP6_SENDMSG_LOCK(sk, uaddr, t_ctx) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_SOCK_OPS(sock_ops) ({ 0; })
 #define BPF_CGROUP_RUN_PROG_DEVICE_CGROUP(type,major,minor,access) ({ 0; })
+#define BPF_CGROUP_RUN_PROG_FILE_FILTER(file) ({ 0; })
 
 #define for_each_cgroup_storage_type(stype) for (; false; )
 

include/linux/bpf_types.h +1 -0

index 5432f4c9f50e..f182b2e37b94 100644
@@ -33,6 +33,7 @@ BPF_PROG_TYPE(BPF_PROG_TYPE_LIRC_MODE2, lirc_mode2)
 #ifdef CONFIG_INET
 BPF_PROG_TYPE(BPF_PROG_TYPE_SK_REUSEPORT, sk_reuseport)
 #endif
+BPF_PROG_TYPE(BPF_PROG_TYPE_FILE_FILTER, file_filter)
 
 BPF_MAP_TYPE(BPF_MAP_TYPE_ARRAY, array_map_ops)
 BPF_MAP_TYPE(BPF_MAP_TYPE_PERCPU_ARRAY, percpu_array_map_ops)

include/uapi/linux/bpf.h +26 -1

index f9187b41dff6..c0df8dd99edc 100644
@@ -154,6 +154,7 @@ enum bpf_prog_type {
        BPF_PROG_TYPE_LIRC_MODE2,
        BPF_PROG_TYPE_SK_REUSEPORT,
        BPF_PROG_TYPE_FLOW_DISSECTOR,
+       BPF_PROG_TYPE_FILE_FILTER,
 };
 
 enum bpf_attach_type {

@@ -175,6 +176,7 @@ enum bpf_attach_type {
        BPF_CGROUP_UDP6_SENDMSG,
        BPF_LIRC_MODE2,
        BPF_FLOW_DISSECTOR,
+       BPF_CGROUP_FILE_OPEN,
        __MAX_BPF_ATTACH_TYPE
 };
 

@@ -2215,6 +2217,18 @@ union bpf_attr {
  *             pointer that was returned from bpf_sk_lookup_xxx\ ().
  *     Return
  *             0 on success, or a negative error in case of failure.
+ *
+ * int bpf_get_file_path(struct bpf_file_info *file, char *buf, u32 size_of_buf)
+ *     Description
+ *             Reconstruct the full path of *file* and store it into *buf* of
+ *             *size_of_buf*. The *size_of_buf* must be strictly positive.
+ *             On success, the helper makes sure that the *buf* is NUL-terminated.
+ *             On failure, it is filled with string "(error)".
+ *             This helper should only be used for debugging.
+ *             'char *path' should never be used for permission checks.
+ *     Return
+ *             0 on success, or a negative error in case of failure.
+ *
  */
 #define __BPF_FUNC_MAPPER(FN)          \
        FN(unspec),                     \

@@ -2303,7 +2317,8 @@ union bpf_attr {
        FN(skb_ancestor_cgroup_id),     \
        FN(sk_lookup_tcp),              \
        FN(sk_lookup_udp),              \
-       FN(sk_release),
+       FN(sk_release),                 \
+       FN(get_file_path),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call

@@ -2896,4 +2911,15 @@ struct bpf_flow_keys {
        };
 };
 
+struct bpf_file_info {
+       __u64 inode;
+       __u32 dev_major;
+       __u32 dev_minor;
+       __u32 fs_magic;
+       __u32 mnt_id;
+       __u32 nlink;
+       __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
+       __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
+};

Quoted 6 lines

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Why do_dentry_open() specifically, and nothing else? If you want to filter open, wouldn't you also want to filter a bunch of other filesystem operations with LSM hooks, like rename, unlink, truncate and so on? Landlock benefits there from re-using the existing security hooks.

Quoted 21 lines

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

Other file attributes can be added in the future to the end of this struct without breaking bpf programs.

For debugging introduce bpf_get_file_path() helper that returns NUL-terminated full path of the file. It should never be used for sandboxing.

Quoted 6 lines

Use cases:
- disallow certain FS types within containers (fs_magic == CGROUP2_SUPER_MAGIC)
- restrict permissions in particular mount (mnt_id == X && (flags & O_RDWR))
- disallow access to hard linked sensitive files (nlink > 1 && mode == 0700)
- disallow access to world writeable files (mode == 0..7)
- disallow access to given set of files (dev_major == X && dev_minor == Y && inode == Z)

That last one sounds weird. It doesn't work if you want to ban access to a whole directory at once. And in general, highly specific blocklists make me nervous, because if you add anything new and forget to put it on the list, you have a problem.

Review

Quoted 2 lines · Jann Horn

(quoted diff) +2 -0

+cc kernel-hardening because this is related to sandboxing
+cc Mickaël Salaün because this looks related to his Landlock proposal

It may seem that this work overlaps with landlock, but the goals are different. Landlock is LSM based to act as _security_ framework with end goal being available to unpriv users.

While this cgroup-bpf hook is expected to stay root only,
since we're trying to restrict what containers can do in a trusted environment. 'sandboxing' is overloaded word.
Sandboxing for security and sandboxing of trusted root are different.

Quoted 9 lines · Alexei Starovoitov

Quoted 6 lines

Similar to networking sandboxing programs and cgroup-v2 based hooks (BPF_CGROUP_INET_[INGRESS|EGRESS,] BPF_CGROUP_INET[4|6]_[BIND|CONNECT], etc) introduce basic per-container sandboxing for file access via
new BPF_PROG_TYPE_FILE_FILTER program type that attaches after security_file_open() LSM hook and works as additional file_open filter. The new cgroup bpf hook is called BPF_CGROUP_FILE_OPEN.

Quoted 5 lines

Why do_dentry_open() specifically, and nothing else? If you want to filter open, wouldn't you also want to filter a bunch of other filesystem operations with LSM hooks, like rename, unlink, truncate and so on? Landlock benefits there from re-using the existing security hooks.

It may make sense to extend in the future, but we don't have clear user cases for rename/unlink/truncate at this point.

As you can see the amount of pushback even for basic file access is high. Hence I don't think the landlock can be upstreamed in the current form, since it touches VFS layer a lot more than this patch.

It's intrusive to LSM, and adds new concepts to BPF as well. This work fits into existing BPF machinery and minimally intrusive to VFS. I hope we can find a common ground with Al regarding what file access primitives are exposed to BPF side.
Once we agree on that the landlock can piggy back on this work and extend it to all file-based LSM hooks.

The first step for everyone interested in bpf-based 'sandboxing' is to figure out VFS<->BPF interface.
If you or Mickael have suggestions on what bpf progs should and should not see at these hooks, it's a good time to discuss.
I believe the fields proposed are the obvious minimum.

Quoted 19 lines

Quoted 19 lines

Just like other cgroup-bpf programs new BPF_PROG_TYPE_FILE_FILTER type is only available to root.

This program type has access to single argument 'struct bpf_file_info' that contains standard sys_stat fields:

struct bpf_file_info {
        __u64 inode;
        __u32 dev_major;
        __u32 dev_minor;
        __u32 fs_magic;
        __u32 mnt_id;
        __u32 nlink;
        __u32 mode;     /* file mode S_ISDIR, S_ISLNK, 0755, etc */
        __u32 flags;    /* open flags O_RDWR, O_CREAT, etc */
};

Other file attributes can be added in the future to the end of this struct without breaking bpf programs.

Quoted 2 lines

Quoted 2 lines

For debugging introduce bpf_get_file_path() helper that returns NUL-terminated full path of the file. It should never be used for sandboxing.

Quoted 7 lines

Quoted 7 lines

Use cases:
- disallow certain FS types within containers (fs_magic == CGROUP2_SUPER_MAGIC)
- restrict permissions in particular mount (mnt_id == X && (flags & O_RDWR))
- disallow access to hard linked sensitive files (nlink > 1 && mode == 0700)
- disallow access to world writeable files (mode == 0..7)
- disallow access to given set of files (dev_major == X && dev_minor == Y && inode == Z)

Quoted 5 lines

That last one sounds weird. It doesn't work if you want to ban access to a whole directory at once. And in general, highly specific blocklists make me nervous, because if you add anything new and forget to put it on the list, you have a problem.

In the upcoming V2 of the patches the direct exposure to dev and inode will be removed. And instead the opaque 'struct file_handle' will be available to bpf progs. The use case is indeed to restrict access to specific blacklist of files. The user space will collect the set of files via sys_name_to_handle_at(), then store the fhandles in bpf map, and bpf prog will consult the map
to deny the access.
It's not a replacement for ACLs, directory permissions, etc. The use case is to prevent trusted containers messing up the environment.
The continuous integration system needs to run some containers (and tests inside them) with root privs. When these jobs mess up the system the subsequent jobs may incorrectly fail. We believe that this cgroup based container enforcement will solve this use case and similar other use cases when containers are trusted, but could be buggy when it comes to file access.

To recap what I'm implementing in V2:
1.

struct bpf_file_info {
        __u32 fs_magic; // file->f_inode->i_sb->s_magic
        __u32 mnt_id;   // real_mount(file->f_path.mnt)->mnt_id
        __u32 nlink;    // file->f_inode->i_nlink
        __u32 mode;     // file->f_inode->i_mode
        __u32 flags;    // file->f_flags
};

I double checked what VFS layer does with above fields and I think there is no additional user space exposure will be made when such fields are seen by bpf progs.
But since I'm not a VFS expert, I'd like Al to confirm.

2.
bpf_get_file_handle(struct bpf_file_info *ctx, struct file_handle *fh, int fh_size); helper that bpf prog will use to obtain fh of the file about to be open.

3.
bpf_get_file_statx(struct bpf_file_info *ctx, struct statx *sx, int size, int flags); Though struct statx is 256 bytes, and the helper would have to touch all bytes I couldn't figure out the faster way to get to inode/dev/uid of the given file that will work on all underlying FSes.

Thoughts?