Re: [viro@zeniv.linux.org.uk: [RFC] audit reporting (or not reporting) pathnames on early failures in syscalls]

[Richard Guy Briggs <rgb@redhat.com>]

On 2025-10-31 08:06, Al Viro wrote:
> OK, that's two misspellings of the list name already;-/

Adding the audit userspace list to get Steve Grubb's certification take on this.

> Al, deeply embarrassed and crawling to get some sleep...
> 
> ----- Forwarded message from Al Viro <viro@zeniv.linux.org.uk> -----
> 
> Date: Fri, 31 Oct 2025 07:58:56 +0000
> From: Al Viro <viro@zeniv.linux.org.uk>
> To: linux-audit@vger.kernel.org
> Cc: Linus Torvalds <torvalds@linux-foundation.org>, linux-fsdevel@vger.kernel.org, Paul Moore <paul@paul-moore.com>
> Subject: [RFC] audit reporting (or not reporting) pathnames on early failures in syscalls
> 
> 	FWIW, I've just noticed that a patch in the series I'd been
> reordering had the following chunk:
> @@ -1421,20 +1421,16 @@ static int do_sys_openat2(int dfd, const char __user *filename,
>                           struct open_how *how)
>  {
>         struct open_flags op;
> -       struct filename *tmp;
>         int err, fd;
>  
>         err = build_open_flags(how, &op);
>         if (unlikely(err))
>                 return err;
>  
> -       tmp = getname(filename);
> -       if (IS_ERR(tmp))
> -               return PTR_ERR(tmp);
> -
>         fd = get_unused_fd_flags(how->flags);
>         if (likely(fd >= 0)) {
> -               struct file *f = do_filp_open(dfd, tmp, &op);
> +               struct filename *name __free(putname) = getname(filename);
> +               struct file *f = do_filp_open(dfd, name, &op);
>                 if (IS_ERR(f)) {
>                         put_unused_fd(fd);
>                         fd = PTR_ERR(f);
> 
> 	From the VFS or userland POV there's no problem - we would get a
> different error reported e.g. in case when *both* EMFILE and ENAMETOOLONG
> would be applicable, but that's perfectly fine.  However, from the audit
> POV it changes behaviour.
> 
> 	Consider behaviour of openat2(2).
> 1.  we do sanity checks on the last ('usize') argument.  If they
> fail, we are done.
> 2.  we copy struct open_how from userland ('how' argument).
> If copyin fails, we are done.
> 3.  we do sanity checks on how->flags, how->resolve and how->mode.
> If they fail, we are done.
> 4.  we copy the pathname to be opened from userland ('filename' argument).
> If that fails, or if the pathname is either empty or too long, we are done.
> 5.  we reserve an unused file descriptor.  If that fails, we are done.
> 6.  we allocate an empty struct file.  If that fails, we are done.
> 7.  we finally get around to the business - finding and opening the damn thing.
> Which also can fail, of course.
> 
> 	We are expected to be able to produce a record of failing
> syscall.  If we fail on step 4, well, the lack of pathname to come with
> the record is to be expected - we have failed to get it, after all.
> The same goes for failures on steps 1..3 - we hadn't gotten around to
> looking at the pathname yet, so there's no pathname to report.	What (if
> anything) makes "insane how->flags" different from "we have too many
> descriptors opened already"?  The contents of the pathname is equally
> irrelevant in both cases.  Yet in the latter case (failure at step 5)
> the pathname would get reported.  Do we need to preserve that behaviour?
> 
> 	Because the patch quoted above would change it.  It puts the failure
> to allocate a descriptor into the same situation as failures on steps 1..3.
> 
> 	As far as I can see, there are three possible approaches:
> 
> 1) if the current kernel imports the pathname before some check, that shall
> always remain that way, no matter what.  Audit might be happy, but nobody
> else would - we'll need to document that constraint and watch out for such
> regressions.  And I'm pretty sure that over the years there had been
> other such changes that went into mainline unnoticed.
> 
> 2) reordering is acceptable.  Of course, the pathname import must happen
> before we start using it, but that's the only real constraint.  That would
> mean the least headache for everyone other than audit folks.
> 
> 3) import the pathnames as early as possible.  It would mean a non-trivial
> amount of churn, but it's at least a definite policy - validity of change
> depends only on the resulting code, not the comparison with the earlier
> state, as it would in case (1).  From QoI POV it's as nice as audit folks
> could possibly ask, but it would cause quite a bit of churn to get there.
> Not impossible to do, but I would rather not go there without a need.
> Said that, struct filename handling is mostly a decent match to CLASS()
> machinery, and all required churn wouldn't be hard to fold into conversion
> to that.
> 
> 	My preference would be (2), obviously.	However, it really depends
> upon the kind of requirements audit users have.  Note that currently the
> position of pathname import in the sequence is not documented anywhere,
> so there's not much audit users can rely upon other than "the current
> behaviour is such-and-such, let's hope it doesn't change"... ;-/
> 
> 	Comments?
> 
> 
> ----- End forwarded message -----
> 

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
Upstream IRC: SunRaycer
Voice: +1.613.860 2354 SMS: +1.613.518.6570