Re: apparmor NULL pointer dereference on resume [efivarfs]

[Christian Brauner <brauner@kernel.org>]

On Tue, Mar 11, 2025 at 04:55:16PM +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:
> > > 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.
> 
> 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.
> 
> > 
> > > 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?
> 
> (2) Is it guaranteed that efivarfs_pm_notify() is only called when and
>     while a mount for the superblock exists?
> 
> 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.
> 
> 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?
> 
> 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.

Btw, sorry but I'm going to be able to (guarantee) to respond Wed - Fri
because I'm on kid-watch-duty.