Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <5f7ee090-1708-c5ac-d194-1cc5c69487a6@schaufler-ca.com>
Date:   Tue, 18 Apr 2023 15:34:07 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v8 04/11] LSM: syscalls for current process attributes
Content-Language: en-US
To:     Paul Moore <paul@paul-moore.com>
Cc:     linux-security-module@vger.kernel.org, jmorris@namei.org,
        keescook@chromium.org, john.johansen@canonical.com,
        penguin-kernel@i-love.sakura.ne.jp, stephen.smalley.work@gmail.com,
        linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
        mic@digikod.net, Casey Schaufler <casey@schaufler-ca.com>
References: <20230411155921.14716-1-casey@schaufler-ca.com>
 <20230411155921.14716-5-casey@schaufler-ca.com>
 <CAHC9VhQHxEzv9bQJQzTiSQZvGiMO86+HizRtaR=2LW=KwsDvfg@mail.gmail.com>
From:   Casey Schaufler <casey@schaufler-ca.com>
In-Reply-To: <CAHC9VhQHxEzv9bQJQzTiSQZvGiMO86+HizRtaR=2LW=KwsDvfg@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 4/18/2023 2:49 PM, Paul Moore wrote:

> On Tue, Apr 11, 2023 at 12:01 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
>> Create a system call lsm_get_self_attr() to provide the security
>> module maintained attributes of the current process.
>> Create a system call lsm_set_self_attr() to set a security
>> module maintained attribute of the current process.
>> Historically these attributes have been exposed to user space via
>> entries in procfs under /proc/self/attr.
>>
>> The attribute value is provided in a lsm_ctx structure. The structure
>> identifies the size of the attribute, and the attribute value. The format
>> of the attribute value is defined by the security module. A flags field
>> is included for LSM specific information. It is currently unused and must
>> be 0. The total size of the data, including the lsm_ctx structure and any
>> padding, is maintained as well.
>>
>> struct lsm_ctx {
>>         __u64   id;
>>         __u64   flags;
>>         __u64   len;
>>         __u64   ctx_len;
>>         __u8    ctx[];
>> };
>>
>> Two new LSM hooks are used to interface with the LSMs.
>> security_getselfattr() collects the lsm_ctx values from the
>> LSMs that support the hook, accounting for space requirements.
>> security_setselfattr() identifies which LSM the attribute is
>> intended for and passes it along.
>>
>> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  Documentation/userspace-api/lsm.rst | 15 +++++
>>  include/linux/lsm_hook_defs.h       |  4 ++
>>  include/linux/lsm_hooks.h           |  9 +++
>>  include/linux/security.h            | 19 ++++++
>>  include/linux/syscalls.h            |  5 ++
>>  include/uapi/linux/lsm.h            | 30 +++++++++
>>  kernel/sys_ni.c                     |  4 ++
>>  security/Makefile                   |  1 +
>>  security/lsm_syscalls.c             | 55 ++++++++++++++++
>>  security/security.c                 | 98 +++++++++++++++++++++++++++++
>>  10 files changed, 240 insertions(+)
>>  create mode 100644 security/lsm_syscalls.c
> ..
>
>> diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
>> index 33a0ee3bcb2e..97487d66dca9 100644
>> --- a/include/linux/syscalls.h
>> +++ b/include/linux/syscalls.h
>> @@ -71,6 +71,7 @@ struct clone_args;
>>  struct open_how;
>>  struct mount_attr;
>>  struct landlock_ruleset_attr;
>> +struct lsm_ctx;
>>  enum landlock_rule_type;
>>
>>  #include <linux/types.h>
>> @@ -1058,6 +1059,10 @@ asmlinkage long sys_memfd_secret(unsigned int flags);
>>  asmlinkage long sys_set_mempolicy_home_node(unsigned long start, unsigned long len,
>>                                             unsigned long home_node,
>>                                             unsigned long flags);
>> +asmlinkage long sys_lsm_get_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> +                                     size_t *size, __u32 flags);
>> +asmlinkage long sys_lsm_set_self_attr(unsigned int attr, struct lsm_ctx *ctx,
>> +                                     __u32 flags);
> As pointed out by the kernel test robot, the above declaration is
> missing the @size parameter.

Yup.

>> diff --git a/include/uapi/linux/lsm.h b/include/uapi/linux/lsm.h
>> index f27c9a9cc376..b10dfab8a4d9 100644
>> --- a/include/uapi/linux/lsm.h
>> +++ b/include/uapi/linux/lsm.h
>> @@ -9,6 +9,36 @@
>> #ifndef _UAPI_LINUX_LSM_H
>> #define _UAPI_LINUX_LSM_H
>>
>> +#include <linux/types.h>
>> +#include <linux/unistd.h>
>> +
>> +/**
>> + * struct lsm_ctx - LSM context information
>> + * @id: the LSM id number, see LSM_ID_XXX
>> + * @flags: LSM specific flags
>> + * @len: length of the lsm_ctx struct, @ctx and any other data or padding
>> + * @ctx_len: the size of @ctx
>> + * @ctx: the LSM context value
>> + *
>> + * The @len field MUST be equal to the size of the lsm_ctx struct
>> + * plus any additional padding and/or data placed after @ctx.
>> + *
>> + * In all cases @ctx_len MUST be equal to the length of @ctx.
>> + * If @ctx is a string value it should be nul terminated with
>> + * @ctx_len equal to `strlen(@ctx) + 1`.  Binary values are
>> + * supported.
>> + *
>> + * The @flags and @ctx fields SHOULD only be interpreted by the
>> + * LSM specified by @id; they MUST be set to zero/0 when not used.
>> + */
>> +struct lsm_ctx {
>> +       __u64   id;
>> +       __u64   flags;
>> +       __u64   len;
>> +       __u64   ctx_len;
>> +       __u8    ctx[];
>> +};
> Sorry, style nitpick since this needs to be respun anyway for the
> syscalls.h fix at the very least ... I *really* dislike when variable
> declarations, and field declarations in the case composite variables,
> are aligned with the vertically neighboring declarations; just use a
> single space please:

