[PATCH] Do not require attributes for security_inode_init_security.

[Paul Moore]

On Tue, Mar 26, 2024 at 6:31 AM Dr. Greg <greg at enjellic.com> wrote:
> On Mon, Mar 25, 2024 at 05:08:54PM -0400, Paul Moore wrote:
> > On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein <greg at 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 at enjellic.com>
> > > ---
> > >  security/security.c | 24 ++++++++++++------------
> > >  1 file changed, 12 insertions(+), 12 deletions(-)

...

> > 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.

I'd prefer the simpler approach.  I'd likely also prefer we park this
patch until it is needed upstream, or am I misunderstanding things and
upstream is currently broken without a fix like this?

> 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.

Starting with v6.9-rc1 there is no longer an explicit call to
evm_inode_init_security() as it is incorporated into the normal LSM
hook processing, e.g. `hp->hook.inode_init_security(...)`.  I'm also
not sure we need to worry about the initxattrs() call near the bottom
of security_inode_init_security() since in the no
@blob.lbs_xattr_count case the @xattr_count variable will also be zero
so the initxattrs() call will be skipped.

Or were you talking about something else?

> 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