Andy Lutomirski <luto@xxxxxxxxxx> wrote: > This is an attempt at a review. I'm replying here because I can't find the > actual relevant patch emails. This was the latest post: https://lkml.org/lkml/2017/11/9/660 and they were posted multiple times before that, plus distributions, such as Fedora, have been carrying them for a long while. > For the rest of this review, I'm going to pretend that you actually want two > features: "try-prevent-root-from-corrupting-the-kernel" and > "try-to-prevent-root-from-reading-kernel-memory". It theoretically boils down into those two, but the line is blurrier than you think. Further, some of the vectors that can be used to do one can potentially do the other also and it starts getting to be a lot of extra work to distinguish the two. > I do *not* see why the mere act of using Secure Boot should have this > effect. To be able to pass secure boot mode over kexec, you have to make sure that the kernel image doesn't get corrupted, lest someone blacklist your signing key in the bootloader. > In particular, UEFI Secure Boot should *not* enable > "try-to-prevent-root-from-reading-kernel-memory", which means that, unless > you actually implement the split, you should drop a bunch of the patches. Yes it should. If someone can read your kernel image, they can steal the crypto keys you use to encrypt your filesystem. > "Restrict /dev/{mem,kmem,port} when the kernel is locked down": this should > probably split into one restriction for read and one for write. Not so for /dev/port. Read & Write here are _not_ the same as Read & Write on, say, /dev/mem. In fact, if /dev/mem gives you access to mmio ports, then the same applies there. Btw, Fedora hasn't even provided /dev/kmem for a while. > "bpf: Restrict kernel image access functions when the kernel is locked down": > This patch just sucks in general. Yes - but that's what Alexei Starovoitov specified. bpf kind of sucks since it gives you unrestricted access to the kernel. > "debugfs: Restrict debugfs when the kernel is locked down": The logic is IMO > nutty. Why the 0444 restriction? I see no reason that reading a 0644 file > should be treated any differently from reading a 0444 file. Yes. IMO it should be locked down entirely. However, it's been abused and there are things in there that are apparently needed (ie. it's not debugging-only now); unfortunately, it *also* contains files that directly map hardware. > "efi: Lock down the kernel if booted in secure boot mode": you have a stray > change in fs/debugfs/inode.c in here. Good catch, thanks. > Also, as above, I really dislike this patch. You dislike everything, but you didn't say so any of the times these patches were posted... > "lockdown: Print current->comm in restriction messages": Shouldn't this be > folded in with whatever patch added that code in the first place? Perhaps, but at the time I added it, I didn't want to go back and change the existing patches again. If I have to do so, I'll fold it in then. David -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
