Re: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM

[KP Singh]

On 10-Feb 19:12, Alexei Starovoitov wrote:
> On Thu, Jan 23, 2020 at 07:24:34AM -0800, KP Singh wrote:
> > +#define CALL_BPF_LSM_INT_HOOKS(FUNC, ...) ({			\
> > +	int _ret = 0;						\
> > +	do {							\
> > +		struct security_hook_list *P;			\
> > +		int _idx;					\
> > +								\
> > +		if (hlist_empty(&bpf_lsm_hook_heads.FUNC))	\
> > +			break;					\
> > +								\
> > +		_idx = bpf_lsm_srcu_read_lock();		\
> > +								\
> > +		hlist_for_each_entry(P,				\
> > +			&bpf_lsm_hook_heads.FUNC, list) {	\
> > +			_ret = P->hook.FUNC(__VA_ARGS__);		\
> > +			if (_ret && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> > +				break;				\
> > +		}						\
> > +		bpf_lsm_srcu_read_unlock(_idx);			\
> > +	} while (0);						\
> > +	IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? _ret : 0;	\
> > +})
> 
> This extra CONFIG_SECURITY_BPF_ENFORCE doesn't make sense to me.

We found it easier to debug the programs if enforcement is disabled.
But we can remove this option if you feel strongly about it.

> Why do all the work for bpf-lsm and ignore return code? Such framework already
> exists. For audit only case the user could have kprobed security_*() in
> security/security.c and had access to exactly the same data. There is no need
> in any of these patches if audit the only use case.
> Obviously bpf-lsm has to be capable of making go/no-go decision, so
> my preference is to drop this extra kconfig knob.
> I think the agreement seen in earlier comments in this thread that the prefered
> choice is to always have bpf-based lsm to be equivalent to LSM_ORDER_LAST. In
> such case how about using bpf-trampoline fexit logic instead?

Even if the BPF LSM is LSM_ORDER_LAST this is still different from
adding a trampoline to the exit of the security hooks for a few other
reasons.

> Pros:
> - no changes to security/ directory

* We do need to initialize the BPF LSM as a proper LSM (i.e. in
  security/bpf) because it needs access to security blobs. This is
  only possible statically for now as they should be set after boot
  time to provide guarantees to any helper that uses information in
  security blobs. I have proposed how we could make this dynamic as a
  discussion topic for the BPF conference:

    https://lore.kernel.org/bpf/20200127171516.GA2710@xxxxxxxxxxxx

  As you can see from some of the prototype use cases e.g:

    https://github.com/sinkap/linux-krsi/blob/patch/v1/examples/samples/bpf/lsm_detect_exec_unlink.c

  that they do rely on security blobs and that they are key in doing
  meaningful security analysis.

* When using the semantic provided by fexit, the BPF LSM program will
  always be executed and will be able to override / clobber the
  decision of LSMs which appear before it in the ordered list. This
  semantic is very different from what we currently have (i.e. the BPF
  LSM hook is only called if all the other LSMs allow the action) and
  seems to be bypassing the LSM framework.

* Not all security_* wrappers simply call the attached hooks and return
  their exit code and not all of them pass the same arguments to the
  hook e.g. security_bprm_check, security_file_open,
  security_task_alloc to just name a few. Illustrating this further
  using security_task_alloc as an example:

	rc = call_int_hook(task_alloc, 0, task, clone_flags);
	if (unlikely(rc))
		security_task_free(task);
	return rc;

Which means we would leak task_structs in this case. While
call_int_hook is sort of equivalent to the fexit trampoline for most
hooks, it's not really the case for some (quite important) LSM hooks.

- KP

> - no changes to call_int_hook() macro
> - patches 4, 5, 6 no longer necessary
> - when security is off all security_*() hooks do single
>   if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) check.
>   With patch 4 there will two such checks. Tiny perf penalty.
>   With fexit approach there will be no extra check.
> - fexit approach is fast even on kernels compiled with retpoline, since
>   its using direct calls
> Cons:
> - bpf trampoline so far is x86 only and arm64 support is wip
> 
> By plugging into fexit I'm proposing to let bpf-lsm prog type modify return
> value. Currently bpf-fexit prog type has read-only access to it. Adding write
> access is a straightforward verifier change. The bpf progs from patch 9 will
> still look exactly the same way:
> SEC("lsm/file_mprotect")
> int BPF_PROG(mprotect_audit, struct vm_area_struct *vma,
>             unsigned long reqprot, unsigned long prot) { ... }
> The difference that libbpf will be finding btf_id of security_file_mprotect()
> function and adding fexit trampoline to it instead of going via
> security_list_options and its own lsm_hook_idx uapi. I think reusing existing
> tracing facilities to attach and multiplex multiple programs is cleaner. More
> code reuse. Unified testing of lsm and tracing, etc.