On Thu, Mar 30, 2023 at 4:42 PM Casey Schaufler [off-list ref] wrote:

On 3/29/2023 6:13 PM, Paul Moore wrote:

quoted

On Wed, Mar 15, 2023 at 6:50 PM Casey Schaufler [off-list ref] wrote:

quoted

Add lsm_name_to_attr(), which translates a text string to a
LSM_ATTR value if one is available.

Add lsm_fill_user_ctx(), which fills a struct lsm_ctx, including
the trailing attribute value.

All are used in module specific components of LSM system calls.

Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
---
 include/linux/security.h | 13 ++++++++++
 security/lsm_syscalls.c  | 51 ++++++++++++++++++++++++++++++++++++++++
 security/security.c      | 31 ++++++++++++++++++++++++
 3 files changed, 95 insertions(+)

..

quoted

diff --git a/security/lsm_syscalls.c b/security/lsm_syscalls.c
index 6efbe244d304..55d849ad5d6e 100644
--- a/security/lsm_syscalls.c
+++ b/security/lsm_syscalls.c

@@ -17,6 +17,57 @@
 #include <linux/lsm_hooks.h>
 #include <uapi/linux/lsm.h>

+struct attr_map {
+       char *name;
+       u64 attr;
+};
+
+static const struct attr_map lsm_attr_names[] = {
+       {
+               .name = "current",
+               .attr = LSM_ATTR_CURRENT,
+       },
+       {
+               .name = "exec",
+               .attr = LSM_ATTR_EXEC,
+       },
+       {
+               .name = "fscreate",
+               .attr = LSM_ATTR_FSCREATE,
+       },
+       {
+               .name = "keycreate",
+               .attr = LSM_ATTR_KEYCREATE,
+       },
+       {
+               .name = "prev",
+               .attr = LSM_ATTR_PREV,
+       },
+       {
+               .name = "sockcreate",
+               .attr = LSM_ATTR_SOCKCREATE,
+       },
+};
+
+/**
+ * lsm_name_to_attr - map an LSM attribute name to its ID
+ * @name: name of the attribute
+ *
+ * Look the given @name up in the table of know attribute names.
+ *
+ * Returns the LSM attribute value associated with @name, or 0 if
+ * there is no mapping.
+ */
+u64 lsm_name_to_attr(const char *name)
+{
+       int i;
+
+       for (i = 0; i < ARRAY_SIZE(lsm_attr_names); i++)
+               if (!strcmp(name, lsm_attr_names[i].name))
+                       return lsm_attr_names[i].attr;

I'm pretty sure this is the only place where @lsm_attr_names is used,
right?  If true, when coupled with the idea that these syscalls are
going to close the door on new LSM attributes in procfs I think we can
just put the mapping directly in this function via a series of
if-statements.

Ick. You're not wrong, but the hard coded if-statement approach goes
against all sorts of coding principles. I'll do it, but I can't say I
like it.

If it helps any, I understand and am sympathetic.  I guess I've gotten
to that point where in addition to "code elegance", I'm also very
concerned about defending against "code abuse", and something like an
nicely defined mapping array is ripe for someone to come along and use
that to justify further use of the attribute string names in some
other function/API.

If you want to stick with the array - I have no problem with that -
make it local to lsm_name_to_attr().

quoted

quoted

+/**
+ * lsm_fill_user_ctx - Fill a user space lsm_ctx structure
+ * @ctx: an LSM context to be filled
+ * @context: the new context value
+ * @context_size: the size of the new context value
+ * @id: LSM id
+ * @flags: LSM defined flags
+ *
+ * Fill all of the fields in a user space lsm_ctx structure.
+ * Caller is assumed to have verified that @ctx has enough space
+ * for @context.
+ * Returns 0 on success, -EFAULT on a copyout error.
+ */
+int lsm_fill_user_ctx(struct lsm_ctx __user *ctx, void *context,
+                     size_t context_size, u64 id, u64 flags)
+{
+       struct lsm_ctx local;
+       void __user *vc = ctx;
+
+       local.id = id;
+       local.flags = flags;
+       local.ctx_len = context_size;
+       local.len = context_size + sizeof(local);
+       vc += sizeof(local);

See my prior comments about void pointer math.

quoted

+       if (copy_to_user(ctx, &local, sizeof(local)))
+               return -EFAULT;
+       if (context_size > 0 && copy_to_user(vc, context, context_size))
+               return -EFAULT;

Should we handle the padding in this function?

This function fills in a lsm_ctx. The padding, if any, is in addition to
the lsm_ctx, not part of it.

Okay, so where is the padding managed?  I may have missed it, but I
don't recall seeing it anywhere in this patchset ...

-- 
paul-moore.com