| Message ID | 20231103190523.6353-12-andrii@kernel.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | BPF token and BPF FS-based delegation | expand |
Hi Andrii,
kernel test robot noticed the following build errors:
[auto build test ERROR on bpf-next/master]
url: https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-align-CAP_NET_ADMIN-checks-with-bpf_capable-approach/20231104-031714
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
patch link: https://lore.kernel.org/r/20231103190523.6353-12-andrii%40kernel.org
patch subject: [PATCH v9 bpf-next 11/17] bpf,lsm: add BPF token LSM hooks
config: m68k-defconfig (https://download.01.org/0day-ci/archive/20231104/202311040829.XrnpSV8z-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311040829.XrnpSV8z-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311040829.XrnpSV8z-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/net/scm.h:8,
from include/linux/netlink.h:9,
from include/uapi/linux/neighbour.h:6,
from include/linux/netdevice.h:45,
from include/net/sock.h:46,
from include/linux/tcp.h:19,
from include/linux/ipv6.h:95,
from include/net/ipv6.h:12,
from include/linux/sunrpc/addr.h:14,
from fs/nfsd/nfsd.h:22,
from fs/nfsd/state.h:42,
from fs/nfsd/xdr4.h:40,
from fs/nfsd/trace.h:17,
from fs/nfsd/trace.c:4:
>> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ~~~~~~~~~~~~~^~~
>> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes]
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: some warnings being treated as errors
--
In file included from include/net/scm.h:8,
from include/linux/netlink.h:9,
from include/uapi/linux/neighbour.h:6,
from include/linux/netdevice.h:45,
from include/net/sock.h:46,
from include/linux/tcp.h:19,
from include/linux/ipv6.h:95,
from include/net/ipv6.h:12,
from include/linux/sunrpc/addr.h:14,
from fs/nfsd/nfsd.h:22,
from fs/nfsd/export.c:21:
>> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ~~~~~~~~~~~~~^~~
>> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes]
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
fs/nfsd/export.c: In function 'exp_rootfh':
fs/nfsd/export.c:1017:34: warning: variable 'inode' set but not used [-Wunused-but-set-variable]
1017 | struct inode *inode;
| ^~~~~
cc1: some warnings being treated as errors
--
In file included from include/net/scm.h:8,
from include/linux/netlink.h:9,
from include/uapi/linux/neighbour.h:6,
from include/linux/netdevice.h:45,
from include/net/sock.h:46,
from include/linux/tcp.h:19,
from include/linux/ipv6.h:95,
from include/net/ipv6.h:12,
from include/linux/sunrpc/addr.h:14,
from fs/nfsd/nfsd.h:22,
from fs/nfsd/state.h:42,
from fs/nfsd/xdr4.h:40,
from fs/nfsd/trace.h:17,
from fs/nfsd/trace.c:4:
>> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ~~~~~~~~~~~~~^~~
>> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes]
2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from fs/nfsd/trace.h:1958:
include/trace/define_trace.h:95:42: fatal error: ./trace.h: No such file or directory
95 | #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
| ^
cc1: some warnings being treated as errors
compilation terminated.
vim +2084 include/linux/security.h
2083
> 2084 static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd)
2085 {
2086 return 0;
2087 }
2088
On Fri, Nov 3, 2023 at 5:38 PM kernel test robot <lkp@intel.com> wrote: > > Hi Andrii, > > kernel test robot noticed the following build errors: > > [auto build test ERROR on bpf-next/master] > > url: https://github.com/intel-lab-lkp/linux/commits/Andrii-Nakryiko/bpf-align-CAP_NET_ADMIN-checks-with-bpf_capable-approach/20231104-031714 > base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master > patch link: https://lore.kernel.org/r/20231103190523.6353-12-andrii%40kernel.org > patch subject: [PATCH v9 bpf-next 11/17] bpf,lsm: add BPF token LSM hooks > config: m68k-defconfig (https://download.01.org/0day-ci/archive/20231104/202311040829.XrnpSV8z-lkp@intel.com/config) > compiler: m68k-linux-gcc (GCC) 13.2.0 > reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311040829.XrnpSV8z-lkp@intel.com/reproduce) > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202311040829.XrnpSV8z-lkp@intel.com/ > > All errors (new ones prefixed by >>): > > In file included from include/net/scm.h:8, > from include/linux/netlink.h:9, > from include/uapi/linux/neighbour.h:6, > from include/linux/netdevice.h:45, > from include/net/sock.h:46, > from include/linux/tcp.h:19, > from include/linux/ipv6.h:95, > from include/net/ipv6.h:12, > from include/linux/sunrpc/addr.h:14, > from fs/nfsd/nfsd.h:22, > from fs/nfsd/state.h:42, > from fs/nfsd/xdr4.h:40, > from fs/nfsd/trace.h:17, > from fs/nfsd/trace.c:4: > >> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ~~~~~~~~~~~~~^~~ > >> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes] > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > cc1: some warnings being treated as errors Ok, so apparently enum forward declaration doesn't work with static inline functions. Would it be ok to just #include <linux/bpf.h> in this file? $ git diff diff --git a/include/linux/security.h b/include/linux/security.h index 1d6edbf45d1c..cfe6176824c2 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -32,6 +32,7 @@ #include <linux/string.h> #include <linux/mm.h> #include <linux/sockptr.h> +#include <linux/bpf.h> struct linux_binprm; struct cred; @@ -60,7 +61,6 @@ struct fs_parameter; enum fs_value_type; struct watch; struct watch_notification; -enum bpf_cmd; /* Default (no) options for the capable function */ #define CAP_OPT_NONE 0x0 If not, then I guess another alternative would be to pass `int cmd` instead of `enum bpf_cmd cmd`, but that doesn't seems like the best solution, tbh. Paul, any preferences? > -- > In file included from include/net/scm.h:8, > from include/linux/netlink.h:9, > from include/uapi/linux/neighbour.h:6, > from include/linux/netdevice.h:45, > from include/net/sock.h:46, > from include/linux/tcp.h:19, > from include/linux/ipv6.h:95, > from include/net/ipv6.h:12, > from include/linux/sunrpc/addr.h:14, > from fs/nfsd/nfsd.h:22, > from fs/nfsd/export.c:21: > >> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ~~~~~~~~~~~~~^~~ > >> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes] > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > fs/nfsd/export.c: In function 'exp_rootfh': > fs/nfsd/export.c:1017:34: warning: variable 'inode' set but not used [-Wunused-but-set-variable] > 1017 | struct inode *inode; > | ^~~~~ > cc1: some warnings being treated as errors > -- > In file included from include/net/scm.h:8, > from include/linux/netlink.h:9, > from include/uapi/linux/neighbour.h:6, > from include/linux/netdevice.h:45, > from include/net/sock.h:46, > from include/linux/tcp.h:19, > from include/linux/ipv6.h:95, > from include/net/ipv6.h:12, > from include/linux/sunrpc/addr.h:14, > from fs/nfsd/nfsd.h:22, > from fs/nfsd/state.h:42, > from fs/nfsd/xdr4.h:40, > from fs/nfsd/trace.h:17, > from fs/nfsd/trace.c:4: > >> include/linux/security.h:2084:92: error: parameter 2 ('cmd') has incomplete type > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ~~~~~~~~~~~~~^~~ > >> include/linux/security.h:2084:19: error: function declaration isn't a prototype [-Werror=strict-prototypes] > 2084 | static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ > In file included from fs/nfsd/trace.h:1958: > include/trace/define_trace.h:95:42: fatal error: ./trace.h: No such file or directory > 95 | #include TRACE_INCLUDE(TRACE_INCLUDE_FILE) > | ^ > cc1: some warnings being treated as errors > compilation terminated. > > > vim +2084 include/linux/security.h > > 2083 > > 2084 static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > 2085 { > 2086 return 0; > 2087 } > 2088 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki
On Nov 3, 2023 Andrii Nakryiko <andrii@kernel.org> wrote: > > Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to > allocate LSM security blob (we add `void *security` field to struct > bpf_token for that), but also control who can instantiate BPF token. > This follows existing pattern for BPF map and BPF prog. > > Also add security_bpf_token_allow_cmd() and security_bpf_token_capable() > LSM hooks that allow LSM implementation to control and negate (if > necessary) BPF token's delegation of a specific bpf_cmd and capability, > respectively. > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > --- > include/linux/bpf.h | 3 ++ > include/linux/lsm_hook_defs.h | 5 +++ > include/linux/security.h | 25 +++++++++++++++ > kernel/bpf/bpf_lsm.c | 4 +++ > kernel/bpf/token.c | 13 ++++++-- > security/security.c | 60 +++++++++++++++++++++++++++++++++++ > 6 files changed, 107 insertions(+), 3 deletions(-) ... > diff --git a/include/linux/security.h b/include/linux/security.h > index 08fd777cbe94..1d6edbf45d1c 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -60,6 +60,7 @@ struct fs_parameter; > enum fs_value_type; > struct watch; > struct watch_notification; > +enum bpf_cmd; Yes, I think it's fine to include bpf.h in security.h instead of the forward declaration. > /* Default (no) options for the capable function */ > #define CAP_OPT_NONE 0x0 > @@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map); > extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, > struct bpf_token *token); > extern void security_bpf_prog_free(struct bpf_prog *prog); > +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > + struct path *path); > +extern void security_bpf_token_free(struct bpf_token *token); > +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd); > +extern int security_bpf_token_capable(const struct bpf_token *token, int cap); > #else > static inline int security_bpf(int cmd, union bpf_attr *attr, > unsigned int size) > @@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr * > > static inline void security_bpf_prog_free(struct bpf_prog *prog) > { } > + > +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > + struct path *path) > +{ > + return 0; > +} > + > +static inline void security_bpf_token_free(struct bpf_token *token) > +{ } > + > +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > +{ > + return 0; > +} > + > +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap) > +{ > + return 0; > +} Another nitpick, but I would prefer to shorten security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both to shorten the name and to better fit convention. I realize the caller is named bpf_token_allow_cmd() but I'd still rather see the LSM hook with the shorter name. > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c > index 35e6f55c2a41..5d04da54faea 100644 > --- a/kernel/bpf/token.c > +++ b/kernel/bpf/token.c > @@ -7,11 +7,12 @@ > #include <linux/idr.h> > #include <linux/namei.h> > #include <linux/user_namespace.h> > +#include <linux/security.h> > > bool bpf_token_capable(const struct bpf_token *token, int cap) > { > /* BPF token allows ns_capable() level of capabilities */ > - if (token) { > + if (token && security_bpf_token_capable(token, cap) == 0) { > if (ns_capable(token->userns, cap)) > return true; > if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN)) We typically perform the capability based access controls prior to the LSM controls, meaning if we want to the token controls to work in a similar way we should do something like this: bool bpf_token_capable(...) { if (token) { if (ns_capable(token, cap) || (cap != ADMIN && ns_capable(token, ADMIN))) return security_bpf_token_capable(token, cap); } return capable(cap) || (cap != ADMIN && capable(...)) } > @@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token) > > static void bpf_token_free(struct bpf_token *token) > { > + security_bpf_token_free(token); > put_user_ns(token->userns); > kvfree(token); > } > @@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr) > token->allowed_progs = mnt_opts->delegate_progs; > token->allowed_attachs = mnt_opts->delegate_attachs; > > + err = security_bpf_token_create(token, attr, &path); > + if (err) > + goto out_token; > + > fd = get_unused_fd_flags(O_CLOEXEC); > if (fd < 0) { > err = fd; > @@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > { > if (!token) > return false; > - > - return token->allowed_cmds & (1ULL << cmd); > + if (!(token->allowed_cmds & (1ULL << cmd))) > + return false; > + return security_bpf_token_allow_cmd(token, cmd) == 0; I'm not sure how much it really matters, but someone might prefer the '!!' approach/style over '== 0'. > } > > bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type) -- paul-moore.com
On Sun, Nov 5, 2023 at 9:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Nov 3, 2023 Andrii Nakryiko <andrii@kernel.org> wrote: > > > > Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to > > allocate LSM security blob (we add `void *security` field to struct > > bpf_token for that), but also control who can instantiate BPF token. > > This follows existing pattern for BPF map and BPF prog. > > > > Also add security_bpf_token_allow_cmd() and security_bpf_token_capable() > > LSM hooks that allow LSM implementation to control and negate (if > > necessary) BPF token's delegation of a specific bpf_cmd and capability, > > respectively. > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > --- > > include/linux/bpf.h | 3 ++ > > include/linux/lsm_hook_defs.h | 5 +++ > > include/linux/security.h | 25 +++++++++++++++ > > kernel/bpf/bpf_lsm.c | 4 +++ > > kernel/bpf/token.c | 13 ++++++-- > > security/security.c | 60 +++++++++++++++++++++++++++++++++++ > > 6 files changed, 107 insertions(+), 3 deletions(-) > > ... > > > diff --git a/include/linux/security.h b/include/linux/security.h > > index 08fd777cbe94..1d6edbf45d1c 100644 > > --- a/include/linux/security.h > > +++ b/include/linux/security.h > > @@ -60,6 +60,7 @@ struct fs_parameter; > > enum fs_value_type; > > struct watch; > > struct watch_notification; > > +enum bpf_cmd; > > Yes, I think it's fine to include bpf.h in security.h instead of the > forward declaration. > > > /* Default (no) options for the capable function */ > > #define CAP_OPT_NONE 0x0 > > @@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map); > > extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, > > struct bpf_token *token); > > extern void security_bpf_prog_free(struct bpf_prog *prog); > > +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > > + struct path *path); > > +extern void security_bpf_token_free(struct bpf_token *token); > > +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd); > > +extern int security_bpf_token_capable(const struct bpf_token *token, int cap); > > #else > > static inline int security_bpf(int cmd, union bpf_attr *attr, > > unsigned int size) > > @@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr * > > > > static inline void security_bpf_prog_free(struct bpf_prog *prog) > > { } > > + > > +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > > + struct path *path) > > +{ > > + return 0; > > +} > > + > > +static inline void security_bpf_token_free(struct bpf_token *token) > > +{ } > > + > > +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > > +{ > > + return 0; > > +} > > + > > +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap) > > +{ > > + return 0; > > +} > > Another nitpick, but I would prefer to shorten > security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both > to shorten the name and to better fit convention. I realize the caller > is named bpf_token_allow_cmd() but I'd still rather see the LSM hook > with the shorter name. Makes sense, renamed to security_bpf_token_cmd() and updated hook name as well > > > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c > > index 35e6f55c2a41..5d04da54faea 100644 > > --- a/kernel/bpf/token.c > > +++ b/kernel/bpf/token.c > > @@ -7,11 +7,12 @@ > > #include <linux/idr.h> > > #include <linux/namei.h> > > #include <linux/user_namespace.h> > > +#include <linux/security.h> > > > > bool bpf_token_capable(const struct bpf_token *token, int cap) > > { > > /* BPF token allows ns_capable() level of capabilities */ > > - if (token) { > > + if (token && security_bpf_token_capable(token, cap) == 0) { > > if (ns_capable(token->userns, cap)) > > return true; > > if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN)) > > We typically perform the capability based access controls prior to the > LSM controls, meaning if we want to the token controls to work in a > similar way we should do something like this: > > bool bpf_token_capable(...) > { > if (token) { > if (ns_capable(token, cap) || > (cap != ADMIN && ns_capable(token, ADMIN))) > return security_bpf_token_capable(token, cap); > } > return capable(cap) || (cap != ADMIN && capable(...)) > } yep, makes sense, I changed it as you suggested above > > > @@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token) > > > > static void bpf_token_free(struct bpf_token *token) > > { > > + security_bpf_token_free(token); > > put_user_ns(token->userns); > > kvfree(token); > > } > > @@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr) > > token->allowed_progs = mnt_opts->delegate_progs; > > token->allowed_attachs = mnt_opts->delegate_attachs; > > > > + err = security_bpf_token_create(token, attr, &path); > > + if (err) > > + goto out_token; > > + > > fd = get_unused_fd_flags(O_CLOEXEC); > > if (fd < 0) { > > err = fd; > > @@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > > { > > if (!token) > > return false; > > - > > - return token->allowed_cmds & (1ULL << cmd); > > + if (!(token->allowed_cmds & (1ULL << cmd))) > > + return false; > > + return security_bpf_token_allow_cmd(token, cmd) == 0; > > I'm not sure how much it really matters, but someone might prefer > the '!!' approach/style over '== 0'. it would have to be !security_bpf_token_cmd(), right? And that single negation is just very confusing when dealing with int-returning function. I find it much easier to make sure the logic is correct when we have explicit `== 0`. Like, when I see `return !security_bpf_token_cmd(...);`, my immediate read of that is "return whether bpf_token_cmd is not allowed" or something along those lines, giving me a huge pause... I have the same relationship with strcmp(), btw, while people seem totally fine with `!strcmp()` (which to me also reads backwards). Anyways, unless you really feel strongly, I'd keep == 0 here and above for security_bpf_token_capable(), just because it's int-returning function result conversion to bool-returning result. > > > } > > > > bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type) > > -- > paul-moore.com
On Mon, Nov 6, 2023 at 2:17 PM Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Sun, Nov 5, 2023 at 9:01 PM Paul Moore <paul@paul-moore.com> wrote: > > On Nov 3, 2023 Andrii Nakryiko <andrii@kernel.org> wrote: > > > > > > Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to > > > allocate LSM security blob (we add `void *security` field to struct > > > bpf_token for that), but also control who can instantiate BPF token. > > > This follows existing pattern for BPF map and BPF prog. > > > > > > Also add security_bpf_token_allow_cmd() and security_bpf_token_capable() > > > LSM hooks that allow LSM implementation to control and negate (if > > > necessary) BPF token's delegation of a specific bpf_cmd and capability, > > > respectively. > > > > > > Signed-off-by: Andrii Nakryiko <andrii@kernel.org> > > > --- > > > include/linux/bpf.h | 3 ++ > > > include/linux/lsm_hook_defs.h | 5 +++ > > > include/linux/security.h | 25 +++++++++++++++ > > > kernel/bpf/bpf_lsm.c | 4 +++ > > > kernel/bpf/token.c | 13 ++++++-- > > > security/security.c | 60 +++++++++++++++++++++++++++++++++++ > > > 6 files changed, 107 insertions(+), 3 deletions(-) > > > > ... > > > > > diff --git a/include/linux/security.h b/include/linux/security.h > > > index 08fd777cbe94..1d6edbf45d1c 100644 > > > --- a/include/linux/security.h > > > +++ b/include/linux/security.h > > > @@ -60,6 +60,7 @@ struct fs_parameter; > > > enum fs_value_type; > > > struct watch; > > > struct watch_notification; > > > +enum bpf_cmd; > > > > Yes, I think it's fine to include bpf.h in security.h instead of the > > forward declaration. > > > > > /* Default (no) options for the capable function */ > > > #define CAP_OPT_NONE 0x0 > > > @@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map); > > > extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, > > > struct bpf_token *token); > > > extern void security_bpf_prog_free(struct bpf_prog *prog); > > > +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > > > + struct path *path); > > > +extern void security_bpf_token_free(struct bpf_token *token); > > > +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd); > > > +extern int security_bpf_token_capable(const struct bpf_token *token, int cap); > > > #else > > > static inline int security_bpf(int cmd, union bpf_attr *attr, > > > unsigned int size) > > > @@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr * > > > > > > static inline void security_bpf_prog_free(struct bpf_prog *prog) > > > { } > > > + > > > +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, > > > + struct path *path) > > > +{ > > > + return 0; > > > +} > > > + > > > +static inline void security_bpf_token_free(struct bpf_token *token) > > > +{ } > > > + > > > +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > > > +{ > > > + return 0; > > > +} > > > + > > > +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap) > > > +{ > > > + return 0; > > > +} > > > > Another nitpick, but I would prefer to shorten > > security_bpf_token_allow_cmd() renamed to security_bpf_token_cmd() both > > to shorten the name and to better fit convention. I realize the caller > > is named bpf_token_allow_cmd() but I'd still rather see the LSM hook > > with the shorter name. > > Makes sense, renamed to security_bpf_token_cmd() and updated hook name as well Thanks. > > > diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c > > > index 35e6f55c2a41..5d04da54faea 100644 > > > --- a/kernel/bpf/token.c > > > +++ b/kernel/bpf/token.c > > > @@ -7,11 +7,12 @@ > > > #include <linux/idr.h> > > > #include <linux/namei.h> > > > #include <linux/user_namespace.h> > > > +#include <linux/security.h> > > > > > > bool bpf_token_capable(const struct bpf_token *token, int cap) > > > { > > > /* BPF token allows ns_capable() level of capabilities */ > > > - if (token) { > > > + if (token && security_bpf_token_capable(token, cap) == 0) { > > > if (ns_capable(token->userns, cap)) > > > return true; > > > if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN)) > > > > We typically perform the capability based access controls prior to the > > LSM controls, meaning if we want to the token controls to work in a > > similar way we should do something like this: > > > > bool bpf_token_capable(...) > > { > > if (token) { > > if (ns_capable(token, cap) || > > (cap != ADMIN && ns_capable(token, ADMIN))) > > return security_bpf_token_capable(token, cap); > > } > > return capable(cap) || (cap != ADMIN && capable(...)) > > } > > yep, makes sense, I changed it as you suggested above Thanks again. > > > @@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token) > > > > > > static void bpf_token_free(struct bpf_token *token) > > > { > > > + security_bpf_token_free(token); > > > put_user_ns(token->userns); > > > kvfree(token); > > > } > > > @@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr) > > > token->allowed_progs = mnt_opts->delegate_progs; > > > token->allowed_attachs = mnt_opts->delegate_attachs; > > > > > > + err = security_bpf_token_create(token, attr, &path); > > > + if (err) > > > + goto out_token; > > > + > > > fd = get_unused_fd_flags(O_CLOEXEC); > > > if (fd < 0) { > > > err = fd; > > > @@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) > > > { > > > if (!token) > > > return false; > > > - > > > - return token->allowed_cmds & (1ULL << cmd); > > > + if (!(token->allowed_cmds & (1ULL << cmd))) > > > + return false; > > > + return security_bpf_token_allow_cmd(token, cmd) == 0; > > > > I'm not sure how much it really matters, but someone might prefer > > the '!!' approach/style over '== 0'. > > it would have to be !security_bpf_token_cmd(), right? Yeah :P In most, although definitely not all, kernel functions when something returns 0 we consider that the positive/success case, with non-zero values being some sort of failure. I must have defaulted to that logic here, but you are correct that just a single negation would be needed here. > And that single > negation is just very confusing when dealing with int-returning > function. I find it much easier to make sure the logic is correct when > we have explicit `== 0`. That's fine, it's something I've seen mentioned over the years and thought I might offer it as a comment. I can read either approach just fine :) Anyway, with the other changes mentioned above, e.g. naming and permission ordering, feel free to add my ACK. Acked-by: Paul Moore <paul@paul-moore.com>
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 975aa7790d51..8344c050e615 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1585,6 +1585,9 @@ struct bpf_token { u64 allowed_maps; u64 allowed_progs; u64 allowed_attachs; +#ifdef CONFIG_SECURITY + void *security; +#endif }; struct bpf_struct_ops_value; diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 795d3860c302..da4c4ef94140 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -404,6 +404,11 @@ LSM_HOOK(void, LSM_RET_VOID, bpf_map_free, struct bpf_map *map) LSM_HOOK(int, 0, bpf_prog_load, struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token) LSM_HOOK(void, LSM_RET_VOID, bpf_prog_free, struct bpf_prog *prog) +LSM_HOOK(int, 0, bpf_token_create, struct bpf_token *token, union bpf_attr *attr, + struct path *path) +LSM_HOOK(void, LSM_RET_VOID, bpf_token_free, struct bpf_token *token) +LSM_HOOK(int, 0, bpf_token_allow_cmd, const struct bpf_token *token, enum bpf_cmd cmd) +LSM_HOOK(int, 0, bpf_token_capable, const struct bpf_token *token, int cap) #endif /* CONFIG_BPF_SYSCALL */ LSM_HOOK(int, 0, locked_down, enum lockdown_reason what) diff --git a/include/linux/security.h b/include/linux/security.h index 08fd777cbe94..1d6edbf45d1c 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -60,6 +60,7 @@ struct fs_parameter; enum fs_value_type; struct watch; struct watch_notification; +enum bpf_cmd; /* Default (no) options for the capable function */ #define CAP_OPT_NONE 0x0 @@ -2031,6 +2032,11 @@ extern void security_bpf_map_free(struct bpf_map *map); extern int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, struct bpf_token *token); extern void security_bpf_prog_free(struct bpf_prog *prog); +extern int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, + struct path *path); +extern void security_bpf_token_free(struct bpf_token *token); +extern int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd); +extern int security_bpf_token_capable(const struct bpf_token *token, int cap); #else static inline int security_bpf(int cmd, union bpf_attr *attr, unsigned int size) @@ -2065,6 +2071,25 @@ static inline int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr * static inline void security_bpf_prog_free(struct bpf_prog *prog) { } + +static inline int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, + struct path *path) +{ + return 0; +} + +static inline void security_bpf_token_free(struct bpf_token *token) +{ } + +static inline int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) +{ + return 0; +} + +static inline int security_bpf_token_capable(const struct bpf_token *token, int cap) +{ + return 0; +} #endif /* CONFIG_SECURITY */ #endif /* CONFIG_BPF_SYSCALL */ diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c index 9e4e615f11eb..2b491e07485d 100644 --- a/kernel/bpf/bpf_lsm.c +++ b/kernel/bpf/bpf_lsm.c @@ -265,6 +265,10 @@ BTF_ID(func, bpf_lsm_bpf_map_free) BTF_ID(func, bpf_lsm_bpf_prog) BTF_ID(func, bpf_lsm_bpf_prog_load) BTF_ID(func, bpf_lsm_bpf_prog_free) +BTF_ID(func, bpf_lsm_bpf_token_create) +BTF_ID(func, bpf_lsm_bpf_token_free) +BTF_ID(func, bpf_lsm_bpf_token_allow_cmd) +BTF_ID(func, bpf_lsm_bpf_token_capable) BTF_ID(func, bpf_lsm_bprm_check_security) BTF_ID(func, bpf_lsm_bprm_committed_creds) BTF_ID(func, bpf_lsm_bprm_committing_creds) diff --git a/kernel/bpf/token.c b/kernel/bpf/token.c index 35e6f55c2a41..5d04da54faea 100644 --- a/kernel/bpf/token.c +++ b/kernel/bpf/token.c @@ -7,11 +7,12 @@ #include <linux/idr.h> #include <linux/namei.h> #include <linux/user_namespace.h> +#include <linux/security.h> bool bpf_token_capable(const struct bpf_token *token, int cap) { /* BPF token allows ns_capable() level of capabilities */ - if (token) { + if (token && security_bpf_token_capable(token, cap) == 0) { if (ns_capable(token->userns, cap)) return true; if (cap != CAP_SYS_ADMIN && ns_capable(token->userns, CAP_SYS_ADMIN)) @@ -28,6 +29,7 @@ void bpf_token_inc(struct bpf_token *token) static void bpf_token_free(struct bpf_token *token) { + security_bpf_token_free(token); put_user_ns(token->userns); kvfree(token); } @@ -172,6 +174,10 @@ int bpf_token_create(union bpf_attr *attr) token->allowed_progs = mnt_opts->delegate_progs; token->allowed_attachs = mnt_opts->delegate_attachs; + err = security_bpf_token_create(token, attr, &path); + if (err) + goto out_token; + fd = get_unused_fd_flags(O_CLOEXEC); if (fd < 0) { err = fd; @@ -216,8 +222,9 @@ bool bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) { if (!token) return false; - - return token->allowed_cmds & (1ULL << cmd); + if (!(token->allowed_cmds & (1ULL << cmd))) + return false; + return security_bpf_token_allow_cmd(token, cmd) == 0; } bool bpf_token_allow_map_type(const struct bpf_token *token, enum bpf_map_type type) diff --git a/security/security.c b/security/security.c index 745edcd7b04b..abcee81b9cea 100644 --- a/security/security.c +++ b/security/security.c @@ -5201,6 +5201,55 @@ int security_bpf_prog_load(struct bpf_prog *prog, union bpf_attr *attr, return call_int_hook(bpf_prog_load, 0, prog, attr, token); } +/** + * security_bpf_token_create() - Check if creating of BPF token is allowed + * @token: BPF token object + * @attr: BPF syscall attributes used to create BPF token + * @path: path pointing to BPF FS mount point from which BPF token is created + * + * Do a check when the kernel instantiates a new BPF token object from BPF FS + * instance. This is also the point where LSM blob can be allocated for LSMs. + * + * Return: Returns 0 on success, error on failure. + */ +int security_bpf_token_create(struct bpf_token *token, union bpf_attr *attr, + struct path *path) +{ + return call_int_hook(bpf_token_create, 0, token, attr, path); +} + +/** + * security_bpf_token_allow_cmd() - Check if BPF token is allowed to delegate + * requested BPF syscall command + * @token: BPF token object + * @cmd: BPF syscall command requested to be delegated by BPF token + * + * Do a check when the kernel decides whether provided BPF token should allow + * delegation of requested BPF syscall command. + * + * Return: Returns 0 on success, error on failure. + */ +int security_bpf_token_allow_cmd(const struct bpf_token *token, enum bpf_cmd cmd) +{ + return call_int_hook(bpf_token_allow_cmd, 0, token, cmd); +} + +/** + * security_bpf_token_capable() - Check if BPF token is allowed to delegate + * requested BPF-related capability + * @token: BPF token object + * @cap: capabilities requested to be delegated by BPF token + * + * Do a check when the kernel decides whether provided BPF token should allow + * delegation of requested BPF-related capabilities. + * + * Return: Returns 0 on success, error on failure. + */ +int security_bpf_token_capable(const struct bpf_token *token, int cap) +{ + return call_int_hook(bpf_token_capable, 0, token, cap); +} + /** * security_bpf_map_free() - Free a bpf map's LSM blob * @map: bpf map @@ -5222,6 +5271,17 @@ void security_bpf_prog_free(struct bpf_prog *prog) { call_void_hook(bpf_prog_free, prog); } + +/** + * security_bpf_token_free() - Free a BPF token's LSM blob + * @token: BPF token struct + * + * Clean up the security information stored inside BPF token. + */ +void security_bpf_token_free(struct bpf_token *token) +{ + call_void_hook(bpf_token_free, token); +} #endif /* CONFIG_BPF_SYSCALL */ /**
Wire up bpf_token_create and bpf_token_free LSM hooks, which allow to allocate LSM security blob (we add `void *security` field to struct bpf_token for that), but also control who can instantiate BPF token. This follows existing pattern for BPF map and BPF prog. Also add security_bpf_token_allow_cmd() and security_bpf_token_capable() LSM hooks that allow LSM implementation to control and negate (if necessary) BPF token's delegation of a specific bpf_cmd and capability, respectively. Signed-off-by: Andrii Nakryiko <andrii@kernel.org> --- include/linux/bpf.h | 3 ++ include/linux/lsm_hook_defs.h | 5 +++ include/linux/security.h | 25 +++++++++++++++ kernel/bpf/bpf_lsm.c | 4 +++ kernel/bpf/token.c | 13 ++++++-- security/security.c | 60 +++++++++++++++++++++++++++++++++++ 6 files changed, 107 insertions(+), 3 deletions(-)