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 <[email protected]> ----- > > Date: Fri, 31 Oct 2025 07:58:56 +0000 > From: Al Viro <[email protected]> > To: [email protected] > Cc: Linus Torvalds <[email protected]>, > [email protected], Paul Moore <[email protected]> > 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 <[email protected]> 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
