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

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

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