| Message ID | 20240711111908.3817636-2-xukuohai@huaweicloud.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | Add return value range check for BPF LSM | expand |
Jul 11, 2024 06:14:08 Xu Kuohai <xukuohai@huaweicloud.com>: > From: Xu Kuohai <xukuohai@huawei.com> > > To be consistent with most LSM hooks, convert the return value of > hook vm_enough_memory to 0 or a negative error code. > > Before: > - Hook vm_enough_memory returns 1 if permission is granted, 0 if not. > - LSM_RET_DEFAULT(vm_enough_memory_mm) is 1. > > After: > - Hook vm_enough_memory reutrns 0 if permission is granted, negative > error code if not. > - LSM_RET_DEFAULT(vm_enough_memory_mm) is 0. I haven't done a detailed review yet, but based on the description this is definitely a good change. Thank you. > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 2 +- > security/commoncap.c | 11 +++-------- > security/security.c | 11 +++++------ > security/selinux/hooks.c | 15 ++++----------- > 5 files changed, 14 insertions(+), 27 deletions(-) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index 44488b1ab9a9..e6e6f8473955 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -48,7 +48,7 @@ LSM_HOOK(int, 0, quota_on, struct dentry *dentry) > LSM_HOOK(int, 0, syslog, int type) > LSM_HOOK(int, 0, settime, const struct timespec64 *ts, > const struct timezone *tz) > -LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages) > +LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) > LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) > LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file) > LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) > diff --git a/include/linux/security.h b/include/linux/security.h > index de3af33e6ff5..454f96307cb9 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -634,7 +634,7 @@ static inline int security_settime64(const struct timespec64 *ts, > > static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > { > - return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages)); > + return __vm_enough_memory(mm, pages, !cap_vm_enough_memory(mm, pages)); > } > > static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) > diff --git a/security/commoncap.c b/security/commoncap.c > index 162d96b3a676..cefad323a0b1 100644 > --- a/security/commoncap.c > +++ b/security/commoncap.c > @@ -1396,17 +1396,12 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, > * Determine whether the allocation of a new virtual mapping by the current > * task is permitted. > * > - * Return: 1 if permission is granted, 0 if not. > + * Return: 0 if permission granted, negative error code if not. > */ > int cap_vm_enough_memory(struct mm_struct *mm, long pages) > { > - int cap_sys_admin = 0; > - > - if (cap_capable(current_cred(), &init_user_ns, > - CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0) > - cap_sys_admin = 1; > - > - return cap_sys_admin; > + return cap_capable(current_cred(), &init_user_ns, CAP_SYS_ADMIN, > + CAP_OPT_NOAUDIT); > } > > /** > diff --git a/security/security.c b/security/security.c > index e5ca08789f74..3475f0cab3da 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1115,15 +1115,14 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) > int rc; > > /* > - * The module will respond with a positive value if > - * it thinks the __vm_enough_memory() call should be > - * made with the cap_sys_admin set. If all of the modules > - * agree that it should be set it will. If any module > - * thinks it should not be set it won't. > + * The module will respond with 0 if it thinks the __vm_enough_memory() > + * call should be made with the cap_sys_admin set. If all of the modules > + * agree that it should be set it will. If any module thinks it should > + * not be set it won't. > */ > hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { > rc = hp->hook.vm_enough_memory(mm, pages); > - if (rc <= 0) { > + if (rc < 0) { > cap_sys_admin = 0; > break; > } > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c > index 7eed331e90f0..9cd5a8f1f6a3 100644 > --- a/security/selinux/hooks.c > +++ b/security/selinux/hooks.c > @@ -2202,23 +2202,16 @@ static int selinux_syslog(int type) > } > > /* > - * Check that a process has enough memory to allocate a new virtual > - * mapping. 0 means there is enough memory for the allocation to > - * succeed and -ENOMEM implies there is not. > + * Check permission for allocating a new virtual mapping. Returns > + * 0 if permission is granted, negative error code if not. > * > * Do not audit the selinux permission check, as this is applied to all > * processes that allocate mappings. > */ > static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) > { > - int rc, cap_sys_admin = 0; > - > - rc = cred_has_capability(current_cred(), CAP_SYS_ADMIN, > - CAP_OPT_NOAUDIT, true); > - if (rc == 0) > - cap_sys_admin = 1; > - > - return cap_sys_admin; > + return cred_has_capability(current_cred(), CAP_SYS_ADMIN, > + CAP_OPT_NOAUDIT, true); > } > > /* binprm security operations */ > -- > 2.30.2
On Jul 11, 2024 Xu Kuohai <xukuohai@huaweicloud.com> wrote: > > To be consistent with most LSM hooks, convert the return value of > hook vm_enough_memory to 0 or a negative error code. > > Before: > - Hook vm_enough_memory returns 1 if permission is granted, 0 if not. > - LSM_RET_DEFAULT(vm_enough_memory_mm) is 1. > > After: > - Hook vm_enough_memory reutrns 0 if permission is granted, negative > error code if not. > - LSM_RET_DEFAULT(vm_enough_memory_mm) is 0. > > Signed-off-by: Xu Kuohai <xukuohai@huawei.com> > --- > include/linux/lsm_hook_defs.h | 2 +- > include/linux/security.h | 2 +- > security/commoncap.c | 11 +++-------- > security/security.c | 11 +++++------ > security/selinux/hooks.c | 15 ++++----------- > 5 files changed, 14 insertions(+), 27 deletions(-) A nice improvement, thank you! -- paul-moore.com
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index 44488b1ab9a9..e6e6f8473955 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -48,7 +48,7 @@ LSM_HOOK(int, 0, quota_on, struct dentry *dentry) LSM_HOOK(int, 0, syslog, int type) LSM_HOOK(int, 0, settime, const struct timespec64 *ts, const struct timezone *tz) -LSM_HOOK(int, 1, vm_enough_memory, struct mm_struct *mm, long pages) +LSM_HOOK(int, 0, vm_enough_memory, struct mm_struct *mm, long pages) LSM_HOOK(int, 0, bprm_creds_for_exec, struct linux_binprm *bprm) LSM_HOOK(int, 0, bprm_creds_from_file, struct linux_binprm *bprm, const struct file *file) LSM_HOOK(int, 0, bprm_check_security, struct linux_binprm *bprm) diff --git a/include/linux/security.h b/include/linux/security.h index de3af33e6ff5..454f96307cb9 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -634,7 +634,7 @@ static inline int security_settime64(const struct timespec64 *ts, static inline int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) { - return __vm_enough_memory(mm, pages, cap_vm_enough_memory(mm, pages)); + return __vm_enough_memory(mm, pages, !cap_vm_enough_memory(mm, pages)); } static inline int security_bprm_creds_for_exec(struct linux_binprm *bprm) diff --git a/security/commoncap.c b/security/commoncap.c index 162d96b3a676..cefad323a0b1 100644 --- a/security/commoncap.c +++ b/security/commoncap.c @@ -1396,17 +1396,12 @@ int cap_task_prctl(int option, unsigned long arg2, unsigned long arg3, * Determine whether the allocation of a new virtual mapping by the current * task is permitted. * - * Return: 1 if permission is granted, 0 if not. + * Return: 0 if permission granted, negative error code if not. */ int cap_vm_enough_memory(struct mm_struct *mm, long pages) { - int cap_sys_admin = 0; - - if (cap_capable(current_cred(), &init_user_ns, - CAP_SYS_ADMIN, CAP_OPT_NOAUDIT) == 0) - cap_sys_admin = 1; - - return cap_sys_admin; + return cap_capable(current_cred(), &init_user_ns, CAP_SYS_ADMIN, + CAP_OPT_NOAUDIT); } /** diff --git a/security/security.c b/security/security.c index e5ca08789f74..3475f0cab3da 100644 --- a/security/security.c +++ b/security/security.c @@ -1115,15 +1115,14 @@ int security_vm_enough_memory_mm(struct mm_struct *mm, long pages) int rc; /* - * The module will respond with a positive value if - * it thinks the __vm_enough_memory() call should be - * made with the cap_sys_admin set. If all of the modules - * agree that it should be set it will. If any module - * thinks it should not be set it won't. + * The module will respond with 0 if it thinks the __vm_enough_memory() + * call should be made with the cap_sys_admin set. If all of the modules + * agree that it should be set it will. If any module thinks it should + * not be set it won't. */ hlist_for_each_entry(hp, &security_hook_heads.vm_enough_memory, list) { rc = hp->hook.vm_enough_memory(mm, pages); - if (rc <= 0) { + if (rc < 0) { cap_sys_admin = 0; break; } diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 7eed331e90f0..9cd5a8f1f6a3 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -2202,23 +2202,16 @@ static int selinux_syslog(int type) } /* - * Check that a process has enough memory to allocate a new virtual - * mapping. 0 means there is enough memory for the allocation to - * succeed and -ENOMEM implies there is not. + * Check permission for allocating a new virtual mapping. Returns + * 0 if permission is granted, negative error code if not. * * Do not audit the selinux permission check, as this is applied to all * processes that allocate mappings. */ static int selinux_vm_enough_memory(struct mm_struct *mm, long pages) { - int rc, cap_sys_admin = 0; - - rc = cred_has_capability(current_cred(), CAP_SYS_ADMIN, - CAP_OPT_NOAUDIT, true); - if (rc == 0) - cap_sys_admin = 1; - - return cap_sys_admin; + return cred_has_capability(current_cred(), CAP_SYS_ADMIN, + CAP_OPT_NOAUDIT, true); } /* binprm security operations */