I'll do it, but the tab after type has been accepted for forever.
As an aside, I pulled out my 1978 K&R to prove my point and discovered
that it isn't consistent regarding this style.

>   struct lsm_ctx {
>   <tab>__u64 id;
>   <tab>__u64 flags;
>   <tab>__u64 len;
>   <tab>__u64 ctx_len;
>   <tab>__u8 ctx[];
>   }
>
>> diff --git a/security/security.c b/security/security.c
>> index 38ca0e646cac..bfe9a1a426b2 100644
>> --- a/security/security.c
>> +++ b/security/security.c
>> @@ -2167,6 +2167,104 @@ void security_d_instantiate(struct dentry *dentry, struct inode *inode)
>>  }
>>  EXPORT_SYMBOL(security_d_instantiate);
>>
>> +/**
>> + * security_getselfattr - Read an LSM attribute of the current process.
>> + * @attr: which attribute to return
>> + * @ctx: the user-space destination for the information, or NULL
>> + * @size: the size of space available to receive the data
>> + * @flags: reserved for future use, must be 0
>> + *
>> + * Returns the number of attributes found on success, negative value
>> + * on error. @size is reset to the total size of the data.
>> + * If @size is insufficient to contain the data -E2BIG is returned.
>> + */
>> +int security_getselfattr(unsigned int __user attr, struct lsm_ctx __user *ctx,
>> +                        size_t __user *size, u32 __user flags)
>> +{
>> +       struct security_hook_list *hp;
>> +       u8 __user *base = (u8 __user *)ctx;
>> +       size_t total = 0;
>> +       size_t entrysize;
>> +       size_t left;
>> +       bool toobig = false;
>> +       int count = 0;
>> +       int rc;
>> +
>> +       if (attr == 0)
>> +               return -EINVAL;
>> +       if (flags)
>> +               return -EINVAL;
> I like Mickaël's idea of supporting a flag (LSM_FLG_SINGLE?) which
> allows one to request a single LSM's attribute.

I don't, but I'll incorporate it.

>   I don't think that
> support has to be part of this initial patchset, but I do think it
> would be good to have it in the same PR that goes up to Linus during
> the merge window.

As this patch set is intended to be what goes to Linus (isn't it?)
I'll put it in.

>   If that's not something you want to do, let me know
> and I'll write up a quick patch on top of this patchset.
>
>> +       if (size == NULL)
>> +               return -EINVAL;
>> +       if (get_user(left, size))
>> +               return -EFAULT;
>> +
>> +       hlist_for_each_entry(hp, &security_hook_heads.getselfattr, list) {
>> +               entrysize = left;
>> +               if (base)
>> +                       ctx = (struct lsm_ctx __user *)(base + total);
>> +               rc = hp->hook.getselfattr(attr, ctx, &entrysize, flags);
>> +               if (rc == -EOPNOTSUPP) {
>> +                       rc = 0;
>> +                       continue;
>> +               }
>> +               if (rc == -E2BIG) {
>> +                       toobig = true;
>> +                       left = 0;
>> +                       break;
> It just occurred to me while reading this that we stop calculating the
> potential lsm_ctx size after we hit the first LSM where we go beyond
> the size given, `rc == -E2BIG`.  I realize that the required size may
> change between calls to lsm_get_self_attr(2), but it seems like we
> should at least run through all the LSMs and total up the required
> buffer size, no?

The "break" should be a "continue". Artifact of an earlier version
that had a switch statement. Fix forthcoming.

>
> I may have missed something in the snippet below, but I think the code
> change should be pretty minor:
>
>   if (rc == -EOPNOTSUPP) {
>     rc = 0;
>     continue;
>   } else if (rc == -E2BIG) {
>     toobig = true;
>     base = NULL;
>   } else if (rc < 0) {
>     return rc;
>   }
>   left = (toobig ? 0 : left - entrysize);
>   total += entrysize;
>
>   count += rc;
>
>> +               }
>> +               if (rc < 0)
>> +                       return rc;
>> +
>> +               left -= entrysize;
>> +               total += entrysize;
>> +               count += rc;
>> +       }
>> +       if (count == 0)
>> +               return LSM_RET_DEFAULT(getselfattr);
>> +       if (put_user(total, size))
>> +               return -EFAULT;
>> +       if (toobig)
>> +               return -E2BIG;
>> +       return count;
>> +}
> --
> paul-moore.com