Re: [PATCH v3 2/2] vfs: avoid duplicating creds in faccessat if possible

From: Al Viro <viro@zeniv.linux.org.uk>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Christian Brauner <brauner@kernel.org>,
	serge@hallyn.com, paul@paul-moore.com,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3 2/2] vfs: avoid duplicating creds in faccessat if possible
Date: Thu, 2 Mar 2023 19:02:10 +0000	[thread overview]
Message-ID: <ZADysodnEPRqhKqc@ZenIV> (raw)
In-Reply-To: <ZADuWxU963sInrj/@ZenIV>

On Thu, Mar 02, 2023 at 06:43:39PM +0000, Al Viro wrote:
> On Thu, Mar 02, 2023 at 07:22:17PM +0100, Mateusz Guzik wrote:
> 
> > Ops, I meant "names_cache", here:
> > 	names_cachep = kmem_cache_create_usercopy("names_cache", PATH_MAX, 0,
> > 			SLAB_HWCACHE_ALIGN|SLAB_PANIC, 0, PATH_MAX, NULL);
> > 
> > it is fs/dcache.c and I brainfarted into the above.
> 
> So you mean __getname() stuff?

The thing is, getname_flags()/getname_kernel() is not the only user of that
thing; grep and you'll see (and keep in mind that cifs alloc_dentry_path()
is a __getname() wrapper, with its own callers).  We might have bugs papered
over^W^Whardened away^W^Wpapered over in some of those users.

I agree that getname_flags()/getname_kernel()/sys_getcwd() have no need of
pre-zeroing; fw_get_filesystem_firmware(), ceph_mdsc_build_path(),
[hostfs]dentry_name() and ima_d_path() seem to be safe.  So's
vboxsf_path_from_dentry() (I think).  But with this bunch I'd need
a review before I'd be willing to say "this security theatre buys us
nothing here":

fs/cifs/cifsproto.h:67: return __getname();
fs/exfat/dir.c:195:     nb->lfn = __getname();
fs/fat/dir.c:287:               *unicode = __getname();
fs/fat/namei_vfat.c:598:        uname = __getname();
fs/ntfs3/dir.c:394:     name = __getname();
fs/ntfs3/inode.c:1289:  new_de = __getname();
fs/ntfs3/inode.c:1694:  de = __getname();
fs/ntfs3/inode.c:1732:  de = __getname();
fs/ntfs3/namei.c:71:    struct cpu_str *uni = __getname();
fs/ntfs3/namei.c:286:   de = __getname();
fs/ntfs3/namei.c:355:   struct cpu_str *uni = __getname();
fs/ntfs3/namei.c:494:   uni = __getname();
fs/ntfs3/namei.c:555:   uni1 = __getname();
fs/ntfs3/xattr.c:532:   buf = __getname();

fs/cifs/cifs_dfs_ref.c:168:     page = alloc_dentry_path();
fs/cifs/cifsacl.c:1697: page = alloc_dentry_path();
fs/cifs/cifsacl.c:1760: page = alloc_dentry_path();
fs/cifs/cifsproto.h:65:static inline void *alloc_dentry_path(void)
fs/cifs/dir.c:187:      void *page = alloc_dentry_path();
fs/cifs/dir.c:604:      page = alloc_dentry_path();
fs/cifs/dir.c:664:      page = alloc_dentry_path();
fs/cifs/file.c:594:     page = alloc_dentry_path();
fs/cifs/file.c:796:     page = alloc_dentry_path();
fs/cifs/file.c:2223:    void *page = alloc_dentry_path();
fs/cifs/file.c:2255:    void *page = alloc_dentry_path();
fs/cifs/inode.c:1663:   page = alloc_dentry_path();
fs/cifs/inode.c:1938:   page = alloc_dentry_path();
fs/cifs/inode.c:2001:   void *page = alloc_dentry_path();
fs/cifs/inode.c:2170:   page1 = alloc_dentry_path();
fs/cifs/inode.c:2171:   page2 = alloc_dentry_path();
fs/cifs/inode.c:2446:   page = alloc_dentry_path();
fs/cifs/inode.c:2738:   void *page = alloc_dentry_path();
fs/cifs/inode.c:2893:   void *page = alloc_dentry_path();
fs/cifs/ioctl.c:34:     void *page = alloc_dentry_path();
fs/cifs/link.c:491:     page1 = alloc_dentry_path();
fs/cifs/link.c:492:     page2 = alloc_dentry_path();
fs/cifs/misc.c:803:     page = alloc_dentry_path();
fs/cifs/readdir.c:1071: void *page = alloc_dentry_path();
fs/cifs/smb2ops.c:2059: void *page = alloc_dentry_path();
fs/cifs/xattr.c:112:    page = alloc_dentry_path();
fs/cifs/xattr.c:277:    page = alloc_dentry_path();
fs/cifs/xattr.c:382:    page = alloc_dentry_path();