On Fri, 2023-03-24 at 17:19 -0400, Paul Moore wrote:

On Fri, Mar 24, 2023 at 6:18 AM Roberto Sassu
[off-list ref] wrote:

quoted

On Thu, 2023-03-23 at 20:09 -0400, Paul Moore wrote:

quoted

On Tue, Mar 14, 2023 at 4:19 AM Roberto Sassu
[off-list ref] wrote:

quoted

From: Roberto Sassu <roberto.sassu@huawei.com>

Currently, security_inode_init_security() supports only one LSM providing
an xattr and EVM calculating the HMAC on that xattr, plus other inode
metadata.

Allow all LSMs to provide one or multiple xattrs, by extending the security
blob reservation mechanism. Introduce the new lbs_xattr field of the
lsm_blob_sizes structure, so that each LSM can specify how many xattrs it
needs, and the LSM infrastructure knows how many xattr slots it should
allocate.

Dynamically allocate the xattrs array to be populated by LSMs with the
inode_init_security hook, and pass it to the latter instead of the
name/value/len triple. Update the documentation accordingly, and fix the
description of the xattr name, as it is not allocated anymore.

Since the LSM infrastructure, at initialization time, updates the number of
the requested xattrs provided by each LSM with a corresponding offset in
the security blob (in this case the xattr array), it makes straightforward
for an LSM to access the right position in the xattr array.

There is still the issue that an LSM might not fill the xattr, even if it
requests it (legitimate case, for example it might have been loaded but not
initialized with a policy). Since users of the xattr array (e.g. the
initxattrs() callbacks) detect the end of the xattr array by checking if
the xattr name is NULL, not filling an xattr would cause those users to
stop scanning xattrs prematurely.

Solve that issue by introducing security_check_compact_filled_xattrs(),
which does a basic check of the xattr array (if the xattr name is filled,
the xattr value should be too, and viceversa), and compacts the xattr array
by removing the holes.

An alternative solution would be to let users of the xattr array know the
number of elements of that array, so that they don't have to check the
termination. However, this seems more invasive, compared to a simple move
of few array elements.

security_check_compact_filled_xattrs() also determines how many xattrs in
the xattr array have been filled. If there is none, skip
evm_inode_init_security() and initxattrs(). Skipping the former also avoids
EVM to crash the kernel, as it is expecting a filled xattr.

Finally, adapt both SELinux and Smack to use the new definition of the
inode_init_security hook, and to correctly fill the designated slots in the
xattr array. For Smack, reserve space for the other defined xattrs although
they are not set yet in smack_inode_init_security().

Reported-by: Nicolas Bouchinet <redacted> (EVM crash)
Link: https://lore.kernel.org/linux-integrity/Y1FTSIo+1x+4X0LS@archlinux/ (local)
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Reviewed-by: Casey Schaufler <casey@schaufler-ca.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/linux/lsm_hook_defs.h |   3 +-
 include/linux/lsm_hooks.h     |   1 +
 security/security.c           | 119 +++++++++++++++++++++++++++++-----
 security/selinux/hooks.c      |  19 ++++--
 security/smack/smack_lsm.c    |  33 ++++++----
 5 files changed, 137 insertions(+), 38 deletions(-)

...

quoted

quoted

quoted

@@ -1604,33 +1654,66 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
                                 const struct qstr *qstr,
                                 const initxattrs initxattrs, void *fs_data)
 {
-       struct xattr new_xattrs[MAX_LSM_EVM_XATTR + 1];
-       struct xattr *lsm_xattr, *evm_xattr, *xattr;
-       int ret;
+       struct security_hook_list *P;
+       struct xattr *new_xattrs;
+       struct xattr *xattr;
+       int ret = -EOPNOTSUPP, num_filled_xattrs = 0;

        if (unlikely(IS_PRIVATE(inode)))
                return 0;

+       if (!blob_sizes.lbs_xattr)
+               return 0;
+
        if (!initxattrs)
                return call_int_hook(inode_init_security, -EOPNOTSUPP, inode,
-                                    dir, qstr, NULL, NULL, NULL);
-       memset(new_xattrs, 0, sizeof(new_xattrs));
-       lsm_xattr = new_xattrs;
-       ret = call_int_hook(inode_init_security, -EOPNOTSUPP, inode, dir, qstr,
-                           &lsm_xattr->name,
-                           &lsm_xattr->value,
-                           &lsm_xattr->value_len);
-       if (ret)
+                                   dir, qstr, NULL);
+       /* Allocate +1 for EVM and +1 as terminator. */
+       new_xattrs = kcalloc(blob_sizes.lbs_xattr + 2, sizeof(*new_xattrs),
+                            GFP_NOFS);
+       if (!new_xattrs)
+               return -ENOMEM;
+
+       hlist_for_each_entry(P, &security_hook_heads.inode_init_security,
+                            list) {
+               ret = P->hook.inode_init_security(inode, dir, qstr, new_xattrs);
+               if (ret && ret != -EOPNOTSUPP)
+                       goto out;
+               /*
+                * As documented in lsm_hooks.h, -EOPNOTSUPP in this context
+                * means that the LSM is not willing to provide an xattr, not
+                * that it wants to signal an error. Thus, continue to invoke
+                * the remaining LSMs.
+                */
+               if (ret == -EOPNOTSUPP)
+                       continue;
+               /*
+                * As the number of xattrs reserved by LSMs is not directly
+                * available, directly use the total number blob_sizes.lbs_xattr
+                * to keep the code simple, while being not the most efficient
+                * way.
+                */

Is there a good reason why the LSM can't return the number of xattrs
it is adding to the xattr array?  It seems like it should be fairly
trivial for the individual LSMs to determine and it could save a lot
of work.  However, given we're at v8 on this patchset I'm sure I'm
missing something obvious, can you help me understand why the idea
above is crazy stupid? ;)

