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

[Paul Moore <paul@paul-moore.com>]

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