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

["Dr. Greg" <greg@enjellic.com>]

On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote:

Good morning, I hope the week is going well.

> 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);

We ran with something similar to the above for several days of TSEMv3
testing.

For the patch that we submitted upstream, we elected to take a 'belt
and suspenders' approach that isolated the 'no attributes' execution
flow from the flow followed if extended attributes are present.

The approach used doesn't make any difference to us as long as we get
the functionality of the hook restored.

If you go with the simpler approach, it may be worthwhile to at least
simplify the handling of the call to the initxattr() function after
the evm_inode_init_security() call.

It seems simpler and with more clear intent, to use a negated
conditional check of the 'ret' value from evm_inode_init_security() to
call the initxattr() function, rather than using the return value to
jump over the call.

Once again, your choice, no preferences on our part.

> paul-moore.com

Have a good day.

As always,
Dr. Greg

The Quixote Project - Flailing at the Travails of Cybersecurity
              https://github.com/Quixote-Project