linux-kernel - Re: [PATCH] vfs: Add AT_EMPTY_PATH_NOCHECK as unchecked AT_EMPTY

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: Sun, 23 Jun 2024 20:26:18 +0800
From: Xi Ruoyao <xry111@...111.site>
To: Mateusz Guzik <mjguzik@...il.com>
Cc: Linus Torvalds <torvalds@...ux-foundation.org>, Christian Brauner
	 <brauner@...nel.org>, Alexander Viro <viro@...iv.linux.org.uk>, Jan Kara
	 <jack@...e.cz>, Steven Rostedt <rostedt@...dmis.org>, Masami Hiramatsu
	 <mhiramat@...nel.org>, Alejandro Colomar <alx@...nel.org>, Arnd Bergmann
	 <arnd@...db.de>, Huacai Chen <chenhuacai@...ngson.cn>, Xuerui Wang
	 <kernel@...0n.name>, Jiaxun Yang <jiaxun.yang@...goat.com>, Icenowy Zheng
	 <uwu@...nowy.me>, linux-fsdevel@...r.kernel.org, 
	linux-trace-kernel@...r.kernel.org, linux-arch@...r.kernel.org, 
	loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vfs: Add AT_EMPTY_PATH_NOCHECK as unchecked
 AT_EMPTY_PATH

On Sun, 2024-06-23 at 14:04 +0200, Mateusz Guzik wrote:
> On Sun, Jun 23, 2024 at 3:22 AM Xi Ruoyao <xry111@...111.site> wrote:
> > 
> > On Sun, 2024-06-23 at 03:07 +0200, Mateusz Guzik wrote:
> > > On Sun, Jun 23, 2024 at 2:59 AM Xi Ruoyao <xry111@...111.site>
> > > wrote:
> > > > 
> > > > On Sat, 2024-06-22 at 15:41 -0700, Linus Torvalds wrote:
> > > > 
> > > > > I do think that we should make AT_EMPTY_PATH with a NULL path
> > > > > "JustWork(tm)", because the stupid "look if the pathname is
> > > > > empty" is
> > > > > horrible.
> > > > > 
> > > > > But moving that check into getname() is *NOT* the right
> > > > > answer,
> > > > > because by the time you get to getname(), you have already
> > > > > lost.
> > > > 
> > > > Oops.  I'll try to get around of getname() too...
> > > > 
> > > > > So the short-cut in vfs_fstatat() to never get a pathname is
> > > > > disgusting - people should have used 'fstat()' - but it's
> > > > > _important_
> > > > > disgusting.
> > > > 
> > > > The problem is we don't have fstat() for LoongArch, and it'll be
> > > > unusable on all 32-bit arch after 2037.
> > > > 
> > > > And Arnd hates the idea adding fstat() for LoongArch because
> > > > there would
> > > > be one more 32-bit arch broken in 2037.
> > > > 
> > > > Or should we just add something like "fstat_2037()"?
> > > > 
> > > 
> > > In that case fstat is out of the question, but no problem.
> > > 
> > > It was suggested to make AT_EMPTY_PATH + NULL pathname do the
> > > right
> > > thing and have the syscalls short-circuit as needed.
> > > 
> > > for statx it would look like this (except you are going to have
> > > implement do_statx_by_fd):
> > > 
> > > diff --git a/fs/stat.c b/fs/stat.c
> > > index 16aa1f5ceec4..0afe72b320cc 100644
> > > --- a/fs/stat.c
> > > +++ b/fs/stat.c
> > > @@ -710,6 +710,9 @@ SYSCALL_DEFINE5(statx,
> > >         int ret;
> > >         struct filename *name;
> > > 
> > > +       if (flags == AT_EMPTY_PATH && filename == NULL)
> > > +               return do_statx_by_fd(...);
> > > +
> > >         name = getname_flags(filename,
> > > getname_statx_lookup_flags(flags));
> > >         ret = do_statx(dfd, name, flags, mask, buffer);
> > >         putname(name);
> > > 
> > > and so on
> > > 
> > > Personally I would prefer if fstatx was added instead, fwiw most
> > > massaging in the area will be the same regardless.
> > 
> > I do agree.  But if we do it *now* would it be "breaking the
> > userspace"
> > if some stupid program relies on fstatx() to return some error when
> > the
> > path is NULL?  The "stupid programs" may just exist in the wild...
> > 
> 
> You mean statx? fstatx would not accept a path to begin with.

Yes I mean statx.

> Worry about some code breaking is why I suggested a dedicated flag
> (AT_NO_PATH) myself in case fstatx is a no-go.

I agree a dedicated flag will be better but I only came up with nasty
names like it AT_FORCE_EMPTY_PATH or AT_EMPTY_PATH_NOCHECK.  I think
your AT_NO_PATH is a better name.

> I am not convinced messing with AT_* flags is justified to begin with.
> Any syscall which does not have a fd-only variant and is found to be
> routinely used with AT_EMPTY_PATH should get one instead.
> 
> As far as I know that's only stat(due to a perf bug in glibc, now
> fixed) and increasingly statx.

And you mean fstatat instead of stat, I guess.

> Suppose AT_EMPTY_PATH + NULL are to land and stat + statx get the
> treatment. What about all the other syscalls? Sorting all that out is
> quite a big of churn which is probably not worth it. But then there is
> a feature gap in that they EFAULT for this pair while the stat* ones
> don't and that's bound to raise confusion. Then one could add the
> check in the bowels of path lookup in similar way you do did to
> maintain the same behavior (but without per-syscall churn) and a big
> fat warning that anyone getting there often needs to get patched with
> short-circuiting the entire thing. So I think that's either a lot of
> churn or nasty additions.
> 
> Regardless, as noted above, either making fstatx a thing or
> short-circuiting mostly the same patching has to be done for
> statx-related stuff.
> 
> However, this is not my call to make.

-- 
Xi Ruoyao <xry111@...111.site>
School of Aerospace Science and Technology, Xidian University