Re: [PATCH bpf-next v5 5/7] bpf: lsm: Initialize the BPF LSM hooks

[Casey Schaufler]

On 3/23/2020 2:44 PM, Kees Cook wrote:
> On Mon, Mar 23, 2020 at 01:47:29PM -0700, Casey Schaufler wrote:
>> On 3/23/2020 12:44 PM, Kees Cook wrote:
>>> On Mon, Mar 23, 2020 at 05:44:13PM +0100, KP Singh wrote:
>>>> +/* Some LSM hooks do not have 0 as their default return values. Override the
>>>> + * __weak definitons generated by default for these hooks
>>> If you wanted to avoid this, couldn't you make the default return value
>>> part of lsm_hooks.h?
>>>
>>> e.g.:
>>>
>>> LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct inode *inode,
>>> 	 const char *name, void **buffer, bool alloc)
>> If you're going to do that you'll have to keep lsm_hooks.h and security.c
>> default values in sync somehow. Note that the four functions you've called
>> out won't be using call_int_hook() after the next round of stacking. I'm not
>> nixing the idea, I just don't want the default return for the security_
>> functions defined in two places.
> Yeah, I actually went looking for this after I sent the email, realizing
> that the defaults were also used in security.c. I've been pondering how
> to keep them from being duplicated. I'm working on some ideas.
>
> The four are:
>
> inode_getsecurity
> inode_setsecurity
> task_prctl
> xfrm_state_pol_flow_match
>
> None of these are already just calling call_int_hook(), but I assume
> they'll need further tweaks in the coming stacking.
>
> To leave things as open-code-able as possible while still benefiting
> from the macro consolidation, how about something like this:
>
> lsm_hook_names.h:
>
> LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity,
> 	 struct inode *inode, const char *name, void **buffer, bool alloc)
>
> ...
>
> security.c:
>
> #define LSM_RET_DEFAULT_void(DEFAULT, NAME)	/* */
> #define LSM_RET_DEFAULT_int(DEFAULT, NAME)
> 	static const int NAME#_default = (DEFAULT);
>
> #define LSM_HOOK(RET, DEFAULT, NAME, ...)	\
> 	LSM_RET_DEFAULT_#RET(DEFAULT, NAME)
> #include <linux/lsm_hook_names.h>
> #undef LSM_HOOK
> ...
>
> Then -EOPNOTSUPP is available as "inode_getsecurity_default":
>
> int security_inode_getsecurity(struct inode *inode, const char *name,
> 			       void **buffer, bool alloc)
> {
>         struct security_hook_list *hp;
>         int rc;
>
>         if (unlikely(IS_PRIVATE(inode)))
>                 return inode_getsecurity_default;
>         /*
>          * Only one module will provide an attribute with a given name.
>          */
>         hlist_for_each_entry(hp, &security_hook_heads.inode_getsecurity, list) {
>                 rc = hp->hook.inode_getsecurity(inode, name, buffer, alloc);
>                 if (rc != inode_getsecurity_default)
>                         return rc;
>         }
>         return inode_getsecurity_default;
> }
>
>
> On the other hand, it's only 4 non-default return codes, so maybe the
> sync burden isn't very high?

That's not too terrible, I suppose. What would you be thinking for
the calls that do use call_int_hook()?

	rc = call_int_hook(something, something_default, goodnesses);

or embedded in the macro:

	rc = call_int_hook(something, goodnesses);