Re: [RFC v1 14/14] krsi: Pin arg pages only when needed
On 9/15/19 2:40 AM, KP Singh wrote: > On 15-Sep 00:33, Yonghong Song wrote: >> >> >> On 9/10/19 12:55 PM, KP Singh wrote: >>> From: KP Singh >>> >>> Adds a callback which is called when a new program is attached >>> to a hook. The callback registered by the process_exection hook >>> checks if a program that has calls to a helper that requires pages to >>> be pinned (eg. krsi_get_env_var). >>> >>> Signed-off-by: KP Singh >>> --- >>>include/linux/krsi.h | 1 + >>>security/krsi/include/hooks.h | 5 ++- >>>security/krsi/include/krsi_init.h | 7 >>>security/krsi/krsi.c | 62 --- >>>security/krsi/ops.c | 10 - >>>5 files changed, 77 insertions(+), 8 deletions(-) >>> >>> diff --git a/include/linux/krsi.h b/include/linux/krsi.h >>> index c7d1790d0c1f..e443d0309764 100644 >>> --- a/include/linux/krsi.h >>> +++ b/include/linux/krsi.h >>> @@ -7,6 +7,7 @@ >>> >>>#ifdef CONFIG_SECURITY_KRSI >>>int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog); >>> +extern const struct bpf_func_proto krsi_get_env_var_proto; >>>#else >>>static inline int krsi_prog_attach(const union bpf_attr *attr, >>>struct bpf_prog *prog) >>> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h >>> index e070c452b5de..38293125ff99 100644 >>> --- a/security/krsi/include/hooks.h >>> +++ b/security/krsi/include/hooks.h >>> @@ -8,7 +8,7 @@ >>> * >>> * Format: >>> * >>> - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN) >>> + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK) >>> * >>> * KRSI adds one layer of indirection between the name of the hook and >>> the name >>> * it exposes to the userspace in Security FS to prevent the userspace >>> from >>> @@ -18,4 +18,5 @@ >>>KRSI_HOOK_INIT(PROCESS_EXECUTION, >>>process_execution, >>>bprm_check_security, >>> - krsi_process_execution) >>> + krsi_process_execution, >>> + krsi_process_execution_cb) >>> diff --git a/security/krsi/include/krsi_init.h >>> b/security/krsi/include/krsi_init.h >>> index 6152847c3b08..99801d5b273a 100644 >>> --- a/security/krsi/include/krsi_init.h >>> +++ b/security/krsi/include/krsi_init.h >>> @@ -31,6 +31,8 @@ struct krsi_ctx { >>> }; >>>}; >>> >>> +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *); >>> + >>>/* >>> * The LSM creates one file per hook. >>> * >>> @@ -61,6 +63,11 @@ struct krsi_hook { >>> * The eBPF programs that are attached to this hook. >>> */ >>> struct bpf_prog_array __rcu *progs; >>> + /* >>> +* The attach callback is called before a new program is attached >>> +* to the hook and is passed the updated bpf_prog_array as an argument. >>> +*/ >>> + krsi_prog_attach_t attach_callback; >>>}; >>> >>>extern struct krsi_hook krsi_hooks_list[]; >>> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c >>> index 00a7150c1b22..a4443d7aa150 100644 >>> --- a/security/krsi/krsi.c >>> +++ b/security/krsi/krsi.c >>> @@ -5,15 +5,65 @@ >>>#include >>>#include >>>#include >>> +#include >>>#include >>> >>>#include "krsi_init.h" >>> >>> +/* >>> + * need_arg_pages is only updated in bprm_check_security_cb >>> + * when a mutex on krsi_hook for bprm_check_security is already >>> + * held. need_arg_pages avoids pinning pages when no program >>> + * that needs them is attached to the hook. >>> + */ >>> +static bool need_arg_pages; >>> + >>> +/* >>> + * Checks if the instruction is a BPF_CALL to an eBPF helper located >>> + * at the given address. >>> + */ >>> +static inline bool bpf_is_call_to_func(struct bpf_insn *insn, >>> + void *func_addr) >>> +{ >>> + u8 opcode = BPF_OP(insn->code); >>> + >>> + if (opcode != BPF_CALL) >>> + return false; >>> + >>> + if (insn->src_reg == BPF_PSEUDO_CALL) >>> + return false; >>> + >>> + /* >>> +* The BPF verifier updates the value of insn->imm from the >>> +* enum bpf_func_id to the offset of the address of helper >>> +* from the __bpf_call_base. >>> +*/ >>> + return __bpf_call_base + insn->imm == func_addr; >> >> In how many cases, krsi program does not have krsi_get_env_var() helper? > > It depends, if the user does not choose to use log environment > variables or use the the value as a part of their MAC policy, the > pinning of the pages is not needed. Thanks. I just want to know whether we want to optimize such cases. I am not a security expert, so I am okay with whatever decision you made. > > Also, the pinning is needed since eBPF helpers cannot run a non-atomic > context. It would not be needed if "sleepable eBPF" becomes a thing. Indeed. 'sleepable BPF' might be also a good thing for realtime linux. Some works need to address bpf area spin lock, per
Re: [RFC v1 14/14] krsi: Pin arg pages only when needed
On 15-Sep 00:33, Yonghong Song wrote: > > > On 9/10/19 12:55 PM, KP Singh wrote: > > From: KP Singh > > > > Adds a callback which is called when a new program is attached > > to a hook. The callback registered by the process_exection hook > > checks if a program that has calls to a helper that requires pages to > > be pinned (eg. krsi_get_env_var). > > > > Signed-off-by: KP Singh > > --- > > include/linux/krsi.h | 1 + > > security/krsi/include/hooks.h | 5 ++- > > security/krsi/include/krsi_init.h | 7 > > security/krsi/krsi.c | 62 --- > > security/krsi/ops.c | 10 - > > 5 files changed, 77 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/krsi.h b/include/linux/krsi.h > > index c7d1790d0c1f..e443d0309764 100644 > > --- a/include/linux/krsi.h > > +++ b/include/linux/krsi.h > > @@ -7,6 +7,7 @@ > > > > #ifdef CONFIG_SECURITY_KRSI > > int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog); > > +extern const struct bpf_func_proto krsi_get_env_var_proto; > > #else > > static inline int krsi_prog_attach(const union bpf_attr *attr, > >struct bpf_prog *prog) > > diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h > > index e070c452b5de..38293125ff99 100644 > > --- a/security/krsi/include/hooks.h > > +++ b/security/krsi/include/hooks.h > > @@ -8,7 +8,7 @@ > >* > >* Format: > >* > > - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN) > > + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK) > >* > >* KRSI adds one layer of indirection between the name of the hook and > > the name > >* it exposes to the userspace in Security FS to prevent the userspace > > from > > @@ -18,4 +18,5 @@ > > KRSI_HOOK_INIT(PROCESS_EXECUTION, > >process_execution, > >bprm_check_security, > > - krsi_process_execution) > > + krsi_process_execution, > > + krsi_process_execution_cb) > > diff --git a/security/krsi/include/krsi_init.h > > b/security/krsi/include/krsi_init.h > > index 6152847c3b08..99801d5b273a 100644 > > --- a/security/krsi/include/krsi_init.h > > +++ b/security/krsi/include/krsi_init.h > > @@ -31,6 +31,8 @@ struct krsi_ctx { > > }; > > }; > > > > +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *); > > + > > /* > >* The LSM creates one file per hook. > >* > > @@ -61,6 +63,11 @@ struct krsi_hook { > > * The eBPF programs that are attached to this hook. > > */ > > struct bpf_prog_array __rcu *progs; > > + /* > > +* The attach callback is called before a new program is attached > > +* to the hook and is passed the updated bpf_prog_array as an argument. > > +*/ > > + krsi_prog_attach_t attach_callback; > > }; > > > > extern struct krsi_hook krsi_hooks_list[]; > > diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c > > index 00a7150c1b22..a4443d7aa150 100644 > > --- a/security/krsi/krsi.c > > +++ b/security/krsi/krsi.c > > @@ -5,15 +5,65 @@ > > #include > > #include > > #include > > +#include > > #include > > > > #include "krsi_init.h" > > > > +/* > > + * need_arg_pages is only updated in bprm_check_security_cb > > + * when a mutex on krsi_hook for bprm_check_security is already > > + * held. need_arg_pages avoids pinning pages when no program > > + * that needs them is attached to the hook. > > + */ > > +static bool need_arg_pages; > > + > > +/* > > + * Checks if the instruction is a BPF_CALL to an eBPF helper located > > + * at the given address. > > + */ > > +static inline bool bpf_is_call_to_func(struct bpf_insn *insn, > > + void *func_addr) > > +{ > > + u8 opcode = BPF_OP(insn->code); > > + > > + if (opcode != BPF_CALL) > > + return false; > > + > > + if (insn->src_reg == BPF_PSEUDO_CALL) > > + return false; > > + > > + /* > > +* The BPF verifier updates the value of insn->imm from the > > +* enum bpf_func_id to the offset of the address of helper > > +* from the __bpf_call_base. > > +*/ > > + return __bpf_call_base + insn->imm == func_addr; > > In how many cases, krsi program does not have krsi_get_env_var() helper? It depends, if the user does not choose to use log environment variables or use the the value as a part of their MAC policy, the pinning of the pages is not needed. Also, the pinning is needed since eBPF helpers cannot run a non-atomic context. It would not be needed if "sleepable eBPF" becomes a thing. - KP > > > +} > > + > > +static int krsi_process_execution_cb(struct bpf_prog_array *array) > > +{ > > + struct bpf_prog_array_item *item = array->items; > > + struct bpf_prog *p; > > + const struct bpf_func_proto *proto = _get_env_var_proto; > > + int i; > > + > > + while ((p = READ_ONCE(item->prog))) { > > + for (i = 0; i <
Re: [RFC v1 14/14] krsi: Pin arg pages only when needed
On 9/10/19 12:55 PM, KP Singh wrote: > From: KP Singh > > Adds a callback which is called when a new program is attached > to a hook. The callback registered by the process_exection hook > checks if a program that has calls to a helper that requires pages to > be pinned (eg. krsi_get_env_var). > > Signed-off-by: KP Singh > --- > include/linux/krsi.h | 1 + > security/krsi/include/hooks.h | 5 ++- > security/krsi/include/krsi_init.h | 7 > security/krsi/krsi.c | 62 --- > security/krsi/ops.c | 10 - > 5 files changed, 77 insertions(+), 8 deletions(-) > > diff --git a/include/linux/krsi.h b/include/linux/krsi.h > index c7d1790d0c1f..e443d0309764 100644 > --- a/include/linux/krsi.h > +++ b/include/linux/krsi.h > @@ -7,6 +7,7 @@ > > #ifdef CONFIG_SECURITY_KRSI > int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog); > +extern const struct bpf_func_proto krsi_get_env_var_proto; > #else > static inline int krsi_prog_attach(const union bpf_attr *attr, > struct bpf_prog *prog) > diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h > index e070c452b5de..38293125ff99 100644 > --- a/security/krsi/include/hooks.h > +++ b/security/krsi/include/hooks.h > @@ -8,7 +8,7 @@ >* >* Format: >* > - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN) > + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK) >* >* KRSI adds one layer of indirection between the name of the hook and the > name >* it exposes to the userspace in Security FS to prevent the userspace from > @@ -18,4 +18,5 @@ > KRSI_HOOK_INIT(PROCESS_EXECUTION, > process_execution, > bprm_check_security, > -krsi_process_execution) > +krsi_process_execution, > +krsi_process_execution_cb) > diff --git a/security/krsi/include/krsi_init.h > b/security/krsi/include/krsi_init.h > index 6152847c3b08..99801d5b273a 100644 > --- a/security/krsi/include/krsi_init.h > +++ b/security/krsi/include/krsi_init.h > @@ -31,6 +31,8 @@ struct krsi_ctx { > }; > }; > > +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *); > + > /* >* The LSM creates one file per hook. >* > @@ -61,6 +63,11 @@ struct krsi_hook { >* The eBPF programs that are attached to this hook. >*/ > struct bpf_prog_array __rcu *progs; > + /* > + * The attach callback is called before a new program is attached > + * to the hook and is passed the updated bpf_prog_array as an argument. > + */ > + krsi_prog_attach_t attach_callback; > }; > > extern struct krsi_hook krsi_hooks_list[]; > diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c > index 00a7150c1b22..a4443d7aa150 100644 > --- a/security/krsi/krsi.c > +++ b/security/krsi/krsi.c > @@ -5,15 +5,65 @@ > #include > #include > #include > +#include > #include > > #include "krsi_init.h" > > +/* > + * need_arg_pages is only updated in bprm_check_security_cb > + * when a mutex on krsi_hook for bprm_check_security is already > + * held. need_arg_pages avoids pinning pages when no program > + * that needs them is attached to the hook. > + */ > +static bool need_arg_pages; > + > +/* > + * Checks if the instruction is a BPF_CALL to an eBPF helper located > + * at the given address. > + */ > +static inline bool bpf_is_call_to_func(struct bpf_insn *insn, > +void *func_addr) > +{ > + u8 opcode = BPF_OP(insn->code); > + > + if (opcode != BPF_CALL) > + return false; > + > + if (insn->src_reg == BPF_PSEUDO_CALL) > + return false; > + > + /* > + * The BPF verifier updates the value of insn->imm from the > + * enum bpf_func_id to the offset of the address of helper > + * from the __bpf_call_base. > + */ > + return __bpf_call_base + insn->imm == func_addr; In how many cases, krsi program does not have krsi_get_env_var() helper? > +} > + > +static int krsi_process_execution_cb(struct bpf_prog_array *array) > +{ > + struct bpf_prog_array_item *item = array->items; > + struct bpf_prog *p; > + const struct bpf_func_proto *proto = _get_env_var_proto; > + int i; > + > + while ((p = READ_ONCE(item->prog))) { > + for (i = 0; i < p->len; i++) { > + if (bpf_is_call_to_func(>insnsi[i], proto->func)) > + need_arg_pages = true; > + } > + item++; > + } > + return 0; > +} > + > struct krsi_hook krsi_hooks_list[] = { > - #define KRSI_HOOK_INIT(TYPE, NAME, H, I) \ > + #define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \ > [TYPE] = { \ > .h_type = TYPE, \ > .name = #NAME, \ > + .attach_callback = CB, \ > }, >
[RFC v1 14/14] krsi: Pin arg pages only when needed
From: KP Singh Adds a callback which is called when a new program is attached to a hook. The callback registered by the process_exection hook checks if a program that has calls to a helper that requires pages to be pinned (eg. krsi_get_env_var). Signed-off-by: KP Singh --- include/linux/krsi.h | 1 + security/krsi/include/hooks.h | 5 ++- security/krsi/include/krsi_init.h | 7 security/krsi/krsi.c | 62 --- security/krsi/ops.c | 10 - 5 files changed, 77 insertions(+), 8 deletions(-) diff --git a/include/linux/krsi.h b/include/linux/krsi.h index c7d1790d0c1f..e443d0309764 100644 --- a/include/linux/krsi.h +++ b/include/linux/krsi.h @@ -7,6 +7,7 @@ #ifdef CONFIG_SECURITY_KRSI int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog); +extern const struct bpf_func_proto krsi_get_env_var_proto; #else static inline int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog) diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h index e070c452b5de..38293125ff99 100644 --- a/security/krsi/include/hooks.h +++ b/security/krsi/include/hooks.h @@ -8,7 +8,7 @@ * * Format: * - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN) + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK) * * KRSI adds one layer of indirection between the name of the hook and the name * it exposes to the userspace in Security FS to prevent the userspace from @@ -18,4 +18,5 @@ KRSI_HOOK_INIT(PROCESS_EXECUTION, process_execution, bprm_check_security, - krsi_process_execution) + krsi_process_execution, + krsi_process_execution_cb) diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h index 6152847c3b08..99801d5b273a 100644 --- a/security/krsi/include/krsi_init.h +++ b/security/krsi/include/krsi_init.h @@ -31,6 +31,8 @@ struct krsi_ctx { }; }; +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *); + /* * The LSM creates one file per hook. * @@ -61,6 +63,11 @@ struct krsi_hook { * The eBPF programs that are attached to this hook. */ struct bpf_prog_array __rcu *progs; + /* +* The attach callback is called before a new program is attached +* to the hook and is passed the updated bpf_prog_array as an argument. +*/ + krsi_prog_attach_t attach_callback; }; extern struct krsi_hook krsi_hooks_list[]; diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c index 00a7150c1b22..a4443d7aa150 100644 --- a/security/krsi/krsi.c +++ b/security/krsi/krsi.c @@ -5,15 +5,65 @@ #include #include #include +#include #include #include "krsi_init.h" +/* + * need_arg_pages is only updated in bprm_check_security_cb + * when a mutex on krsi_hook for bprm_check_security is already + * held. need_arg_pages avoids pinning pages when no program + * that needs them is attached to the hook. + */ +static bool need_arg_pages; + +/* + * Checks if the instruction is a BPF_CALL to an eBPF helper located + * at the given address. + */ +static inline bool bpf_is_call_to_func(struct bpf_insn *insn, + void *func_addr) +{ + u8 opcode = BPF_OP(insn->code); + + if (opcode != BPF_CALL) + return false; + + if (insn->src_reg == BPF_PSEUDO_CALL) + return false; + + /* +* The BPF verifier updates the value of insn->imm from the +* enum bpf_func_id to the offset of the address of helper +* from the __bpf_call_base. +*/ + return __bpf_call_base + insn->imm == func_addr; +} + +static int krsi_process_execution_cb(struct bpf_prog_array *array) +{ + struct bpf_prog_array_item *item = array->items; + struct bpf_prog *p; + const struct bpf_func_proto *proto = _get_env_var_proto; + int i; + + while ((p = READ_ONCE(item->prog))) { + for (i = 0; i < p->len; i++) { + if (bpf_is_call_to_func(>insnsi[i], proto->func)) + need_arg_pages = true; + } + item++; + } + return 0; +} + struct krsi_hook krsi_hooks_list[] = { - #define KRSI_HOOK_INIT(TYPE, NAME, H, I) \ + #define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \ [TYPE] = { \ .h_type = TYPE, \ .name = #NAME, \ + .attach_callback = CB, \ }, #include "hooks.h" #undef KRSI_HOOK_INIT @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm) .bprm = bprm, }; - ret = pin_arg_pages(_ctx); - if (ret < 0) - goto out_arg_pages; + if (READ_ONCE(need_arg_pages)) { + ret =