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

On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > 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().
There's no sense in doing both, as the purpose of defining a reiserfs
initxattrs function was to clean up this code making it more readable.

Mimi

On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar at linux.ibm.com>
wrote:
>
> On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > > 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().
FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we
can remove the IMA/EVM special cases before then :)
> > 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()).
I would much rather remove security_old_inode_init_security() then
worry about what out-of-tree modules might be using it.  Hopefully we
can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook
without too much trouble, which means all we have left is reiserfs ...
how difficult would you expect the conversion to be for reiserfs?
> > If there is still the preference, I will implement the reiserfs
> > callback and make a fix for security_old_inode_init_security().
>
> There's no sense in doing both, as the purpose of defining a reiserfs
> initxattrs function was to clean up this code making it more readable.
>
> Mimi
-- 
paul-moore.com

On Mon, 2022-11-21 at 18:55 -0500, Paul Moore wrote:
> On Mon, Nov 21, 2022 at 3:54 PM Mimi Zohar <zohar at linux.ibm.com>
wrote:
> > On Mon, 2022-11-21 at 10:45 +0100, Roberto Sassu wrote:
> > > > 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().
> 
> FWIW, reiserfs isn't going to be removed until 2025, I'm hopeful we
> can remove the IMA/EVM special cases before then :)
Well, we are not that far...
> > > 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()).
> 
> I would much rather remove security_old_inode_init_security() then
> worry about what out-of-tree modules might be using it.  Hopefully we
> can resolve the ocfs2 usage and get ocfs2 exclusively on the new hook
> without too much trouble, which means all we have left is reiserfs ...
> how difficult would you expect the conversion to be for reiserfs?
Ok for removing security_old_inode_init_security().

For reiserfs, I guess maintaining the current behavior of setting only
one xattr should be easy. For multiple xattrs, I need to understand
exactly how many blocks need to be reserved.
> > > If there is still the preference, I will implement the reiserfs
> > > callback and make a fix for security_old_inode_init_security().
> > 
> > There's no sense in doing both, as the purpose of defining a
reiserfs
> > initxattrs function was to clean up this code making it more readable.
The fix for security_old_inode_init_security(), stopping at the first
LSM returning zero, was to avoid the memory leak. Will not be needed if
the function is removed.

Roberto