Ocfs2 devel - Nov 2022 - [PATCH v4 2/5] security: Rewrite security_old_inode_init_security()

Hi Roberto,

On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> From: Roberto Sassu <roberto.sassu at huawei.com>
> 
> Rewrite security_old_inode_init_security() to call
> security_inode_init_security() before making changes to support multiple
> LSMs providing xattrs. Do it so that the required changes are done only in
> one place.
Only security_inode_init_security() has support for EVM.   Making
security_old_inode_init_security() a wrapper for
security_inode_init_security() could result in security.evm extended
attributes being created that previously weren't created.

In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
the old and new inode_init_security calls based on the caller's
preference.   Only mknod and symlink seem to use the old function. 
Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
mailing list as they're affected by this change.)

"[PATCH v4 1/5] reiserfs: Add missing calls to
reiserfs_security_free()"  fixed a memory leak.  I couldn't tell if
there was a similar memory leak in ocfs2, the only other user of
security_old_inode_init_security().

As ocfs2 already defines initxattrs, that leaves only reiserfs missing
initxattrs().  A better, cleaner solution would be to define one.

thanks,

Mimi
> 
> Define the security_initxattrs() callback and pass it to
> security_inode_init_security() as argument, to obtain the first xattr
> provided by LSMs.
> 
> This behavior is a bit different from the current one. Before this patch
> calling call_int_hook() could cause multiple LSMs to provide an xattr,
> since call_int_hook() does not stop when an LSM returns zero. The caller of
> security_old_inode_init_security() receives the last xattr set. The pointer
> of the xattr value of previous LSMs is lost, causing memory leaks.
> 
> However, in practice, this scenario does not happen as the only in-tree
> LSMs providing an xattr at inode creation time are SELinux and Smack, which
> are mutually exclusive.
> 
> Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>b

On 11/17/2022 2:03 PM, Mimi Zohar wrote:
> Hi Roberto,
> 
> On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
>> From: Roberto Sassu <roberto.sassu at huawei.com>
>>
>> Rewrite security_old_inode_init_security() to call
>> security_inode_init_security() before making changes to support
multiple
>> LSMs providing xattrs. Do it so that the required changes are done only
in
>> one place.
> 
> Only security_inode_init_security() has support for EVM.   Making
> security_old_inode_init_security() a wrapper for
> security_inode_init_security() could result in security.evm extended
> attributes being created that previously weren't created.
Hi Mimi

yes, I thought about this problem. In fact, it should not matter too 
much. Since security_old_inode_init_security() supports setting only one 
xattr: if there is an LSM xattr, that one will be set, and the EVM one 
will be discarded; if there is no LSM xattr, EVM would not add one.
> In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
> the old and new inode_init_security calls based on the caller's
> preference.   Only mknod and symlink seem to use the old function.
> Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
> mailing list as they're affected by this change.)
> 
> "[PATCH v4 1/5] reiserfs: Add missing calls to
> reiserfs_security_free()"  fixed a memory leak.  I couldn't tell
if
> there was a similar memory leak in ocfs2, the only other user of
> security_old_inode_init_security().
Will look into it.
> As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> initxattrs().  A better, cleaner solution would be to define one.
Yes, great idea!

Thanks

Roberto
> thanks,
> 
> Mimi
> 
>>
>> Define the security_initxattrs() callback and pass it to
>> security_inode_init_security() as argument, to obtain the first xattr
>> provided by LSMs.
>>
>> This behavior is a bit different from the current one. Before this
patch
>> calling call_int_hook() could cause multiple LSMs to provide an xattr,
>> since call_int_hook() does not stop when an LSM returns zero. The
caller of
>> security_old_inode_init_security() receives the last xattr set. The
pointer
>> of the xattr value of previous LSMs is lost, causing memory leaks.
>>
>> However, in practice, this scenario does not happen as the only in-tree
>> LSMs providing an xattr at inode creation time are SELinux and Smack,
which
>> are mutually exclusive.
>>
>> Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>b

On Thu, 2022-11-17 at 08:03 -0500, Mimi Zohar wrote:
> Hi Roberto,
> 
> On Thu, 2022-11-10 at 10:46 +0100, Roberto Sassu wrote:
> > From: Roberto Sassu <roberto.sassu at huawei.com>
> > 
> > Rewrite security_old_inode_init_security() to call
> > security_inode_init_security() before making changes to support
multiple
> > LSMs providing xattrs. Do it so that the required changes are done
only in
> > one place.
> 
> Only security_inode_init_security() has support for EVM.   Making
> security_old_inode_init_security() a wrapper for
> security_inode_init_security() could result in security.evm extended
> attributes being created that previously weren't created.
> 
> In fact ocfs2 defines ocfs2_init_security_get() as a wrapper for both
> the old and new inode_init_security calls based on the caller's
> preference.   Only mknod and symlink seem to use the old function. 
> Wondering why do they differentiate between callers?  (Cc'ing the ocfs2
> mailing list as they're affected by this change.)
> 
> "[PATCH v4 1/5] reiserfs: Add missing calls to
> reiserfs_security_free()"  fixed a memory leak.  I couldn't tell
if
> there was a similar memory leak in ocfs2, the only other user of
> security_old_inode_init_security().
>From what I see, there is no memory leak there.
> As ocfs2 already defines initxattrs, that leaves only reiserfs missing
> initxattrs().  A better, cleaner solution would be to define one.
If I understood why security_old_inode_init_security() is called
instead of security_inode_init_security(), the reason seems that the
filesystem code uses the length of the obtained xattr to make some
calculations (e.g. reserve space). The xattr is written at a later
time.

Since for reiserfs there is a plan to deprecate it, it probably
wouldn't be worth to support the creation of multiple xattrs. I would
define a callback to take the first xattr and make a copy, so that
calling security_inode_init_security() + reiserfs_initxattrs() is
equivalent to calling security_old_inode_init_security().

But then, this is what anyway I was doing with the
security_initxattrs() callback, for all callers of security_old_inode_i
nit_security().

Also, security_old_inode_init_security() is exported to kernel modules.
Maybe, it is used somewhere. So, unless we plan to remove it
completely, it should be probably be fixed to avoid multiple LSMs
successfully setting an xattr, and losing the memory of all except the
last (which this patch fixes by calling security_inode_init_security())
.

If there is still the preference, I will implement the reiserfs
callback and make a fix for security_old_inode_init_security().

Thanks

Roberto
> thanks,
> 
> Mimi
> 
> > Define the security_initxattrs() callback and pass it to
> > security_inode_init_security() as argument, to obtain the first xattr
> > provided by LSMs.
> > 
> > This behavior is a bit different from the current one. Before this
patch
> > calling call_int_hook() could cause multiple LSMs to provide an xattr,
> > since call_int_hook() does not stop when an LSM returns zero. The
caller of
> > security_old_inode_init_security() receives the last xattr set. The
pointer
> > of the xattr value of previous LSMs is lost, causing memory leaks.
> > 
> > However, in practice, this scenario does not happen as the only
in-tree
> > LSMs providing an xattr at inode creation time are SELinux and Smack,
which
> > are mutually exclusive.
> > 
> > Signed-off-by: Roberto Sassu <roberto.sassu at huawei.com>b