Re: apparmor NULL pointer dereference on resume [efivarfs]

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

On Tue, 2025-03-11 at 16:55 +0100, Christian Brauner wrote:
> On Tue, Mar 11, 2025 at 09:01:36AM -0400, James Bottomley wrote:
> > On Tue, 2025-03-11 at 09:45 +0100, Christian Brauner wrote:
[...]
> > > 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.
> 
> So efivarfs uses get_tree_single(). That means that only a single
> superblock of the filesystem type efivarfs can ever exist on the
> system.
> 
> If efivars is mounted multiple times it will be the exact same
> superblock that's used. IOW, mounting efivars multiple times is akin
> to a bind-mount. It would be a bit ugly but it could be done by
> making sure that any uid/gid changes are reflected. But see below.

OK, so it's a fair bet that either the uid/gid option is never used or
only used once globally.

> > > 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.
> 
> I have some questions. efivarfs registers efivarfs_pm_notify even
> before a superblock exists in efivarfs_init_fs_context(). That's
> called during
> fd_context = fsopen("efivarfs") before a superblock even exists:
> 
> (1) Is it guaranteed that efivarfs_pm_notify() is only called once a
>     superblock exists?

Yes (there was a fix that ensured it)

> 
> (2) Is it guaranteed that efivarfs_pm_notify() is only called when  
> and while a mount for the superblock exists?

That's the intention, but I thought the last unmount would trigger the
superblock kill.  However, I was thinking I'd need a new mechanism
based on reconfiguration to do the dentries only if the filesystem got
mounted, so I think I can cope with that.

> If the question to either one of those is "no" then the global
> vfsmount hack will not help.
> 
> From reading efivarfs_pm_notify() it looks like the answer to (1) is
> "yes" because you're dereferencing sfi->sb->s_root in
> efivarfs_pm_notify().
> 
> But I'm not at all certain that (2) isn't a "no" and that
> efivarfs_pm_notify() can be called before a mount exists. IOW, once
> fsconfig(FSCONFIG_CMD_CREATE) is called the notifier seems ready and
> registered but userspace isn't forced to call fsmount(fd_fs) at all.

I'd like to have efivarfs so that if no-one's actually mounted anything
then the reflection of the variables isn't taking up any space, so I
was planning a new mechanism for that anyway.

> They could just not to do it for whatever reason but the notifier
> should already be able to run.
> 
> Another question is whether the superblock can be freed while
> efivarfs_pm_notify() is running? I think that can't happen because
> blocking_notifier_chain_unregister(&efivar_ops_nh, &sfi->nb) will
> block in efivarfs_kill_sb() until all outstanding calls to
> efivarfs_pm_notify() are finished?

That's the way it's supposed to work, yes.  However, if we move to an
always persistent superblock and mnt, I was thinking there'd have to be
an indicator in the sfi about whether the variables were reflected or
not.

Regards,

James

> If (2) isn't guranteed then efivarfs_pm_notify() needs to be
> rewritten without relying on files because there's no guarantee that
> a mount exists at all.
>