Re: [RFC v2 19/19] ima: Setup securityfs for IMA namespace

[James Bottomley]

On Mon, 2021-12-06 at 09:03 -0500, Stefan Berger wrote:
> On 12/5/21 23:27, James Bottomley wrote:
> > On Fri, 2021-12-03 at 14:11 -0500, Stefan Berger wrote:
> > > On 12/3/21 13:50, James Bottomley wrote:
> > > > On Fri, 2021-12-03 at 13:06 -0500, Stefan Berger wrote:
> > [...]
> > > > > I suppose any late filesystem init callchain would have to be
> > > > > connected to the user_namespace somehow?
> > > >   
> > > > I don't think so; I think just moving some securityfs entries
> > > > into
> > > > the user_namespace and managing the notifier chain from within
> > > > securityfs will do for now.  [although I'd have to spec this
> > > > out in
> > > > code before I knew for sure].
> > > It doesn't have to be right in the user_namespace. The IMA
> > > namespace
> > > is  connected to the user namespace and holds the dentries now...
> > > 
> > > Please spec it out...
> > OK, this is what I have.  fill_super turned out to be a locking
> > nightmare, so I triggered it from free context instead (which
> > doesn't
> > have the once per keyed superblock property, so I added a flag in
> > the
> > user namespace).  I've got it to the point where the event is
> > triggered
> > on mount and unmount, so all the entries for the namespace are
> > added
> > when the filesystem is mounted and remove when it's
> > unmounted.  This
> > style of addition no longer needs the simple_pin_fs, because the
> > add/remove callbacks substitute (plus, if we pinned, the free_super
> > wouldn't trigger on unmount).  The default behaviour still does
> > pinning
> > and unpinning, but that can be keyed off the current
> > user_namespace.
> > 
> > This is all on top of your current series ... some of the functions
> > should probably be renamed, but I kept them to show how the code
> > was
> > migrating in this sketch.
> > 
> > James
> > 
> > ---
> > 
> >  From 59c45daa8698c66c3bcebfb194123977d548a9a6 Mon Sep 17 00:00:00
> > 2001
> > From: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx>
> > Date: Sat, 4 Dec 2021 16:38:37 +0000
> > Subject: [PATCH] rework securityfs
> > 
> > ---
> > 
> > -
> > -static void _securityfs_remove(struct dentry *dentry,
> > -			       struct vfsmount **mount, int
> > *mount_count)
> > +void securityfs_remove(struct dentry *dentry)
> >   {
> >   	struct inode *dir;
> > +	struct user_namespace *ns = current_user_ns();
> 
> I had problems with this in this place. So I had to use use
> 
> struct user_namespace *user_ns = dentry->d_sb->s_user_ns;

Yes, I think that works ... the owner in the parent namespace could
actually unmount it, so keying off the user namespace it was mounted on
is definitely the correct form.

> I'll try to split up your patch and post a v3 with then. Or is it too
> early?

It's never too early to see what the series is shaping up as.  However,
I'm still not sure I got the right trigger for the SECURITYFS_NS_ADD
notifier, so that may still have to move ... or even that there isn't
some locking subtlety I missed in triggering SECURITY_NS_REMOVE from
kill_sb.

I also suspect Christian will want a pointer to the securityfs pieces
in struct user_namespace rather than discrete elements added directly.

James