On Jul 11, 2024 Xu Kuohai <xukuohai@xxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> ---
> 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(¤t->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
