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

[Mickaël Salaün]

On 22/02/2021 11:47, Mickaël Salaün wrote:
> 
> 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 sent a new patch series dedicated to the LSM issue:
https://lore.kernel.org/linux-security-module/20210222150608.808146-1-mic@xxxxxxxxxxx/

> 
>>
>>>>>
>>>>> 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
>>