Ok, I looked back at what I did for v3.

Moving from v3 to v4, I decided to put less burden on LSMs, and to make
all the processing from the LSM infrastructure side.

As a general rule I think it's a good goal to keep the LSM layer as
small as possible; I believe it allows us to be more flexible with the
LSMs and it keeps the LSM as simple as possible.  I mean less code,
less bugs, amirite? ... ;)

quoted

v3 had some safeguards to prevent some programming mistakes by LSMs,
which maybe made the code less understandable.

However, if we say we keep things as simple as possible and assume that
LSMs implement this correctly, we can just pass num_filled_xattrs to
them and they simply increment it.

The EVM bug should not arise (accessing xattr->name = NULL), even if
BPF LSM alone returns zero, due to the check of num_filled_xattrs
before calling evm_inode_init_security().

Patch 6 (at the end) will prevent the bug from arising when EVM is
moved to the LSM infrastructure (no num_filled_xattrs check anymore).
There is a loop that stops if xattr->name is NULL, so
evm_protected_xattr() will not be called.

Or, like you suggested, we just return a positive value from LSMs and
we keep num_filled_xattrs in security_inode_init_security().

I like the idea of individual LSMs simply reporting the number of
xattrs they've generated instead of incrementing the num_filled_xattrs
variable.

It seems like returning the xattr count as a positive return value
should work just fine, leaving negative values for errors, but if you
run into problems you can always pass the value back in a new
parameter pointer if needed.

quoted

quoted

quoted

@@ -2868,11 +2870,11 @@ static int selinux_dentry_create_files_as(struct dentry *dentry, int mode,

 static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
                                       const struct qstr *qstr,
-                                      const char **name,
-                                      void **value, size_t *len)
+                                      struct xattr *xattrs)
 {
        const struct task_security_struct *tsec = selinux_cred(current_cred());
        struct superblock_security_struct *sbsec;
+       struct xattr *xattr = NULL;
        u32 newsid, clen;
        int rc;
        char *context;

@@ -2899,16 +2901,18 @@ static int selinux_inode_init_security(struct inode *inode, struct inode *dir,
            !(sbsec->flags & SBLABEL_MNT))
                return -EOPNOTSUPP;

-       if (name)
-               *name = XATTR_SELINUX_SUFFIX;
+       if (xattrs)
+               xattr = xattrs + selinux_blob_sizes.lbs_xattr;

Please abstract that away to an inline function similar to
selinux_cred(), selinux_file(), selinux_inode(), etc.

Ok.

quoted

quoted

+       if (xattr) {
+               xattr->name = XATTR_SELINUX_SUFFIX;

I'm guessing the xattr->name assignment is always done, regardless of
if security_sid_to_context_force() is successful, due to the -EINVAL
check in security_check_compact_filled_xattrs()?  If yes, it would be
good to make note of that here in the code.  If not, it would be nice
to move this down the function to go with the other xattr->XXX
assignments, unless there is another reason for its placement that I'm
missing.

Uhm, if an LSM returns an error, security_inode_init_security() stops
and does the cleanup. It should not matter if xattr->name was set.

Okay, I thought I might be missing something during the review.  Since
there is no special reason for putting the xattr->name assignment up
there, please move it down below with the other xattr->XXX
assignments.

Ok, will do.

Thanks

Roberto