Re: [PATCH v10 2/4] security: Allow all LSMs to provide xattrs for inode_init_security hook
From: Roberto Sassu
Date: Thu Apr 06 2023 - 05:15:50 EST
On 4/5/2023 11:07 PM, Casey Schaufler wrote:
On 4/5/2023 1:49 PM, Paul Moore wrote:
On Wed, Apr 5, 2023 at 4:43 PM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
On 4/5/2023 12:59 PM, Paul Moore wrote:
On Wed, Apr 5, 2023 at 5:44 AM Roberto Sassu
<roberto.sassu@xxxxxxxxxxxxxxx> wrote:
On 4/5/2023 4:08 AM, Casey Schaufler wrote:
On 4/4/2023 11:54 AM, Paul Moore wrote:
On Fri, Mar 31, 2023 at 8:33 AM Roberto Sassu
<roberto.sassu@xxxxxxxxxxxxxxx> wrote:
..
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index cfcbb748da2..8392983334b 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -52,6 +52,15 @@
#define SMK_RECEIVING 1
#define SMK_SENDING 2
+/*
+ * Smack uses multiple xattrs.
+ * SMACK64 - for access control, SMACK64EXEC - label for the program,
I think it would be good to move SMACK64EXEC to its own line; it took
me a minute to figure out why SMACK_INODE_INIT_XATTRS was set to '4'
when I only say three comment lines ... ;)
+ * SMACK64MMAP - controls library loading,
+ * SMACK64TRANSMUTE - label initialization,
+ * Not saved on files - SMACK64IPIN and SMACK64IPOUT
+ */
+#define SMACK_INODE_INIT_XATTRS 4
If smack_inode_init_security() only ever populates a single xattr, and
that is the only current user of SMACK_INODE_INIT_XATTRS, can we make
this '1' and shrink the xattr allocation a bit?
If the parent directory is marked with SMACK64_TRANSMUTE, the access
rule allowing the access has the "t" mode, and the object being initialized
is a directory, the new inode should get the SMACK64_TRANSMUTE attribute.
The callers of security_inode_init_security() don't seem to care.
I can't say if the evm code is getting SMACK64_TRANSMUTE or, for that
matter, SMACK64_EXEC and SMACK64_MMAP, some other way. The older system
allowed for multiple Smack xattrs, but I'm not clear on exactly how.
If you like to set an additional xattr, that would be possible now.
Since we reserve multiple xattrs, we can call lsm_get_xattr_slot()
another time and set SMACK64_TRANSMUTE.
I think, if the kernel config has CONFIG_EVM_EXTRA_SMACK_XATTRS set,
EVM would protect SMACK64_TRANSMUTE too.
Ooookay, but can someone explain to me how either the current, or
patched, smack_inode_init_security() function can return multiple
xattrs via the security_inode_init_security() LSM hook?
It can't.
I didn't think so.
To be really specific, that's what we're talking about with this
patch: the number of xattrs that smack_inode_init_security() can
return to the LSM hook (and EVM, and the caller ...). If it's only
ever going to be one, I think we can adjust the
'SMACK_INODE_INIT_XATTRS' down to '1' and save ourselves some
allocation space.
Does evm have an expectation that mumble_inode_init_security() is
going to report all the relevant attributes? It has to be getting
them somehow, which leads me to wonder if we might want to extend
smack_inode_init_security() to do so. Even if we did, the maximum
value would be '2', SMACK64 and SMACK64_TRANSMUTE. Now that would
require a whole lot of work in the calling filesystems, as setting
the transmute attribute would be moving out of smack_d_instantiate()
and into the callers. Or something like that.
After changing the inode_init_security hook definition to pass the full
xattr array, this is not going to be a problem. EVM sees all xattrs that
are going to be set when an inode is created, and adds its own too.
If you have enough information to set security.SMACK_TRANSMUTE64 in
smack_inode_init_security(), this patch sets already allows to set both
xattrs at the same time. We would just need to call lsm_get_xattr_slot()
another time, assuming that we reserve two xattrs.
Roberto