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

[Casey Schaufler]

On 11/16/2022 9:55 AM, Alexei Starovoitov wrote:
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>> On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
>>> On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
>>> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>>>> +static bool is_ret_value_allowed(int ret, u32 ret_flags)
>>>> +{
>>>> +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
>>>> +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
>>>> +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
>>>> +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
>>>> +               return false;
>>>> +
>>>> +       return true;
>>>> +}
>>>> +
>>>>  /* For every LSM hook that allows attachment of BPF programs, declare a nop
>>>>   * function where a BPF program can be attached.
>>>>   */
>>>> @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
>>>>  #include <linux/lsm_hook_defs.h>
>>>>  #undef LSM_HOOK
>>>>
>>>> +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
>>>> +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
>>>> +{                                              \
>>>> +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
>>>> +}
>>>> +
>>>> +#include <linux/lsm_hook_defs.h>
>>>> +#undef LSM_HOOK
>>>> +
>>> because lsm hooks is mess of undocumented return values your
>>> "solution" is to add hundreds of noninline functions
>>> and hack the call into them in JITs ?!
>> I revisited the documentation and checked each LSM hook one by one.
>> Hopefully, I completed it correctly, but I would review again (others
>> are also welcome to do it).
>>
>> Not sure if there is a more efficient way. Do you have any idea?
>> Maybe we find a way to use only one check function (by reusing the
>> address of the attachment point?).
>>
>> Regarding the JIT approach, I didn't find a reliable solution for using
>> just the verifier. As I wrote to you, there could be the case where the
>> range can include positive values, despite the possible return values
>> are zero and -EACCES.
> Didn't you find that there are only 12 or so odd return cases.
> Maybe refactor some of them to something that the verifier can enforce
> and denylist the rest ?

Changing security_mumble() often requires changes in either VFS, audit or
networking code. Even simple changes can require extensive review and
difficult to obtain Acked-by's. It may be the correct approach, but it
won't be easy or quick.

> Also denylist those that Casey mentioned like security_secid_to_secctx ?

Identifying all the hooks that could be "dangerous" isn't an easy chore,
and some of the "dangerous" hooks are key to implementing classes of policy
and absolutely necessary for audit support.