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

OK, that's two misspellings of the list name already;-/

Al, deeply embarrassed and crawling to get some sleep... ----- Forwarded message from Al Viro <viro(a)zeniv.linux.org.uk&gt; ----- Date: Fri, 31 Oct 2025 07:58:56 +0000 From: Al Viro <viro(a)zeniv.linux.org.uk&gt; To: linux-audit(a)vger.kernel.org Cc: Linus Torvalds <torvalds(a)linux-foundation.org&gt;, linux-fsdevel(a)vger.kernel.org, Paul Moore <paul(a)paul-moore.com&gt; 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 -----