Re: apparmor NULL pointer dereference on resume [efivarfs]

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
> On Tue, Mar 11, 2025 at 08:16:34AM +0100, Ard Biesheuvel wrote:
> > (cc Al Viro)
> > 
> > On Mon, 10 Mar 2025 at 22:49, James Bottomley
> > <James.Bottomley@hansenpartnership.com> wrote:
[...]
> > > The problem comes down to the superblock functions not being able
> > > to get the struct vfsmount for the superblock (because it isn't
> > > even allocated until after they've all been called).  The
> > > assumption I was operating under was that provided I added
> > > O_NOATIME to prevent the parent directory being updated, passing
> > > in a NULL mnt for the purposes of iterating the directory dentry
> > > was safe.  What apparmour is trying to do is look up the idmap
> > > for the mount point to do one of its checks.
> > > 
> > > There are two ways of fixing this that I can think of.  One would
> > > be exporting a function that lets me dig the vfsmount out of
> > > s_mounts and use that (it's well hidden in the internals of
> > > fs/mount.h, so I suspect this might not be very acceptable) or to
> > > get mnt_idmap to return
> 
> Nope, please don't.
> 
> > > &nop_mnt_idmap if the passed in mnt is NULL.  I'd lean towards
> > > the latter, but I'm cc'ing fsdevel to see what others think.
> 
> A struct path with mount NULL and dentry != NULL is guaranteed to bit
> us in the ass in other places. That's the bug.
> 
> > > 
> > 
> > 
> > Al spotted the same issue based on a syzbot report [0]
> > 
> > [0] https://lore.kernel.org/all/20250310235831.GL2023217@ZenIV/
> 
> efivars as written only has a single global superblock and it doesn't
> support idmapped mounts and I don't see why it ever would.

So that's not quite true: efivarfs currently supports uid and gid
mapping as mount options, which certainly looks like they were designed
to allow a second mount in a user directory.  I've no idea what the
actual use case for this is, but if I go for a single superblock, any
reconfigure with new uid/gid would become globally effective (change
every current mount) because they're stored in the superblock
information.

So what is the use case for this uid/gid parameter?  If no-one can
remember and no-one actually uses it, perhaps the whole config path can
be simplified by getting rid of the options?  Even if there is a use
case, if it's single mount only then we can still go with a global
superblock.

> But since efivars does only ever have a single global superblock, one
> possibility is to an internal superblock that always exits and is
> resurfaced whenever userspace mounts efivarfs. That's essentially the
> devtmpfs model.
> 
> Then you can stash:
> 
> static struct vfsmount *efivarfs_mnt;
> 
> globally and use that in efivarfs_pm_notify() to fill in struct path.

I didn't see devtmpfs when looking for examples, since it's hiding
outside of the fs/ directory.  However, it does seem to be a bit legacy
nasty as an example to copy.  However, I get the basics: we'd
instantiate the mnt and superblock on init (stashing mnt in the sfi so
the notifier gets it).  Then we can do the variable population on
reconfigure, just in case an EFI system doesn't want to mount efivarfs
to save memory.

I can code that up if I can get an answer to the uid/gid parameter
question above.

Regards,

James