Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

[Roberto Sassu <roberto.sassu@huaweicloud.com>]

On Fri, 2022-11-18 at 09:44 +0100, Roberto Sassu wrote:
> On 11/16/2022 6:12 PM, Casey Schaufler wrote:
> > On 11/16/2022 7:47 AM, Roberto Sassu wrote:
> > > From: Roberto Sassu <roberto.sassu@huawei.com>
> > > 
> > > eBPF allows certain types of eBPF programs to modify the return value of
> > > the functions they attach to. This is used for example by BPF LSM to let
> > > security modules make their decision on LSM hooks.
> > > 
> > > The JITed code looks like the following:
> > > 
> > >      ret = bpf_lsm_inode_permission_impl1(); // from a security module
> > >      if (ret)
> > >          goto out;
> > > 
> > > ..
> > > 
> > >      ret = bpf_lsm_inode_permission_implN(); // from a security module
> > >      if (ret)
> > >          goto out;
> > > 
> > >      ret = bpf_lsm_inode_permission(); // in the kernel, returns DEFAULT
> > > out:
> > > 
> > > If ret is not zero, the attachment points of BPF LSM are not executed. For
> > > this reason, the return value check cannot be done there.
> > > 
> > > Instead, the idea is to use the LSM_HOOK() macro to define a per-hook check
> > > function.
> > > 
> > > Whenever an eBPF program attaches to an LSM hook, the eBPF verifier
> > > resolves the address of the check function (whose name is
> > > bpf_lsm_<hook name>_ret()) and adds a call to that function just after the
> > > out label. If the return value is illegal, the check function changes it
> > > back to the default value defined by the LSM infrastructure:
> > > 
> > > ..
> > > 
> > > out:
> > >      ret = bpf_lsm_inode_permission_ret(ret);
> > 
> > As I've mentioned elsewhere, the return value is a small part of
> > the problem you have with eBPF programs and the BPF LSM. Because
> > the LSM infrastructure is inconsistent with regard to return codes,
> > values returned in pointers and use of secids there is no uniform
> > mechanism that I can see to address the "legitimate return" problem.
> > 
> > Lets look at one of the ickyest interfaces we have, security_getprocattr().
> > It returns the size of a string that it has allocated. It puts the
> > pointer to the allocated buffer into a char **value that was passed to it.
> > If bpf_getprocattr() returns a positive number and sets value to NULL Bad
> > Things can happen. If the return value is greater than the size allocated
> > ditto. If it returns an error but allocates a string you get a memory leak.
> 
> I hope I understood how it works correctly, but you cannot modify 
> directly data accessible from a pointer provided as parameter by the LSM 
> hook you attach to. The pointer is treated as scalar value and the eBPF 
> verifier detects any attempt to dereference as an illegal access. The 
> only way to modify such data is through helpers that need to be properly 
> declared to be usable by eBPF programs.

I wanted to double check about accessing the LSM hook arguments from an
eBPF program. I checked what it could prevent to access them.

First, in kernel/bpf/btf.c:

if (!btf_type_is_struct(t)) {
	bpf_log(log,
		"func '%s' arg%d type %s is not a struct\n",

If the argument is not a struct, it is not accessible.


Second, if a btf_struct_access method has not been defined for a
structure, only read can be done (kernel/bpf/verifier.c):

if (env->ops->btf_struct_access) {
	ret = env->ops->btf_struct_access(...);
} else {
	if (atype != BPF_READ) {
		verbose(env, "only read is supported\n");
		return -EACCES;
	}

I found four:

net/bpf/bpf_dummy_struct_ops.c: .btf_struct_access =
bpf_dummy_ops_btf_struct_access,
net/core/filter.c:      .btf_struct_access      =
tc_cls_act_btf_struct_access,
net/core/filter.c:      .btf_struct_access      =
xdp_btf_struct_access,
net/ipv4/bpf_tcp_ca.c:  .btf_struct_access      =
bpf_tcp_ca_btf_struct_access,

Anything else?

Thanks

Roberto