Re: [GIT PULL] selinux/selinux-pr-20250323

[Thiébaud Weksteen]

On Thu, Mar 27, 2025 at 11:43 AM Linus Torvalds
<torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, 26 Mar 2025 at 16:03, Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> >
> > On Wed, Mar 26, 2025 at 5:06 PM Linus Torvalds
> > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> > > That's insane. So explain to me why you added what looks like
> > > completely insane policy hooks.
> >
> > In the security_kernel_read_file() LSM hook, the SELinux callback
> > examines if the current task has the ability to perform the designated
> > read operation on file A.  Loading a kernel module and loading its
> > associated firmware are two events inside the kernel with the current
> > task loading data from two different files, each with potentially
> > different security properties, and we want to perform an access check
> > on each file to ensure it is permitted under the security policy
> > defined by the admin.

Paul, Linus,

Thank you for discussing the reasoning behind this patch. We wanted to
share how Android will leverage the new hooks introduced here.
Hopefully, this should help move forward this discussion and show why
these changes have concrete value to projects using SELinux like
Android.

Android, for a long time, has used the module loading hooks to ensure
that kernel modules are loaded from cryptographically signed,
dm-verity protected partitions. Even if an attacker compromised a
userspace process with CAP_SYS_MODULE, they cannot leverage this to
load modules from outside of protected partitions.

The changes presented in this merge request build upon this functionality.

Taking one example from this merge request: kexec image loading.

Currently, any process which has CAP_SYS_BOOT can use kexec to replace
the existing kernel. Android has 5 processes with CAP_SYS_BOOT, only 1
of which needs kexec functionality [1]. By using these new
permissions, we will ensure that this process is able to call kexec,
while prohibiting other processes. SELinux provides us strong, kernel
enforced guarantees which can be checked at policy compile time.
Extending on this, we will use this patchset to guarantee that kernels
and ramdisks executed by kexec come from known, good sources.

The other hooks are of similar value to Android.

Thiebaud and Nick

[1] https://cs.android.com/android/platform/superproject/main/+/main:system/sepolicy/microdroid/system/private/kexec.te;l=12

>
> What this tells me is that there wasn't actually any real ask for
> this, and you're trying to make up arguments for a silly patch.
>
> First off, "loading a module" and then "the module loads the firmware"
> file are *not* two distinct things with distinct security properties.
>
> The module DOES NOT WORK without the firmware file. So the argument
> that they are independent action is complete nonsense. If you don't
> trust the firmware, then don't load the module. It's that simple.
>
> Second, your argument that there are different tasks involved is true,
> but no, that doesn't mean that there are "potentially different
> security properties".
>
> Why? Because as mentioned, loading the module very much implies having
> to be able to load the firmware for it - but yes, the firmware loading
> might actually happen _later_ and in a different and completely
> independent context.
>
> In particular, drivers can do their firmware loads at various random times.
>
> Yes, one common situation is that they do it during module load, for
> example, in which case it would be done in the same context that the
> module load itself happened.
>
> But it's *also* common that it is done asynchronously while scanning
> for devices.
>
> Or done when the device is opened.
>
> Or the firmware file is reloaded when the system resumes from suspend,
> because the device lost all its context, so now it's reloading
> something that it loaded earlier in a very *different* context, and
> that had better not start randomly failing resulting in basically
> impossible-to-debug resume problems.
>
> In other words, the context of actual firmware loading is pretty much
> random. It isn't necessarily tied to the module loading, but it *also*
> isn't necessarily tied to a particular other actor.
>
> So any argument that depends on the context of the firmware load is
> bogus a priori. Since there isn't some well-defined context the load
> happens in, any policy based on such a context is garbage.
>
> To put it bluntly, your whole argument for why separate policy makes
> sense seems completely made-up and has no actual reality behind it.
>
> It also very much sounds like this change was *not* triggered by
> anybody actually needing it, and as in fact sounds like just "let's
> pattern-match and do the same thing for these things that look similar
> but aren't".
>
> This is *EXACTLY* the kind of thing that I think the security layers
> should absolutely NOT DO.
>
> You should not add nonsensical policy hooks "just because".
>
> Security policy needs to have some thought behind it, not be some
> mindless random thing. And there should have been a documented case of
> people needing it.
>
> This is literally why I was asking for the reasoning behind that
> patch. The patch looked nonsensical. And I have not at all been
> convinced otherwise.
>
>                      Linus
>