linux-kernel - Re: [PATCH v2 20/25] ovl: add fscaps handlers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 27 Feb 2024 08:57:51 -0600
From: "Seth Forshee (DigitalOcean)" <sforshee@...nel.org>
To: Amir Goldstein <amir73il@...il.com>
Cc: Christian Brauner <brauner@...nel.org>, Serge Hallyn <serge@...lyn.com>,
	Paul Moore <paul@...l-moore.com>, Eric Paris <eparis@...hat.com>,
	James Morris <jmorris@...ei.org>,
	Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara <jack@...e.cz>,
	Stephen Smalley <stephen.smalley.work@...il.com>,
	Ondrej Mosnacek <omosnace@...hat.com>,
	Casey Schaufler <casey@...aufler-ca.com>,
	Mimi Zohar <zohar@...ux.ibm.com>,
	Roberto Sassu <roberto.sassu@...wei.com>,
	Dmitry Kasatkin <dmitry.kasatkin@...il.com>,
	Eric Snowberg <eric.snowberg@...cle.com>,
	"Matthew Wilcox (Oracle)" <willy@...radead.org>,
	Jonathan Corbet <corbet@....net>,
	Miklos Szeredi <miklos@...redi.hu>, linux-kernel@...r.kernel.org,
	linux-fsdevel@...r.kernel.org,
	linux-security-module@...r.kernel.org, audit@...r.kernel.org,
	selinux@...r.kernel.org, linux-integrity@...r.kernel.org,
	linux-doc@...r.kernel.org, linux-unionfs@...r.kernel.org
Subject: Re: [PATCH v2 20/25] ovl: add fscaps handlers

On Tue, Feb 27, 2024 at 03:28:18PM +0200, Amir Goldstein wrote:
> > +int ovl_set_fscaps(struct mnt_idmap *idmap, struct dentry *dentry,
> > +                  const struct vfs_caps *caps, int setxattr_flags)
> > +{
> > +       int err;
> > +       struct ovl_fs *ofs = OVL_FS(dentry->d_sb);
> > +       struct dentry *upperdentry = ovl_dentry_upper(dentry);
> > +       struct dentry *realdentry = upperdentry ?: ovl_dentry_lower(dentry);
> > +       const struct cred *old_cred;
> > +
> > +       /*
> > +        * If the fscaps are to be remove from a lower file, check that they
> > +        * exist before copying up.
> > +        */
> 
> Don't you need to convert -ENODATA to 0 return value in this case?

Do you mean that trying to remove an xattr that does not exist should
return 0? Standard behavior is to return -ENODATA in this situation.

> 
> > +       if (!caps && !upperdentry) {
> > +               struct path realpath;
> > +               struct vfs_caps lower_caps;
> > +
> > +               ovl_path_lower(dentry, &realpath);
> > +               old_cred = ovl_override_creds(dentry->d_sb);
> > +               err = vfs_get_fscaps(mnt_idmap(realpath.mnt), realdentry,
> > +                                    &lower_caps);
> > +               revert_creds(old_cred);
> > +               if (err)
> > +                       goto out;
> > +       }
> > +
> > +       err = ovl_want_write(dentry);
> > +       if (err)
> > +               goto out;
> > +
> 
> ovl_want_write() should after ovl_copy_up(), see:
> 162d06444070 ("ovl: reorder ovl_want_write() after ovl_inode_lock()")

Fixed.

> > @@ -758,6 +826,8 @@ static const struct inode_operations ovl_symlink_inode_operations = {
> >         .get_link       = ovl_get_link,
> >         .getattr        = ovl_getattr,
> >         .listxattr      = ovl_listxattr,
> > +       .get_fscaps     = ovl_get_fscaps,
> > +       .set_fscaps     = ovl_set_fscaps,
> >         .update_time    = ovl_update_time,
> >  };
> >
> > @@ -769,6 +839,8 @@ static const struct inode_operations ovl_special_inode_operations = {
> >         .get_inode_acl  = ovl_get_inode_acl,
> >         .get_acl        = ovl_get_acl,
> >         .set_acl        = ovl_set_acl,
> > +       .get_fscaps     = ovl_get_fscaps,
> > +       .set_fscaps     = ovl_set_fscaps,
> >         .update_time    = ovl_update_time,
> >  };
> >
> 
> 
> Sorry, I did not understand the explanation why fscaps ops are needed
> for non regular files. It does not look right to me.

The kernel does not forbid XATTR_NAME_CAPS for non-regular files and
will internally even try to read them from non-regular files during
killpriv checks. If we do not add handlers then we will end up using the
normal ovl xattr handlers, which call vfs_*xattr(). These will return an
error for fscaps xattrs after this series, which would be a change in
behavior for overlayfs and make it behave differently from other
filesystems.