On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov
[off-list ref] wrote:

On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
[off-list ref] wrote:

quoted

On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:

quoted

On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
[off-list ref] wrote:

quoted

+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 ?

+1

Also denylist those that Casey mentioned like security_secid_to_secctx ?

Just replied to Casey's comment and I agree, these hooks should be denylisted.