Re: [PATCH net-next v6 04/11] landlock: Add LSM hooks related to filesystem

[MickaÃl SalaÃn]

On 19/04/2017 01:40, Kees Cook wrote:
> On Tue, Apr 18, 2017 at 4:16 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
>> On 4/18/2017 3:44 PM, MickaÃl SalaÃn wrote:
>>> On 19/04/2017 00:17, Kees Cook wrote:
>>>> On Tue, Mar 28, 2017 at 4:46 PM, MickaÃl SalaÃn <mic@xxxxxxxxxxx> wrote:
>>>>> +void __init landlock_add_hooks(void)
>>>>> +{
>>>>> +       pr_info("landlock: Version %u", LANDLOCK_VERSION);
>>>>> +       landlock_add_hooks_fs();
>>>>> +       security_add_hooks(NULL, 0, "landlock");
>>>>> +       bpf_register_prog_type(&bpf_landlock_type);
>>>> I'm confused by the separation of hook registration here. The call to
>>>> security_add_hooks is with count=0 is especially weird. Why isn't this
>>>> just a single call with security_add_hooks(landlock_hooks,
>>>> ARRAY_SIZE(landlock_hooks), "landlock")?
>>> Yes, this is ugly with the new security_add_hooks() with three arguments
>>> but I wanted to split the hooks definition in multiple files.
>>
>> Why? I'll buy a good argument, but there are dangers in
>> allowing multiple calls to security_add_hooks().

I prefer to have one file per hook "family" (e.g. filesystem, network,
ptraceâ). This reduce the mess with all the included files (needed for
LSM hook argument types) and make the files easier to read, understand
and maintain.

>>
>>>
>>> The current security_add_hooks() use lsm_append(lsm, &lsm_names) which
>>> is not exported. Unfortunately, calling multiple security_add_hooks()
>>> with the same LSM name would register multiple names for the same LSMâ
>>> Is it OK if I modify this function to not add duplicated entries?
>>
>> It may seem absurd, but it's conceivable that a module might
>> have two hooks it wants called. My example is a module that
>> counts the number of times SELinux denies a process access to
>> things (which needs to be called before and after SELinux in
>> order to detect denials) and takes "appropriate action" if
>> too many denials occur. It would be weird, wonky and hackish,
>> but that never stopped anybody before.

Right, but now, with the new lsm_append(), module names are concatenated
("%s,%s") in the lsm_names variable. It would be nice to not pollute
this string with multiple time the same module name.

> 
> If ends up being sane and clear, I'm fine with allowing multiple calls.
> 
> -Kees
>

Attachment: signature.asc
Description: OpenPGP digital signature