Messages in this thread | | | Date | Tue, 5 Apr 2022 17:38:39 +0200 | From | Mickaël Salaün <> | Subject | Re: [GIT PULL] Add trusted_for(2) (was O_MAYEXEC) |
| |
On 04/04/2022 23:28, Linus Torvalds wrote: > On Mon, Apr 4, 2022 at 1:29 PM Mickaël Salaün <mic@digikod.net> wrote: >> >> This initial proposal was using a new faccessat2(2) flag: >> AT_INTERPRETED, see >> https://lore.kernel.org/all/20200908075956.1069018-2-mic@digikod.net/ >> What do you think about that? I'm happy to get back to this version if >> everyone is OK with it. > > I'm certainly happi_er_ with that, but I find that particular patch > odd for other reasons. > > In no particular order: > > - what's with the insane non-C syntax? Double parentheses have no > actual meaning in C: > > if ((flags & AT_INTERPRETED)) { > if ((mode & MAY_EXEC)) { > > so I don't understand why you'd use that strance thing.
I guess it comes from a previous version that ANDed two booleans.
> > - why is this an AT_INTERPRETED flag? I don't understand the name, I > don't understand the semantics.
I wasn't sure it was a good idea to add another mode bit, so I ended up using a flag to not break compatibility of other mode checks but extend the semantic to interpreted scripts. But I agree that a new mode makes sense.
> > Why would that flag have the same value as AT_SYMLINK_FOLLOW?
It was a bug.
> > Why isn't this just a new _mode_ bit, which is what I feel is > sensible? We only use three bits (with no bits set meaning > "existence"), so we have *tons* of bits left in that namespace, and it > would make much more sense to me to have > > #define EXECVE_OK 8 > > which is the same as the "group exec" bit, so it actually makes > some kind of sense too.
Looks fine to me. The "EXECVE_" prefix is a bit weird but it will not be defined in the kernel like X_OK and others anyway, and as you said, it matches S_IXGRP.
> > - related to that "I don't understand the semantics", the > "documentation" for that thing doesn't make sense either: > > + The > + main usage is for script > + interpreters to enforce a policy > + consistent with the kernel's one > + (through sysctl configuration or LSM > + policy). */
I'll synchronize the documentation with a next series.
> > Now, what I *think* you mean is See a following email in reply to Kees. [...]
> > And yes, we still need to talk details: > > - no backwards compatibility issues, because we've happily always > checked 'mode' for being valid, so old kernels will always return > -EINVAL. > > - POSIX namespace issues for EXECVE_OK means that the name probably > needs some thinking (maybe we just need to call it __ACCESS_OK_EXECVE > internally or something - the kernel actually doesn't even export the > existing [FRWX]_OK values, because we "know" they map tho the usual > "owner RWX" bits, with F being "no bit set")
Right, I cannot find a better name for now.
See a following email in reply to Kees. [...]
> > So to recap: I'm very much ok with some access() extension, but I > think even that very much needs clarification, and the existing patch > is just odd in many many ways.
This v8 was kind of an early version, I'll update everything. Thanks!
| |