[PATCH] Do not require attributes for security_inode_init_security.

[Paul Moore]

On Wed, Mar 27, 2024 at 5:17 AM Dr. Greg <greg at enjellic.com> wrote:
> On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote:
> > 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?
>
> As of the 6.8 release, a security handler that previously functioned
> in a consistent manner now functions inconsistently depending on the
> LSM stacking configuration that is in effect.

In Linux v6.8[1] only Smack and SELinux provide implementations for
the security_inode_init_security() hook, and both also increment the
associated lsm_blob_sizes::lbs_xattr_count field.  While the behavior
of the hook may have changed, I see no indications of any harm with
respect to the standard upstream Linux kernel.  We obviously want to
ensure that we work to fix harmful behavior, but I simply don't see
that here; convince me there is a problem, send me a patch as we've
discussed, and I'll merge it.

If we are talking about future code, simply include the change with
the associated patchset.

If we are talking about out-of-tree code, that's something else.

[1] In Linux v6.9-rc1 this grows to include EVM, but EVM also provides
both a hook implementation and a lbs_xattr_count bump.

> Perhaps more problematically, when the handler does not function
> correctly, there is no indication of that fact passed upward to the
> LSM invoking the handler.  This would cause the LSM to conclude that a
> security relevant action was conducted when it did not actually occur.
>
> I believe we would all universally conclude that having security
> critical infrastructure function in a consistent and reliable manner
> is of benefit, so we should return the previous behavior of the
> handler, particularly since it can be done with a one line fix if that
> is your preference.

You need to demonstrate the harm caused to the upstream Linux kernel,
either a proper tagged release in Linus' tree, the current development
code in Linus tree, or a subsystem branch/repository.

> If you would be so kind, please put a 'Reported-by:' tag on whatever
> you commit upstream.

As you initially submitted a patch for this, it would be preferable if
you would send a patch ... if necessary (see above comments).  Of
course if you are unable to do so, and we all agree that a problem in
the upstream kernel exists, I can submit a patch with the appropriate
credit.

I will mention that bug fixes like this are a great way for new
contributors to gain experience working with the upstream Linux
kernel; I would encourage you to see this through.  As frustrating as
this might be, debates like this are part of the process :)

-- 
paul-moore.com