On Tue, Feb 08, 2022 at 10:52:42AM +0100, Omar Polo wrote:
> Omar Polo <op@omarpolo.com> writes:
> 
> > Bryan Steele <brynet@gmail.com> writes:
> >
> >> On Tue, Feb 08, 2022 at 12:00:05AM -0500, Bryan Steele wrote:
> >>> On Sun, Feb 06, 2022 at 04:59:05PM +0100, Omar Polo wrote:
> >>> > Thomas Adam <thomas@xteddy.org> writes:
> >>> > 
> >>> > > On Sun, Feb 06, 2022 at 12:06:27PM +0100, Omar Polo wrote:
> >>> > >> here's a revised diff.  it's equivalent in practice to the previous one,
> >>> > >> but hopefully less scary :)
> >>> > >
> >>> > > Thanks for this.  I have no means of testing this though (the kernel version I
> >>> > > have here on Arch Linux doesn't seem to offer Landlock) but I have a few
> >>> > > comment in-line below.
> >>> > 
> >>> > unfortunately it doesn't seem to be that much available yet.  I can
> >>> > confirm that fedora has it, and I'm told nixos ship with landlock
> >>> > enabled too.
> >>> > 
> >>> > I just tried on a devuan ceres (unstable) vm and they ship
> >>> > linux/landlock.h, but the actual kernel has landlock disabled!  I've
> >>> > added a check for ENOSYS and ENOTSUP to handle this situation where
> >>> > landlock was found at compile time but it's not present at runtime.
> >>> 
> >>> I'd agrue that in the ENOTSUP case it shouldn't fail silently and
> >>> should instead return an error, that way distributions attempting
> >>> to ship packages have a chance of catching it, rather than shipping
> >>> out binaries that don't actually provide any sort of protection.
> 
> Please discard everything, I misunderstood.  I thought you were talking
> about dropping the check completely, not the ENOTSUP case alone.
> 
> I sort of agree then, leaving ENOSYS for older kernels but removing the
> ENOTSUP to make sure that distros build the package correctly.
> 
> I'll do some testing on different distribution, but I agree on the idea,
> thanks!
> 
> > in one hand, i wholeheartedly agree.  any failure here should be treated
> > as fatal error and abort everything, as you noted with the pledge
> > example.
> >
> > on the other hand, we're talking about linux here >.>
> >
> > I've got a report from a user (on a different project) that a program I
> > wrote was failing for him because of landlock.  The package was built in
> > an environment with landlock enabled but then ran on a previous linux
> > version and thus failing.  These kind of monstrosities are probably not
> > so uncommon I guess.
> >
> > I don't want got to hard-fail at everything if the user boots with a
> > previous kernel version, that's the main reason.
> >
> > Furthermore, it's not that easy to disable a kernel feature, it requires
> > root access on the machine and probably a reboot too.
> >
> > Also, see the patch for gtar from the landlock author, they don't seem
> > to be too much concerned of it failing :D
> >
> > https://lists.gnu.org/archive/html/bug-tar/2021-04/msg00002.html
> >
> > what about keeping the status quo for a while still and then enforcing
> > it properly when more people are running >=5.13 ?
> >
> >>> At least in that case the distros adding --disable-landlock will be
> >>> easier spot as well.
> >>
> >> I guess that's not actually even a flag, but I guess they might
> >> try and override cv_header_linux_landlock_h=no during builds.
> >>
> >> Still if it's always going to get compiled on Linux, there should be
> >> some kind of indication it is actually doing something and isn't
> >> just a no-op.
> >>
> >> I know this has already been committed, but just had a chance to
> >> reply.
> >
> > An indicator wouldn't be bad, it's difficult to make it not extremely
> > verbose thought
> >
> > op@devuan:~/w/got-portable$ got st -S?
> > got-read-gotconfig: landlock disabled, continuing nevertheless
> > got-read-gitconfig: landlock disabled, continuing nevertheless
> > got-read-pack: landlock disabled, continuing nevertheless
> > got-read-gotconfig: landlock disabled, continuing nevertheless
> > got-read-gitconfig: landlock disabled, continuing nevertheless
> > got-read-pack: landlock disabled, continuing nevertheless
> > M  compat/landlock.c
> > op@devuan:~/w/got-portable$ got di compat/
> > got-read-gotconfig: landlock disabled, continuing nevertheless
> > got-read-gitconfig: landlock disabled, continuing nevertheless
> > got-read-pack: landlock disabled, continuing nevertheless
> > got-read-gotconfig: landlock disabled, continuing nevertheless
> > got-read-gitconfig: landlock disabled, continuing nevertheless
> > got-read-pack: landlock disabled, continuing nevertheless
> > diff 97799ccd4b67a81f97039305d4fdd66588da9962 /home/op/w/got-portable
> > blob - 47a5209dbfe20f149a142f16faaca86ca659120c
> > file + compat/landlock.c
> > --- compat/landlock.c
> > +++ compat/landlock.c
> > @@ -86,8 +86,10 @@ landlock_no_fs(void)
> >         fd = landlock_create_ruleset(&rattr, sizeof(rattr), 0);
> >         if (fd == -1) {
> >                 /* this kernel doesn't have landlock built in */
> > -               if (errno == ENOSYS || errno == ENOTSUP)
> > +               if (errno == ENOSYS || errno == ENOTSUP) {
> > +                       warnx("landlock disabled, continuing nevertheless");
> >                         return 0;
> > +               }
> >                 return -1;
> >         }

Just as a follow-up to this, documentation appears to be hard to find
online, but it seems that there is some confusion between ENOTSUP and
EOPNOTSUPP.

https://github.com/landlock-lsm/man-pages/blob/landlock-v4/man2/landlock_restrict_self.2#L96

Anyway, I can't find whether this was changed in the upstream kernel or
not, and of course it's Linux, so man pages for syscalls are *maintained
seperately*. Sigh.

-Bryan.