On Wed, Mar 27, 2024 at 11:18:47AM -0400, Paul Moore wrote:

Good morning to everyone.

On Wed, Mar 27, 2024 at 5:17???AM Dr. Greg [off-list ref] wrote:

quoted

On Tue, Mar 26, 2024 at 03:12:37PM -0400, Paul Moore wrote:

quoted

On Tue, Mar 26, 2024 at 6:31???AM Dr. Greg [off-list ref] wrote:

quoted

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

quoted

On Sun, Mar 24, 2024 at 6:33???PM Greg Wettstein [off-list ref] wrote:

quoted

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 <redacted>
---
 security/security.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

...

quoted

quoted

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.

quoted

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.

BPF provides an implementation and would be affected.

Simply grepping for 'inode_init_security', in the security
sub-directory of the kernel source tree, will miss the dependency.

Google initialized the BPF LSM hooks by redefining the LSM_HOOK macro
before including the include/linux/lsm_hook_defs.h file.  This causes
all of the LSM event handlers to be defined as BPF LSM hooks but
doesn't explicitly disclose any of the event handlers by name.

We just checked our documentation on BPF LSM tap points for
CrowdStrike's Falcon agent, which is a couple of months old by now,
and don't see them using it, but that is only a small and dated
sample.

You probably remember, I believe you were there for an LSM update
panel, that Alexei Starovoitov commented in his keynote talk at the
European Linux Security Summit last September, that he estimated 90%
of the use of the BPF LSM is not publically disclosed.  Given that,
affecting any functionality, particularly the way this issue does,
would seem to be imprudent.

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

Given the dynamics of all this, it would seem to make sense for this
to be a straight forward fix that would get directed to stable.

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

We understand Linux kernel dynamics pretty well [1], we wouldn't
advocate for changes to mainline for such initiatives.

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

BPF initialization, as of 6.8 does not include an xattr request.

quoted

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.

BPF would be currently affected, including its derivatives, from 6.8
forward.

quoted

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.

The one-liner is a simple, straight forward and acceptable fix but it
would not be our preference for fixing it.  Given that, it would
probably be best to go in under alternate authorship.

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

[1]:

With respect to a new contributor gaining experience.  For whatever it
is worth moving forward, personally, I've been spinning patches for
the Linux kernel since December of 1991.  So I have a high degree of
familiarity with respect to the dynamics of Linux kernel development
and how they have evolved over time.

Personally I also enjoy a substantive background in forensic exchange,
so debate is not a foreign concept by any means.

Given that a one-liner restores long standing behavior and potentially
fixes what could be silent corruption of desired security
functionality, there would seem to be little basis for debate, as
entertaining as that may be... :-)

paul-moore.com

Have a good remainder of the day.

As always,
Dr. Greg

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