netdev - Re: [PATCH v8 04/12] landlock: Move unmask_layers() and init_layer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Nov 2022 21:25:46 +0100
From:   Mickaël Salaün <mic@...ikod.net>
To:     "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>,
        Andy Whitcroft <apw@...onical.com>,
        Joe Perches <joe@...ches.com>,
        Dwaipayan Ray <dwaipayanray1@...il.com>,
        Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc:     willemdebruijn.kernel@...il.com, gnoack3000@...il.com,
        linux-security-module@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, artem.kuzin@...wei.com
Subject: Re: [PATCH v8 04/12] landlock: Move unmask_layers() and
 init_layer_masks()


On 28/11/2022 04:25, Konstantin Meskhidze (A) wrote:
> 
> 
> 11/17/2022 9:42 PM, Mickaël Salaün пишет:
>>
>> On 21/10/2022 17:26, Konstantin Meskhidze wrote:
>>> This patch moves unmask_layers() and init_layer_masks() helpers
>>> to ruleset.c to share with landlock network implementation in
>>
>> …to share them with the Landlock network implementation in
>>
>      Got it.
>>
>>> following commits.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>>> ---

[...]

>>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>>> index 608ab356bc3e..50baff4fcbb4 100644
>>> --- a/security/landlock/ruleset.h
>>> +++ b/security/landlock/ruleset.h
>>> @@ -34,6 +34,16 @@ typedef u16 layer_mask_t;
>>>    /* Makes sure all layers can be checked. */
>>>    static_assert(BITS_PER_TYPE(layer_mask_t) >= LANDLOCK_MAX_NUM_LAYERS);
>>>
>>> +/*
>>> + * All access rights that are denied by default whether they are handled or not
>>> + * by a ruleset/layer.  This must be ORed with all ruleset->fs_access_masks[]
>>> + * entries when we need to get the absolute handled access masks.
>>> + */
>>> +/* clang-format off */
>>> +#define ACCESS_INITIALLY_DENIED ( \
>>> +	LANDLOCK_ACCESS_FS_REFER)
>>> +/* clang-format on */
>>
>> This ACCESS_INITIALLY_DENIED definition must be moved, not copied. You
>> can rename ACCESS_INITIALLY_DENIED to ACCESS_FS_INITIALLY_DENIED and
>> move this hunk before the access_mask_t definition.
>>
>     Yep. Will be fixed.
>>
>>> +
>>>    /**
>>>     * struct landlock_layer - Access rights for a given layer
>>>     */
>>> @@ -246,4 +256,14 @@ landlock_get_fs_access_mask(const struct landlock_ruleset *const ruleset,
>>>    		LANDLOCK_SHIFT_ACCESS_FS) &
>>>    	       LANDLOCK_MASK_ACCESS_FS;
>>>    }
>>> +
>>> +bool unmask_layers(const struct landlock_rule *const rule,
>>
>> All public Landlock helpers must be prefixed with "landlock_"
> 
>     Do you mean ones which are shared between fs and net parts?

All helpers that ends up in the exported ELF symbols, so all implemented 
in the .c files with their signature defined in .h files. The static 
inlined .h helpers don't need to have such prefix if there is no conflict.


>>
>>> +		   const access_mask_t access_request,
>>> +		   layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>>> +
>>> +access_mask_t
>>> +init_layer_masks(const struct landlock_ruleset *const domain,
>>> +		 const access_mask_t access_request,
>>> +		 layer_mask_t (*const layer_masks)[LANDLOCK_NUM_ACCESS_FS]);
>>
>> There is a warning generated by checkpatch.pl about this line:
>>      WARNING: function definition argument 'layer_mask_t' should also have
>> an identifier name
>>
>> I think this is a bug in checkpatch.pl
>>
>      I got this warn, but cant get rid of it.
>      Also think its a bug in checkpatck.pl

Please ignore it for now. It would be nice to have a checkpatch.pl fix 
though.

> 
>> Any though Andy, Joe, Dwaipayan or Lukas?