NACK: Cmnt: [SRU][J][PATCH 1/2] UBUNTU: SAUCE: Revert "UBUNTU: SAUCE: overlayfs: Skip permission checking for trusted.overlayfs.* xattrs"

[Massimiliano Pellizzer]

On Fri, 3 Apr 2026 at 08:08, Masahiro Yamada
<masahiro.yamada at canonical.com> wrote:
>
>
> On 4/1/26 03:26, Massimiliano Pellizzer wrote:
> > This reverts commit 3fb38c98e060b327cb58373775dcc95ed52d1f22.
> >
> > The reverted commit bypasses vfs_setxattr() in ovl_do_setxattr() by
> > calling __vfs_setxattr_noperm() directly. After After upstream commit
>
> "After" is doubled.
>
>
> > c914c0e27eb0 ("ovl: use wrappers to all vfs_*xattr() calls")
> > was backported, this routed security.capability writes during copy-up
> > through the unchecked path, bypassing cap_convert_nscap() and enabling
> > CVE-2023-2640 and CVE-2023-32629.
> >
> > CVE-2023-2640
> > CVE-2023-32629
> > Signed-off-by: Massimiliano Pellizzer <massimiliano.pellizzer at canonical.com>
> > ---
> >   fs/overlayfs/overlayfs.h | 15 ++-------------
> >   fs/xattr.c               | 36 ++++++------------------------------
> >   include/linux/xattr.h    |  1 -
> >   3 files changed, 8 insertions(+), 44 deletions(-)
> >
> > diff --git a/fs/overlayfs/overlayfs.h b/fs/overlayfs/overlayfs.h
> > index 585797e23b547..43b211cf437cc 100644
> > --- a/fs/overlayfs/overlayfs.h
> > +++ b/fs/overlayfs/overlayfs.h
> > @@ -211,12 +211,7 @@ static inline int ovl_do_setxattr(struct ovl_fs *ofs, struct dentry *dentry,
> >                                 const char *name, const void *value,
> >                                 size_t size, int flags)
> >   {
> > -     struct inode *inode = dentry->d_inode;
> > -     int err;
> > -
> > -     inode_lock(inode);
> > -     err = __vfs_setxattr_noperm(&init_user_ns, dentry, name, value, size, flags);
> > -     inode_unlock(inode);
> > +     int err = vfs_setxattr(&init_user_ns, dentry, name, value, size, flags);
> >
> >       pr_debug("setxattr(%pd2, \"%s\", \"%*pE\", %zu, %d) = %i\n",
> >                dentry, name, min((int)size, 48), value, size, flags, err);
> > @@ -233,13 +228,7 @@ static inline int ovl_setxattr(struct ovl_fs *ofs, struct dentry *dentry,
> >   static inline int ovl_do_removexattr(struct ovl_fs *ofs, struct dentry *dentry,
> >                                    const char *name)
> >   {
> > -     struct inode *inode = dentry->d_inode;
> > -     int err;
> > -
> > -     inode_lock(inode);
> > -     err = __vfs_removexattr_noperm(&init_user_ns, dentry, name);
> > -     inode_unlock(inode);
> > -
> > +     int err = vfs_removexattr(&init_user_ns, dentry, name);
> >       pr_debug("removexattr(%pd2, \"%s\") = %i\n", dentry, name, err);
> >       return err;
> >   }
> > diff --git a/fs/xattr.c b/fs/xattr.c
> > index bad89a9144cc7..030f93f3f9d0e 100644
> > --- a/fs/xattr.c
> > +++ b/fs/xattr.c
> > @@ -239,7 +239,6 @@ int __vfs_setxattr_noperm(struct user_namespace *mnt_userns,
> >
> >       return error;
> >   }
> > -EXPORT_SYMBOL_GPL(__vfs_setxattr_noperm);
> >
> >   /**
> >    * __vfs_setxattr_locked - set an extended attribute while holding the inode
> > @@ -474,34 +473,6 @@ __vfs_removexattr(struct user_namespace *mnt_userns, struct dentry *dentry,
> >   }
> >   EXPORT_SYMBOL(__vfs_removexattr);
> >
> > -/**
> > - *  __vfs_removexattr_noperm - perform removexattr operation without
> > - *  performing permission checks.
> > - *
> > - *  @dentry - object to perform setxattr on
> > - *  @name - xattr name to set
> > - *
> > - *  returns the result of the internal setxattr or setsecurity operations.
> > - *
> > - *  This function requires the caller to lock the inode's i_mutex before it
> > - *  is executed. It also assumes that the caller will make the appropriate
> > - *  permission checks.
> > - */
> > -int
> > -__vfs_removexattr_noperm(struct user_namespace *mnt_userns,
> > -                      struct dentry *dentry, const char *name)
> > -{
> > -     int error;
> > -
> > -     error =__vfs_removexattr(mnt_userns, dentry, name);
> > -     if (!error) {
> > -             fsnotify_xattr(dentry);
> > -             evm_inode_post_removexattr(dentry, name);
> > -     }
> > -     return error;
> > -}
> > -EXPORT_SYMBOL_GPL(__vfs_removexattr_noperm);
> > -
> >   /**
> >    * __vfs_removexattr_locked - set an extended attribute while holding the inode
> >    * lock
> > @@ -532,7 +503,12 @@ __vfs_removexattr_locked(struct user_namespace *mnt_userns,
> >       if (error)
> >               goto out;
> >
> > -     error = __vfs_removexattr_noperm(mnt_userns, dentry, name);
> > +     error = __vfs_removexattr(mnt_userns, dentry, name);
> > +
> > +     if (!error) {
> > +             fsnotify_xattr(dentry);
> > +             evm_inode_post_removexattr(dentry, name);
> > +     }
> >
> >   out:
> >       return error;
> > diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> > index 077b3844f2eeb..4c379d23ec6e7 100644
> > --- a/include/linux/xattr.h
> > +++ b/include/linux/xattr.h
> > @@ -63,7 +63,6 @@ int __vfs_setxattr_locked(struct user_namespace *, struct dentry *,
> >   int vfs_setxattr(struct user_namespace *, struct dentry *, const char *,
> >                const void *, size_t, int);
> >   int __vfs_removexattr(struct user_namespace *, struct dentry *, const char *);
> > -int __vfs_removexattr_noperm(struct user_namespace *, struct dentry *, const char *);
> >   int __vfs_removexattr_locked(struct user_namespace *, struct dentry *,
> >                            const char *, struct inode **);
> >   int vfs_removexattr(struct user_namespace *, struct dentry *, const char *);

Thanks for catching the typo.
Nack.

-- 
Massimiliano Pellizzer