linux-kernel - Re: [PATCH 2/2] vfs: Make sure {statx,fstatat}(..., AT_EMPTY_PATH | ..., NULL, ...) behave as (..., AT_EMPTY_PATH

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20241008045203.GX4017910@ZenIV>
Date: Tue, 8 Oct 2024 05:52:03 +0100
From: Al Viro <viro@...iv.linux.org.uk>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: Xi Ruoyao <xry111@...111.site>, Christian Brauner <brauner@...nel.org>,
	Miao Wang <shankerwangmiao@...il.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	stable@...r.kernel.org
Subject: Re: [PATCH 2/2] vfs: Make sure {statx,fstatat}(..., AT_EMPTY_PATH |
 ..., NULL, ...) behave as (..., AT_EMPTY_PATH | ..., "", ...)

On Tue, Oct 08, 2024 at 05:27:51AM +0100, Al Viro wrote:
> On Tue, Oct 08, 2024 at 05:16:21AM +0100, Al Viro wrote:
> 
> > Folks, please don't go there.  Really.  IMO vfs_empty_path() is a wrong API
> > in the first place.  Too low-level and racy as well.
> > 
> > 	See the approach in #work.xattr; I'm going to lift that into fs/namei.c
> > (well, the slow path - everything after "if path is NULL, we are done") and
> > yes, fs/stat.c users get handled better that way.
> 
> FWIW, the intermediate (just after that commit) state of those functions is
> 
> int vfs_fstatat(int dfd, const char __user *filename,
>                               struct kstat *stat, int flags)
> {
>         int ret;
>         int statx_flags = flags | AT_NO_AUTOMOUNT;
>         struct filename *name = getname_maybe_null(filename, flags);
> 
>         if (!name)
>                 return vfs_fstat(dfd, stat);
> 
>         ret = vfs_statx(dfd, name, statx_flags, stat, STATX_BASIC_STATS);
>         putname(name); 
> 
>         return ret;  
> }
> 
> and
> 
> SYSCALL_DEFINE5(statx,
>                 int, dfd, const char __user *, filename, unsigned, flags,
>                 unsigned int, mask,
>                 struct statx __user *, buffer)
> {
>         int ret;
>         unsigned lflags;
>         struct filename *name = getname_maybe_null(filename, flags);
> 
>         /*
>          * Short-circuit handling of NULL and "" paths.
>          *
>          * For a NULL path we require and accept only the AT_EMPTY_PATH flag
>          * (possibly |'d with AT_STATX flags).
>          *
>          * However, glibc on 32-bit architectures implements fstatat as statx
>          * with the "" pathname and AT_NO_AUTOMOUNT | AT_EMPTY_PATH flags.
>          * Supporting this results in the uglification below.
>          */
>         lflags = flags & ~(AT_NO_AUTOMOUNT | AT_STATX_SYNC_TYPE);
>         if (!name)
>                 return do_statx_fd(dfd, flags & ~AT_NO_AUTOMOUNT, mask, buffer);
> 
>         ret = do_statx(dfd, name, flags, mask, buffer);
>         putname(name);
> 
>         return ret;
> }
> 
> static inline struct filename *getname_maybe_null(const char __user *name, int flags)
> {
>         if (!(flags & AT_EMPTY_PATH))
>                 return getname(name);
> 
>         if (!name)
>                 return NULL;
>         return __getname_maybe_null(name);
> }
> 
> struct filename *__getname_maybe_null(const char __user *pathname)
> {
>         struct filename *name;
>         char c;
> 
>         /* try to save on allocations; loss on um, though */
>         if (get_user(c, pathname))
>                 return ERR_PTR(-EFAULT);
>         if (!c)
>                 return NULL;
> 
>         name = getname_flags(pathname, LOOKUP_EMPTY);
>         if (!IS_ERR(name) && !(name->name[0])) {
>                 putname(name);
>                 name = NULL;
>         }
>         return name;   
> }

Incidentally, the name 'getname_statx_lookup_flags' is an atrocity:
	* getname and its variants do not give a fuck for the state of
any flags besides AT_EMPTY_PATH
	* lookups, OTOH, ignore LOOKUP_EMPTY (which shouldn't have been
in the LOOKUP_... namespace to start with).

Another fun question: why do we play with setting ->mnt_id, etc. in
vfs_statx_path() if vfs_getattr() returns non-zero?  Or when we hit
it via vfs_statx() from vfs_fstatat()...