Re: [PATCH v12 10/11] SELinux: Add selfattr hooks

From: Mickaël Salaün
Date: Tue Jul 11 2023 - 11:38:41 EST

Next message: Heiko Stuebner: "[PATCH v4 01/12] riscv: Add support for kernel mode vector"
Previous message: Mickaël Salaün: "Re: [PATCH v12 06/11] LSM: wireup Linux Security Module syscalls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 29/06/2023 21:55, Casey Schaufler wrote:

Add hooks for setselfattr and getselfattr. These hooks are not very
different from their setprocattr and getprocattr equivalents, and
much of the code is shared.

Signed-off-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
Cc: selinux@xxxxxxxxxxxxxxx
Cc: Paul Moore <paul@xxxxxxxxxxxxxx>
---
security/selinux/hooks.c | 148 ++++++++++++++++++++++++++++++++-------
1 file changed, 121 insertions(+), 27 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 49ec74bc006c..a4a536552717 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -6226,8 +6226,8 @@ static void selinux_d_instantiate(struct dentry *dentry, struct inode *inode)
inode_doinit_with_dentry(inode, dentry);
}
-static int selinux_getprocattr(struct task_struct *p,
- const char *name, char **value)
+static int selinux_lsm_getattr(unsigned int attr, struct task_struct *p,
+ char **value)
{
const struct task_security_struct *__tsec;
u32 sid;
@@ -6244,20 +6244,27 @@ static int selinux_getprocattr(struct task_struct *p,
goto bad;
}
- if (!strcmp(name, "current"))
+ switch (attr) {
+ case LSM_ATTR_CURRENT:
sid = __tsec->sid;
- else if (!strcmp(name, "prev"))
+ break;
+ case LSM_ATTR_PREV:
sid = __tsec->osid;
- else if (!strcmp(name, "exec"))
+ break;
+ case LSM_ATTR_EXEC:
sid = __tsec->exec_sid;
- else if (!strcmp(name, "fscreate"))
+ break;
+ case LSM_ATTR_FSCREATE:
sid = __tsec->create_sid;
- else if (!strcmp(name, "keycreate"))
+ break;
+ case LSM_ATTR_KEYCREATE:
sid = __tsec->keycreate_sid;
- else if (!strcmp(name, "sockcreate"))
+ break;
+ case LSM_ATTR_SOCKCREATE:
sid = __tsec->sockcreate_sid;
- else {
- error = -EINVAL;
+ break;
+ default:
+ error = -EOPNOTSUPP;
goto bad;
}
rcu_read_unlock();
@@ -6275,7 +6282,7 @@ static int selinux_getprocattr(struct task_struct *p,
return error;
}
-static int selinux_setprocattr(const char *name, void *value, size_t size)
+static int selinux_lsm_setattr(u64 attr, void *value, size_t size)
{
struct task_security_struct *tsec;
struct cred *new;
@@ -6286,23 +6293,31 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
/*
* Basic control over ability to set these attributes at all.
*/
- if (!strcmp(name, "exec"))
+ switch (attr) {
+ case LSM_ATTR_EXEC:
error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETEXEC, NULL);
- else if (!strcmp(name, "fscreate"))
+ break;
+ case LSM_ATTR_FSCREATE:
error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETFSCREATE, NULL);
- else if (!strcmp(name, "keycreate"))
+ break;
+ case LSM_ATTR_KEYCREATE:
error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETKEYCREATE, NULL);
- else if (!strcmp(name, "sockcreate"))
+ break;
+ case LSM_ATTR_SOCKCREATE:
error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETSOCKCREATE, NULL);
- else if (!strcmp(name, "current"))
+ break;
+ case LSM_ATTR_CURRENT:
error = avc_has_perm(mysid, mysid, SECCLASS_PROCESS,
PROCESS__SETCURRENT, NULL);
- else
- error = -EINVAL;
+ break;
+ default:
+ error = -EOPNOTSUPP;
+ break;
+ }
if (error)
return error;
@@ -6314,13 +6329,14 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
}
error = security_context_to_sid(value, size,
&sid, GFP_KERNEL);
- if (error == -EINVAL && !strcmp(name, "fscreate")) {
+ if (error == -EINVAL && attr == LSM_ATTR_FSCREATE) {
if (!has_cap_mac_admin(true)) {
struct audit_buffer *ab;
size_t audit_size;
- /* We strip a nul only if it is at the end, otherwise the
- * context contains a nul and we should audit that */
+ /* We strip a nul only if it is at the end,
+ * otherwise the context contains a nul and
+ * we should audit that */
if (str[size - 1] == '\0')
audit_size = size - 1;
else
@@ -6331,7 +6347,8 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
if (!ab)
return error;
audit_log_format(ab, "op=fscreate invalid_context=");
- audit_log_n_untrustedstring(ab, value, audit_size);
+ audit_log_n_untrustedstring(ab, value,
+ audit_size);
audit_log_end(ab);
return error;
@@ -6354,11 +6371,11 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
checks and may_create for the file creation checks. The
operation will then fail if the context is not permitted. */
tsec = selinux_cred(new);
- if (!strcmp(name, "exec")) {
+ if (attr == LSM_ATTR_EXEC) {
tsec->exec_sid = sid;
- } else if (!strcmp(name, "fscreate")) {
+ } else if (attr == LSM_ATTR_FSCREATE) {
tsec->create_sid = sid;
- } else if (!strcmp(name, "keycreate")) {
+ } else if (attr == LSM_ATTR_KEYCREATE) {
if (sid) {
error = avc_has_perm(mysid, sid,
SECCLASS_KEY, KEY__CREATE, NULL);
@@ -6366,9 +6383,9 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
goto abort_change;
}
tsec->keycreate_sid = sid;
- } else if (!strcmp(name, "sockcreate")) {
+ } else if (attr == LSM_ATTR_SOCKCREATE) {
tsec->sockcreate_sid = sid;
- } else if (!strcmp(name, "current")) {
+ } else if (attr == LSM_ATTR_CURRENT) {
error = -EINVAL;
if (sid == 0)
goto abort_change;
@@ -6410,6 +6427,81 @@ static int selinux_setprocattr(const char *name, void *value, size_t size)
return error;
}
+static int selinux_getselfattr(unsigned int attr, struct lsm_ctx __user *ctx,
+ size_t *size, u32 flags)
+{
+ char *value;
+ size_t total_len;
+ int len;
+ int rc;
+
+ len = selinux_lsm_getattr(attr, current, &value);
+ if (len < 0)
+ return len;
+
+ total_len = ALIGN(struct_size(ctx, ctx, len), 8);
+
+ if (total_len > *size)
+ rc = -E2BIG;
+ else if (ctx)
+ rc = lsm_fill_user_ctx(ctx, value, len, LSM_ID_SELINUX, 0);
+ else
+ rc = 1;
+
+ *size = total_len;
+ if (rc < 0)
+ return rc;
+ return 1;
+}
+
+static int selinux_setselfattr(unsigned int __user attr,
+ struct lsm_ctx __user *ctx, size_t __user size,
+ u32 __user flags)
+{
+ struct lsm_ctx *lctx;
+ int rc;
+
+ lctx = kmalloc(size, GFP_KERNEL);
+ if (lctx == NULL)
+ return -ENOMEM;
+
+ if (copy_from_user(lctx, ctx, size))
+ rc = -EFAULT;
+ else if (lctx->ctx_len > size)

Isn't this check redundant with the ones from security_setselfattr()?

This also applies to AppArmor and Smack patches. It would be simpler to move the common code/checks to security_setselfattr().

+ rc = -EINVAL;

The Smack implementation returns E2BIG instead. It would be a good idea to return the same kind of error code, and document it. Again, factoring out the common part would help.

+ else
+ rc = selinux_lsm_setattr(attr, lctx->ctx, lctx->ctx_len);
+
+ kfree(lctx);
+ if (rc > 0)
+ return 0;
+ return rc;
+}
+
+static int selinux_getprocattr(struct task_struct *p,
+ const char *name, char **value)
+{
+ unsigned int attr = lsm_name_to_attr(name);
+ int rc;
+
+ if (attr) {
+ rc = selinux_lsm_getattr(attr, p, value);
+ if (rc != -EOPNOTSUPP)
+ return rc;
+ }
+
+ return -EINVAL;
+}
+
+static int selinux_setprocattr(const char *name, void *value, size_t size)
+{
+ int attr = lsm_name_to_attr(name);
+
+ if (attr)
+ return selinux_lsm_setattr(attr, value, size);
+ return -EINVAL;
+}
+
static int selinux_ismaclabel(const char *name)
{
return (strcmp(name, XATTR_SELINUX_SUFFIX) == 0);
@@ -7041,6 +7133,8 @@ static struct security_hook_list selinux_hooks[] __ro_after_init = {
LSM_HOOK_INIT(d_instantiate, selinux_d_instantiate),
+ LSM_HOOK_INIT(getselfattr, selinux_getselfattr),
+ LSM_HOOK_INIT(setselfattr, selinux_setselfattr),
LSM_HOOK_INIT(getprocattr, selinux_getprocattr),
LSM_HOOK_INIT(setprocattr, selinux_setprocattr),

Next message: Heiko Stuebner: "[PATCH v4 01/12] riscv: Add support for kernel mode vector"
Previous message: Mickaël Salaün: "Re: [PATCH v12 06/11] LSM: wireup Linux Security Module syscalls"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]