Re: [PATCH v2 3/3] security: Add LSMs dependencies to CONFIG_LSM

[Mickaël Salaün]

On 21/02/2021 15:45, Masahiro Yamada wrote:
> On Sun, Feb 21, 2021 at 8:11 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
>>
>>
>> On 21/02/2021 09:50, Masahiro Yamada wrote:
>>> On Tue, Feb 16, 2021 at 4:03 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>>>>
>>>> On Mon, Feb 15, 2021 at 7:17 PM Mickaël Salaün <mic@xxxxxxxxxxx> wrote:
>>>>> From: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
>>>>>
>>>>> Thanks to the previous commit, this gives the opportunity to users, when
>>>>> running make oldconfig, to update the list of enabled LSMs at boot time
>>>>> if an LSM has just been enabled or disabled in the build.  Moreover,
>>>>> this list only makes sense if at least one LSM is enabled.
>>>>>
>>>>> Cc: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>>>>> Cc: James Morris <jmorris@xxxxxxxxx>
>>>>> Cc: Masahiro Yamada <masahiroy@xxxxxxxxxx>
>>>>> Cc: Serge E. Hallyn <serge@xxxxxxxxxx>
>>>>> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxxxxxxxxxx>
>>>>> Link: https://lore.kernel.org/r/20210215181511.2840674-4-mic@xxxxxxxxxxx
>>>>> ---
>>>>>
>>>>> Changes since v1:
>>>>> * Add CONFIG_SECURITY as a dependency of CONFIG_LSM.  This prevent an
>>>>>   error when building without any LSMs.
>>>>> ---
>>>>>  security/Kconfig | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/security/Kconfig b/security/Kconfig
>>>>> index 7561f6f99f1d..addcc1c04701 100644
>>>>> --- a/security/Kconfig
>>>>> +++ b/security/Kconfig
>>>>> @@ -277,6 +277,10 @@ endchoice
>>>>>
>>>>>  config LSM
>>>>>         string "Ordered list of enabled LSMs"
>>>>> +       depends on SECURITY || SECURITY_LOCKDOWN_LSM || SECURITY_YAMA || \
>>>>> +               SECURITY_LOADPIN || SECURITY_SAFESETID || INTEGRITY || \
>>>>> +               SECURITY_SELINUX || SECURITY_SMACK || SECURITY_TOMOYO || \
>>>>> +               SECURITY_APPARMOR || BPF_LSM
>>>>
>>>> This looks really awkward, since all of these already depend on
>>>> SECURITY (if not, it's a bug)... I guarantee you that after some time
>>>> someone will come, see that the weird boolean expression is equivalent
>>>> to just SECURITY, and simplify it.
>>>
>>>
>>> Currently, LSM does not depend on SECURITY.
>>> So you can always define LSM irrespective of SECURITY,
>>> which seems a bug.
>>>
>>> So, I agree with adding 'depends on SECURITY'.
>>>
>>> What he is trying to achieve in this series
>>> seems wrong, of course.
>>
>> This may be wrong in the general case, but not for CONFIG_LSM.
>>
>>>
>>>
>>>> I assume the new mechanism wouldn't work as intended if there is just
>>>> SECURITY? If not, then maybe you should rather specify this value
>>>> dependency via some new  field rather than abusing "depends on" (say,
>>>> "value depends on"?). The fact that a seemingly innocent change to the
>>>> config definition breaks your mechanism suggests that the design is
>>>> flawed.
>>
>> Masahiro, what do you think about this suggested "value depends on"?
> 
> 
> Of course, no.
> 
> 
> See the help text in init/Kconfig:
> 
>           This choice is there only for converting CONFIG_DEFAULT_SECURITY
>           in old kernel configs to CONFIG_LSM in new kernel configs. Don't
>           change this choice unless you are creating a fresh kernel config,
>           for this choice will be ignored after CONFIG_LSM has been set.
> 
> 
> When CONFIG_LSM is already set in the .config,
> this choice is just ignored.
> So, oldconfig is working as the help message says.
> 
> If you think 2623c4fbe2ad1341ff2d1e12410d0afdae2490ca
> is a pointless commit, you should ask Kees about it.

This commit was for backward compatibility to not change the configured
system behavior because of a new default configuration.
Here I want to address a forward compatibility issue: when users want to
enable an LSM, give them the opportunity to enable it at boot time too
instead of silently ignoring this new configuration at boot time.
Indeed, there is two kind of configurations: built time configuration
with Kconfig, and boot time configuration with the content of
CONFIG_LSM. However, there is no direct dependency between LSM toggles
and CONFIG_LSM once it is set.

I think a better solution would be to add a new CONFIG_LSM_AUTO boolean
to automatically generate the content of CONFIG_LSM according to the
(build/kconfig) enabled LSMs, while letting users the ability to
manually configure CONFIG_LSM otherwise. What do you think?

> 
>>>>
>>>> I do think this would be a useful feature, but IMHO shouldn't be
>>>> implemented like this.
>>>>
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
>>>>>         default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
>>>>> --
>>>>> 2.30.0
>>>>>
>>>>
>>>> --
>>>> Ondrej Mosnacek
>>>> Software Engineer, Linux Security - SELinux kernel
>>>> Red Hat, Inc.
>>>>
>>>
>>>
> --
> Best Regards
> Masahiro Yamada
>