| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9f26368cc7aeccba460c9bce0a13f301@paul-moore.com>
Date: Thu, 18 Jul 2024 22:08:05 -0400
From: Paul Moore <paul@...l-moore.com>
To: Xu Kuohai <xukuohai@...weicloud.com>, bpf@...r.kernel.org, netdev@...r.kernel.org, linux-security-module@...r.kernel.org, linux-kselftest@...r.kernel.org, linux-integrity@...r.kernel.org, apparmor@...ts.ubuntu.com, selinux@...r.kernel.org
Cc: Alexei Starovoitov <ast@...nel.org>, Andrii Nakryiko <andrii@...nel.org>,
Daniel Borkmann <daniel@...earbox.net>, Martin KaFai Lau <martin.lau@...ux.dev>,
Eduard Zingerman <eddyz87@...il.com>, Song Liu <song@...nel.org>,
Yonghong Song <yonghong.song@...ux.dev>, John Fastabend <john.fastabend@...il.com>,
KP Singh <kpsingh@...nel.org>, Stanislav Fomichev <sdf@...gle.com>, Hao Luo <haoluo@...gle.com>,
Jiri Olsa <jolsa@...nel.org>, Matt Bobrowski <mattbobrowski@...gle.com>,
Brendan Jackman <jackmanb@...omium.org>, James Morris <jmorris@...ei.org>,
"Serge E . Hallyn" <serge@...lyn.com>, Khadija Kamran <kamrankhadijadj@...il.com>,
Casey Schaufler <casey@...aufler-ca.com>, Ondrej Mosnacek <omosnace@...hat.com>,
Kees Cook <keescook@...omium.org>, John Johansen <john.johansen@...onical.com>,
Lukas Bulwahn <lukas.bulwahn@...il.com>, Roberto Sassu <roberto.sassu@...wei.com>,
Shung-Hsi Yu <shung-hsi.yu@...e.com>, Edward Cree <ecree.xilinx@...il.com>,
Alexander Viro <viro@...iv.linux.org.uk>, Christian Brauner <brauner@...nel.org>,
Trond Myklebust <trond.myklebust@...merspace.com>, Anna Schumaker <anna@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Stephen Smalley <stephen.smalley.work@...il.com>
Subject: Re: [PATCH v4 7/20] lsm: Refactor return value of LSM hook setprocattr
On Jul 11, 2024 Xu Kuohai <xukuohai@...weicloud.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@...wei.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(¤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
Powered by blists - more mailing lists