From: Omar Polo Subject: Re: [got-portable] landlock support, second try To: Bryan Steele Cc: gameoftrees@openbsd.org Date: Tue, 08 Feb 2022 10:10:13 +0100 Bryan Steele 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 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. 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; }