Re: [PATCH] Do not require attributes for security_inode_init_security.

[Paul Moore <paul@paul-moore.com>]

On Sun, Mar 24, 2024 at 6:33 PM Greg Wettstein <greg@enjellic.com> wrote:
>
> The integration of the Integrity Measurement Architecture (IMA)
> into the LSM infrastructure introduced a conditional check that
> denies access to the security_inode_init_security() event handler
> if the LSM extended attribute 'blob' size is 0.
>
> This changes the previous behavior of this event handler and
> results in variable behavior of LSM's depending on the LSM boot
> configuration.
>
> Modify the function so that it removes the need for a non-zero
> extended attribute blob size and bypasses the memory allocation
> and freeing that is not needed if the LSM infrastructure is not
> using extended attributes.
>
> Use a break statement to exit the loop that is iterating over the
> defined handlers for this event if a halting error condition is
> generated by one of the invoked LSM handlers.  The checks for how
> to handle cleanup are executed at the end of the loop regardless
> of how the loop terminates.
>
> A two exit label strategy is implemented.  One of the exit
> labels is a target for the no attribute case while the second is
> the target for the case where memory allocated for processing of
> extended attributes needs to be freed.
>
> Signed-off-by: Greg Wettstein <greg@enjellic.com>
> ---
>  security/security.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/security/security.c b/security/security.c
> index 7035ee35a393..a0b52b964688 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1717,10 +1717,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>         if (unlikely(IS_PRIVATE(inode)))
>                 return 0;
>
> -       if (!blob_sizes.lbs_xattr_count)
> -               return 0;
> -
> -       if (initxattrs) {
> +       if (blob_sizes.lbs_xattr_count && initxattrs) {
>                 /* Allocate +1 for EVM and +1 as terminator. */
>                 new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 2,
>                                      sizeof(*new_xattrs), GFP_NOFS);
> @@ -1733,7 +1730,7 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>                 ret = hp->hook.inode_init_security(inode, dir, qstr, new_xattrs,
>                                                   &xattr_count);
>                 if (ret && ret != -EOPNOTSUPP)
> -                       goto out;
> +                       break;
>                 /*
>                  * As documented in lsm_hooks.h, -EOPNOTSUPP in this context
>                  * means that the LSM is not willing to provide an xattr, not
> @@ -1742,19 +1739,22 @@ int security_inode_init_security(struct inode *inode, struct inode *dir,
>                  */
>         }
>
> -       /* If initxattrs() is NULL, xattr_count is zero, skip the call. */
> -       if (!xattr_count)
> -               goto out;
> +       /* Skip xattr processing if no attributes are in use. */
> +       if (!blob_sizes.lbs_xattr_count)
> +               goto out2;
> +       /* No attrs or an LSM returned an actionable error code. */
> +       if (!xattr_count || (ret && ret != -EOPNOTSUPP))
> +               goto out1;
>
>         ret = evm_inode_init_security(inode, dir, qstr, new_xattrs,
>                                       &xattr_count);
> -       if (ret)
> -               goto out;
> -       ret = initxattrs(inode, new_xattrs, fs_data);
> -out:
> +       if (!ret)
> +               ret = initxattrs(inode, new_xattrs, fs_data);
> + out1:
>         for (; xattr_count > 0; xattr_count--)
>                 kfree(new_xattrs[xattr_count - 1].value);
>         kfree(new_xattrs);
> + out2:
>         return (ret == -EOPNOTSUPP) ? 0 : ret;
>  }
>  EXPORT_SYMBOL(security_inode_init_security);
> --
> 2.39.1

Looking at this quickly, why does something like the following not work?

[WARNING: copy-n-paste patch, likely whitespace damaged]

diff --git a/security/security.c b/security/security.c
index 7e118858b545..007ce438e636 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1712,10 +1712,7 @@ int security_inode_init_security(struct inode *inode, str
uct inode *dir,
       if (unlikely(IS_PRIVATE(inode)))
               return 0;

-       if (!blob_sizes.lbs_xattr_count)
-               return 0;
-
-       if (initxattrs) {
+       if (initxattrs && blob_sizes.lbs_xattr_count) {
               /* Allocate +1 as terminator. */
               new_xattrs = kcalloc(blob_sizes.lbs_xattr_count + 1,
                                    sizeof(*new_xattrs), GFP_NOFS);

-- 
paul-moore.com