On Fri, Oct 28, 2022 at 6:55 PM Roberto Sassu
<roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>
> From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
>
> BPF LSM defines a bpf_lsm_*() function for each LSM hook, so that
> security modules can define their own implementation for the desired hooks.
>
> Unfortunately, BPF LSM does not restrict which values security modules can
> return (for non-void LSM hooks). Security modules might follow the
> conventions stated in include/linux/lsm_hooks.h, or put arbitrary values.
>
> This could cause big troubles, as the kernel is not ready to handle
> possibly malicious return values from LSMs. Until now, it was not the
I am not sure I would call this malicious. This would be incorrect, if
someone is writing a BPF LSM program they already have the powers
to willingly do a lot of malicious stuff.
It's about unknowingly returning values that can break the system.
> case, as each LSM is carefully reviewed and it won't be accepted if it
> does not meet the return value conventions.
>
> The biggest problem is when an LSM returns a positive value, instead of a
> negative value, as it could be converted to a pointer. Since such pointer
> escapes the IS_ERR() check, its use later in the code can cause
> unpredictable consequences (e.g. invalid memory access).
>
> Another problem is returning zero when an LSM is supposed to have done some
> operations. For example, the inode_init_security hook expects that their
> implementations return zero only if they set the name and value of the new
> xattr to be added to the new inode. Otherwise, other kernel subsystems
> might encounter unexpected conditions leading to a crash (e.g.
> evm_protected_xattr_common() getting NULL as argument).
>
> Finally, there are LSM hooks which are supposed to return just one as
> positive value, or non-negative values. Also in these cases, although it
> seems less critical, it is safer to return to callers of the LSM
> infrastructure more precisely what they expect.
>
> As eBPF allows code outside the kernel to run, it is its responsibility
> to ensure that only expected values are returned to LSM infrastructure
> callers.
>
> Create four new BTF ID sets, respectively for hooks that can return
> positive values, only one as positive value, that cannot return zero, and
> that cannot return negative values. Create also corresponding functions to
> check if the hook a security module is attached to belongs to one of the
> defined sets.
>
> Finally, check in the eBPF verifier the value returned by security modules
> for each attached LSM hook, and return -EINVAL (the security module cannot
> run) if the hook implementation does not satisfy the hook return value
> policy.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> ---
> include/linux/bpf_lsm.h | 24 ++++++++++++++++++
> kernel/bpf/bpf_lsm.c | 56 +++++++++++++++++++++++++++++++++++++++++
> kernel/bpf/verifier.c | 35 +++++++++++++++++++++++---
> 3 files changed, 112 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 4bcf76a9bb06..cd38aca4cfc0 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -28,6 +28,10 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> const struct bpf_prog *prog);
>
> bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> +bool bpf_lsm_can_ret_pos_value(u32 btf_id);
> +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id);
> +bool bpf_lsm_cannot_ret_zero(u32 btf_id);
> +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id);
>
This does not need to be exported to the rest of the kernel. Please
have this logic in bpf_lsm.c and export a single verify function.
Also, these really don't need to be such scattered logic, Could we
somehow encode this into the LSM_HOOK definition?
> static inline struct bpf_storage_blob *bpf_inode(
> const struct inode *inode)
> @@ -51,6 +55,26 @@ static inline bool bpf_lsm_is_sleepable_hook(u32 btf_id)
> return false;
> }
>
> +static inline bool bpf_lsm_can_ret_pos_value(u32 btf_id)
> +{
> + return false;
> +}
> +
> +static inline bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id)
> +{
> + return false;
> +}
> +
> +static inline bool bpf_lsm_cannot_ret_zero(u32 btf_id)
> +{
> + return false;
> +}
> +
> +static inline bool bpf_lsm_cannot_ret_neg_value(u32 btf_id)
> +{
> + return false;
> +}
> +
> static inline int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> const struct bpf_prog *prog)
> {
> diff --git a/kernel/bpf/bpf_lsm.c b/kernel/bpf/bpf_lsm.c
> index d6c9b3705f24..3dcb70b2f978 100644
> --- a/kernel/bpf/bpf_lsm.c
> +++ b/kernel/bpf/bpf_lsm.c
> @@ -348,6 +348,62 @@ bool bpf_lsm_is_sleepable_hook(u32 btf_id)
> return btf_id_set_contains(&sleepable_lsm_hooks, btf_id);
> }
>
> +/* The set of hooks which are allowed to return a positive value. */
> +BTF_SET_START(pos_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_inode_getsecurity)
> +BTF_ID(func, bpf_lsm_inode_listsecurity)
> +BTF_ID(func, bpf_lsm_inode_need_killpriv)
> +BTF_ID(func, bpf_lsm_inode_copy_up_xattr)
> +BTF_ID(func, bpf_lsm_getprocattr)
> +BTF_ID(func, bpf_lsm_setprocattr)
> +BTF_ID(func, bpf_lsm_xfrm_state_pol_flow_match)
> +BTF_ID(func, bpf_lsm_key_getsecurity)
> +BTF_ID(func, bpf_lsm_ismaclabel)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_ID(func, bpf_lsm_audit_rule_match)
> +BTF_SET_END(pos_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_can_ret_pos_value(u32 btf_id)
> +{
> + return btf_id_set_contains(&pos_ret_value_lsm_hooks, btf_id);
> +}
> +
> +BTF_SET_START(one_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_inode_copy_up_xattr)
> +BTF_ID(func, bpf_lsm_xfrm_state_pol_flow_match)
> +BTF_ID(func, bpf_lsm_ismaclabel)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_ID(func, bpf_lsm_audit_rule_match)
> +BTF_SET_END(one_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id)
> +{
> + return btf_id_set_contains(&one_ret_value_lsm_hooks, btf_id);
> +}
> +
> +/* The set of hooks which are not allowed to return zero. */
> +BTF_SET_START(not_zero_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_inode_init_security)
> +BTF_SET_END(not_zero_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_cannot_ret_zero(u32 btf_id)
> +{
> + return btf_id_set_contains(¬_zero_ret_value_lsm_hooks, btf_id);
> +}
> +
> +/* The set of hooks which are not allowed to return a negative value. */
> +BTF_SET_START(not_neg_ret_value_lsm_hooks)
> +BTF_ID(func, bpf_lsm_vm_enough_memory)
> +BTF_ID(func, bpf_lsm_audit_rule_known)
> +BTF_SET_END(not_neg_ret_value_lsm_hooks)
> +
> +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id)
> +{
> + return btf_id_set_contains(¬_neg_ret_value_lsm_hooks, btf_id);
> +}
> +
> const struct bpf_prog_ops lsm_prog_ops = {
> };
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7f0a9f6cb889..099c1bf88fed 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -10623,9 +10623,38 @@ static int check_return_code(struct bpf_verifier_env *env)
>
> case BPF_PROG_TYPE_LSM:
> if (env->prog->expected_attach_type != BPF_LSM_CGROUP) {
> - /* Regular BPF_PROG_TYPE_LSM programs can return
> - * any value.
> - */
> + /* < 0 */
> + if (tnum_in(tnum_range((u64)(~0) << 31, (u64)(~0)), reg->var_off)) {
> + if (bpf_lsm_cannot_ret_neg_value(env->prog->aux->attach_btf_id)) {
> + verbose(env, "Invalid R0, cannot return negative value\n");
> + return -EINVAL;
> + }
> + /* = 0 */
> + } else if (tnum_equals_const(reg->var_off, 0)) {
> + if (bpf_lsm_cannot_ret_zero(env->prog->aux->attach_btf_id)) {
> + verbose(env, "Invalid R0, cannot return zero value\n");
> + return -EINVAL;
> + }
> + /* = 1 */
> + } else if (tnum_equals_const(reg->var_off, 1)) {
> + if (!bpf_lsm_can_ret_pos_value(env->prog->aux->attach_btf_id)) {
> + verbose(env, "Invalid R0, cannot return positive value\n");
> + return -EINVAL;
> + }
> + /* > 1 */
> + } else {
> + if (!bpf_lsm_can_ret_pos_value(env->prog->aux->attach_btf_id)) {
> + verbose(env, "Invalid R0, cannot return positive value\n");
> + return -EINVAL;
> + }
> +
> + if (bpf_lsm_can_ret_only_one_as_pos_value(env->prog->aux->attach_btf_id)) {
> + verbose(env,
> + "Invalid R0, can return only one as positive value\n");
> + return -EINVAL;
> + }
> + }
> +
> return 0;
> }
> if (!env->prog->aux->attach_func_proto->type) {
> --
> 2.25.1
>
