On 6/10/2024 2:17 AM, Paul Moore wrote:
On Sun, Jun 9, 2024 at 1:39 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
On 6/8/2024 6:54 AM, Alexei Starovoitov wrote:
On Sat, Jun 8, 2024 at 1:04 AM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
On 6/7/2024 5:53 AM, Paul Moore wrote:
On Thu, Apr 11, 2024 at 8:24 AM Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> wrote:
From: Xu Kuohai <xukuohai@xxxxxxxxxx>
Add macro LSM_RET_INT to annotate lsm hook return integer type and the
default return value, and the expected return range.
The LSM_RET_INT is declared as:
LSM_RET_INT(defval, min, max)
where
- defval is the default return value
- min and max indicate the expected return range is [min, max]
The return value range for each lsm hook is taken from the description
in security/security.c.
The expanded result of LSM_RET_INT is not changed, and the compiled
product is not changed.
Signed-off-by: Xu Kuohai <xukuohai@xxxxxxxxxx>
---
include/linux/lsm_hook_defs.h | 591 +++++++++++++++++-----------------
include/linux/lsm_hooks.h | 6 -
kernel/bpf/bpf_lsm.c | 10 +
security/security.c | 1 +
4 files changed, 313 insertions(+), 295 deletions(-)
...
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index 334e00efbde4..708f515ffbf3 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -18,435 +18,448 @@
* The macro LSM_HOOK is used to define the data structures required by
* the LSM framework using the pattern:
*
- * LSM_HOOK(<return_type>, <default_value>, <hook_name>, args...)
+ * LSM_HOOK(<return_type>, <return_description>, <hook_name>, args...)
*
* struct security_hook_heads {
- * #define LSM_HOOK(RET, DEFAULT, NAME, ...) struct hlist_head NAME;
+ * #define LSM_HOOK(RET, RETVAL_DESC, NAME, ...) struct hlist_head NAME;
* #include <linux/lsm_hook_defs.h>
* #undef LSM_HOOK
* };
*/
-LSM_HOOK(int, 0, binder_set_context_mgr, const struct cred *mgr)
-LSM_HOOK(int, 0, binder_transaction, const struct cred *from,
+LSM_HOOK(int, LSM_RET_INT(0, -MAX_ERRNO, 0), binder_set_context_mgr, const struct cred *mgr)
+LSM_HOOK(int, LSM_RET_INT(0, -MAX_ERRNO, 0), binder_transaction, const struct cred *from,
const struct cred *to)
-LSM_HOOK(int, 0, binder_transfer_binder, const struct cred *from,
+LSM_HOOK(int, LSM_RET_INT(0, -MAX_ERRNO, 0), binder_transfer_binder, const struct cred *from,
const struct cred *to)
-LSM_HOOK(int, 0, binder_transfer_file, const struct cred *from,
+LSM_HOOK(int, LSM_RET_INT(0, -MAX_ERRNO, 0), binder_transfer_file, const struct cred *from,
const struct cred *to, const struct file *file)
I'm not overly excited about injecting these additional return value
range annotations into the LSM hook definitions, especially since the
vast majority of the hooks "returns 0 on success, negative values on
error". I'd rather see some effort put into looking at the
feasibility of converting some (all?) of the LSM hook return value
exceptions into the more conventional 0/-ERRNO format. Unfortunately,
I haven't had the time to look into that myself, but if you wanted to
do that I think it would be a good thing.
I agree that keeping all hooks return a consistent range of 0/-ERRNO
is more elegant than adding return value range annotations. However, there
are two issues that might need to be addressed first:
1. Compatibility
For instance, security_vm_enough_memory_mm() determines whether to
set cap_sys_admin by checking if the hook vm_enough_memory returns
a positive number. If we were to change the hook vm_enough_memory
to return 0 to indicate the need for cap_sys_admin, then for the
LSM BPF program currently returning 0, the interpretation of its
return value would be reversed after the modification.
This is not an issue. bpf lsm progs are no different from other lsm-s.
If the meaning of return value or arguments to lsm hook change
all lsm-s need to adjust as well. Regardless of whether they are
written as in-kernel lsm-s, bpf-lsm, or out-of-tree lsm-s.
Yes, the are no guarantees around compatibility in kernel/LSM
interface from one kernel release to the next. If we need to change a
LSM hook, we can change a LSM hook; the important part is that when we
change the LSM hook we must make sure to update all of the in-tree
LSMs which make use of that hook.
Great, so there are no compatibility restrictions on both LSM and BPF
sides.
2. Expressing multiple non-error states using 0/-ERRNO
IIUC, although 0/-ERRNO can be used to express different errors,
only 0 can be used for non-error state. If there are multiple
non-error states, they cannot be distinguished. For example,
security_inode_need_killpriv() returns < 0 on error, 0 if
security_inode_killpriv() doesn't need to be called, and > 0
if security_inode_killpriv() does need to be called.
This looks like a problem indeed.
Hang on. There aren't really three states here. security_inode_killpriv()
is called only on the security_inode_need_killpriv() > 0 case. I'm not
looking at the code this instant, but adjusting the return to something
like -ENOSYS (OK, maybe not a great choice, but you get the idea) instead
of 0 in the don't call case and switching the positive value to 0 should
work just fine.
We're working on getting the LSM interfaces to be more consistent. This
particular pair of hooks is an example of why we need to do that.
Yes, exactly. Aside from the issues with BPF verification, we've seen
problems in the past with LSM hooks that differ from the usual "0 on
success, negative values on failure" pattern. I'm not saying it is
possible to convert all of the hooks to fit this model, but even if we
can only adjust one or two I think that is still a win.
As far as security_inode_need_killpriv()/security_inode_killpriv() is
concerned, one possibility would be to shift the ATTR_KILL_PRIV
set/mask operation into the LSM hook, something like this:
[WARNING: completely untested, likely broken, yadda yadda]
/**
* ...
* Returns: Return 0 on success, negative values on failure. @attrs
may be updated
* on success.
*/
int security_inode_need_killpriv(*dentry, attrs)
{
int rc;
rc = call_int_hook(inode_killpriv, dentry);
if (rc < 0)
return rc;
if (rc > 0)
attrs |= ATTR_KILL_PRIV;
else if (rc == 0)
attrs &= ~ATTR_KILL_PRIV;
return 0;
}
Yes, that doesn't fix the problem for the individual LSMs, but it does
make the hook a bit more consistent from the rest of the kernel.
Alright, I'll give it a try. Perhaps in the end, there will be a few
hooks that cannot be converted. If that's the case, it seems we can
just provide exceptions for the return value explanations for these
not unconverted hooks, maybe on the BPF side only, thus avoiding the
need to annotate return values for all LSM hooks.
