Re: [PATCH v4 7/20] lsm: Refactor return value of LSM hook setprocattr

On Jul 11, 2024 Xu Kuohai xukuohai@huaweicloud.com wrote:

To be consistent with most LSM hooks, convert the return value of hook setprocattr to 0 or a negative error code.

Before:

• Hook setprocattr returns the number of bytes written on success or a negative error code on failure.

After:

• Hook setprocattr returns 0 on success or a negative error code on failure. An output parameter @wbytes is introduced to hold the number of bytes written on success.

Signed-off-by: Xu Kuohai xukuohai@huawei.com

fs/proc/base.c | 5 +++-- include/linux/lsm_hook_defs.h | 3 ++- include/linux/security.h | 5 +++-- security/apparmor/lsm.c | 10 +++++++--- security/security.c | 8 +++++--- security/selinux/hooks.c | 11 ++++++++--- security/smack/smack_lsm.c | 14 ++++++++++---- 7 files changed, 38 insertions(+), 18 deletions(-)

The security_setprocattr() hook is another odd case that we probably just want to leave alone for two reasons:

1. With the move to LSM syscalls for getting/setting a task's LSM attributes we are "freezing" the procfs API and not adding any new entries to it.

2. The BPF LSM doesn't currently register any procfs entries.

I'd suggest leaving security_setprocattr() as-is and blocking it in the BPF verifier, I can't see any reason why a BPF LSM would need this hook.

diff --git a/fs/proc/base.c b/fs/proc/base.c index 72a1acd03675..9e1cf6cc674d 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -2740,6 +2740,7 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, { struct inode * inode = file_inode(file); struct task_struct *task;

• size_t wbytes; void *page; int rv;

@@ -2785,12 +2786,12 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf, rv = security_setprocattr(PROC_I(inode)->op.lsmid, file->f_path.dentry->d_name.name, page,

• 		  count);
  

• 		  count, &wbytes);
  

  mutex_unlock(&current->signal->cred_guard_mutex);

out_free: kfree(page); out:

• return rv;

• return rv < 0 ? rv : wbytes;

} static const struct file_operations proc_pid_attr_operations = { diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index dbc16f14f42f..2628514bb19c 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -287,7 +287,8 @@ LSM_HOOK(int, -EOPNOTSUPP, setselfattr, unsigned int attr, struct lsm_ctx *ctx, u32 size, u32 flags) LSM_HOOK(int, -EINVAL, getprocattr, struct task_struct *p, const char *name, char **value) -LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size) +LSM_HOOK(int, -EINVAL, setprocattr, const char *name, void *value, size_t size,

• size_t *wbytes)
  

LSM_HOOK(int, 0, ismaclabel, const char *name) LSM_HOOK(int, -EOPNOTSUPP, secid_to_secctx, u32 secid, char **secdata, u32 *seclen) diff --git a/include/linux/security.h b/include/linux/security.h index 96a63e132abf..1f1a9696e65d 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -496,7 +496,8 @@ int security_setselfattr(unsigned int attr, struct lsm_ctx __user *ctx, u32 size, u32 flags); int security_getprocattr(struct task_struct *p, int lsmid, const char *name, char **value); -int security_setprocattr(int lsmid, const char *name, void *value, size_t size); +int security_setprocattr(int lsmid, const char *name, void *value, size_t size,

• 	 size_t *wbytes);
  

int security_netlink_send(struct sock *sk, struct sk_buff *skb); int security_ismaclabel(const char *name); int security_secid_to_secctx(u32 secid, char **secdata, u32 *seclen); @@ -1440,7 +1441,7 @@ static inline int security_getprocattr(struct task_struct *p, int lsmid, } static inline int security_setprocattr(int lsmid, char *name, void *value,

• 		       size_t size)
  

• 		       size_t size, size_t *wbytes)
  

{ return -EINVAL; } diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c index 72dd09993f28..6c8b1f8c5781 100644 --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -939,13 +939,17 @@ static int apparmor_setselfattr(unsigned int attr, struct lsm_ctx *ctx, } static int apparmor_setprocattr(const char *name, void *value,

• 		size_t size)
  

• 		size_t size, size_t *wbytes)
  

{

• int rc = -EINVAL; int attr = lsm_name_to_attr(name);

if (attr)

• return do_setattr(attr, value, size);
  

• return -EINVAL;

• rc = do_setattr(attr, value, size);
  

• if (rc < 0)

• return rc;
  

• *wbytes = rc;
• return 0;

} /** diff --git a/security/security.c b/security/security.c index 095e78efcb32..9685096dbf16 100644 --- a/security/security.c +++ b/security/security.c @@ -4141,20 +4141,22 @@ int security_getprocattr(struct task_struct *p, int lsmid, const char *name,

• @name: attribute name
• @value: attribute value
• @size: attribute value size

• • @wbytes: bytes written on success
  • Write (set) the current task's attribute @name to @value, size @size if
  • allowed.

• • Return: Returns bytes written on success, a negative value otherwise.

• • Return: Returns 0 on success, a negative error code otherwise.
  */

-int security_setprocattr(int lsmid, const char *name, void *value, size_t size) +int security_setprocattr(int lsmid, const char *name, void *value, size_t size,

• 	 size_t *wbytes)
  

{ struct security_hook_list *hp; hlist_for_each_entry(hp, &security_hook_heads.setprocattr, list) { if (lsmid != 0 && lsmid != hp->lsmid->id) continue;

• return hp->hook.setprocattr(name, value, size);
  

• return hp->hook.setprocattr(name, value, size, wbytes);
  

  } return LSM_RET_DEFAULT(setprocattr);

} diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c index 0d35bb93baca..7a73f3710025 100644 --- a/security/selinux/hooks.c +++ b/security/selinux/hooks.c @@ -6589,13 +6589,18 @@ static int selinux_getprocattr(struct task_struct *p, return -EINVAL; } -static int selinux_setprocattr(const char *name, void *value, size_t size) +static int selinux_setprocattr(const char *name, void *value, size_t size,

• 	       size_t *wbytes)
  

{

• int rc = -EINVAL; int attr = lsm_name_to_attr(name);

if (attr)

• return selinux_lsm_setattr(attr, value, size);
  

• return -EINVAL;

• rc = selinux_lsm_setattr(attr, value, size);
  

• if (rc < 0)

• return rc;
  

• *wbytes = rc;
• return 0;

} static int selinux_ismaclabel(const char *name) diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c index 63d9c5f456c1..4265f2639106 100644 --- a/security/smack/smack_lsm.c +++ b/security/smack/smack_lsm.c @@ -3797,19 +3797,25 @@ static int smack_setselfattr(unsigned int attr, struct lsm_ctx *ctx,

• @name: the name of the attribute in /proc/.../attr
• @value: the value to set
• @size: the size of the value

• • @wbytes: the length of the smack label written
  • Sets the Smack value of the task. Only setting self
  • is permitted and only with privilege

• • Returns the length of the smack label or an error code

• • Returns 0 on success or a negative error code
  */

-static int smack_setprocattr(const char *name, void *value, size_t size) +static int smack_setprocattr(const char *name, void *value, size_t size,

• 	     size_t *wbytes)
  

{

• int rc = -EINVAL; int attr = lsm_name_to_attr(name);

if (attr != LSM_ATTR_UNDEF)

• return do_setattr(attr, value, size);
  

• return -EINVAL;

• rc = do_setattr(attr, value, size);
  

• if (rc < 0)

• return rc;
  

• *wbytes = rc;
• return 0;

} /** -- 2.30.2

-- paul-moore.com