Re: [PATCH v5 15/15] samples/landlock: adds network demo

["Mickaël Salaün" <mic@digikod.net>]

On 19/05/2022 15:33, Konstantin Meskhidze wrote:
> 
> 
> 5/17/2022 12:19 PM, Mickaël Salaün пишет:
>>
>>
>> On 16/05/2022 17:20, Konstantin Meskhidze wrote:
>>> This commit adds network demo. It's possible to
>>> allow a sandoxer to bind/connect to a list of
>>> particular ports restricting networks actions to
>>> the rest of ports.
>>>
>>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@huawei.com>
>>> ---
>>>
>>> Changes since v4:
>>> * Adds ENV_TCP_BIND_NAME "LL_TCP_BIND" and
>>> ENV_TCP_CONNECT_NAME "LL_TCP_CONNECT" variables
>>> to insert TCP ports.
>>> * Renames populate_ruleset() to populate_ruleset_fs().
>>> * Adds populate_ruleset_net() and parse_port_num() helpers.
>>> * Refactoring main() to support network sandboxing.
>>>
>>> ---

[...]

>>>       if (ruleset_fd < 0) {
>>>           perror("Failed to create a ruleset");
>>>           return 1;
>>>       }
>>> -    if (populate_ruleset(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro)) {
>>> +    if (populate_ruleset_fs(ENV_FS_RO_NAME, ruleset_fd, access_fs_ro))
>>>           goto err_close_ruleset;
>>> -    }
>>
>> Why? I know that checkpatch.pl prints a warning for that but I 
>> delibirately chooe to use curly braces even for "if" statements with 
>> one line because it is safer. This code may be copied/pasted and I'd 
>> like others to avoid introducing goto-fail-like issues.
>>
> 
>   It was done just to reduce the number of checkpatch.pl warnings.
>   If you want it to be formated in your way I will fix it.

Yes please, checkpatch.pl helps to mantain kernel code but this is a 
user space code and I prefer to follow safe practices for this kind of 
checks.

[...]

>>> diff --git a/security/landlock/ruleset.h b/security/landlock/ruleset.h
>>> index 916b30b31c06..e1ff40f238a6 100644
>>> --- a/security/landlock/ruleset.h
>>> +++ b/security/landlock/ruleset.h
>>> @@ -19,7 +19,7 @@
>>>   #include "limits.h"
>>>   #include "object.h"
>>>
>>> -typedef u16 access_mask_t;
>>> +typedef u32 access_mask_t;
>>
>> What‽
> 
>    You are right. I will move this changes to another commit, related 
> the kernel updates. I might have forgotten to rebase this change and 
> left it in sandboxer patch. Thank you..

Indeed. Please check that every commit build (without warning) and that 
the related tests are OK.


>>
>>
>>>
>>>   /* Makes sure all filesystem access rights can be stored. */
>>>   static_assert(BITS_PER_TYPE(access_mask_t) >= LANDLOCK_NUM_ACCESS_FS);
>>> @@ -157,7 +157,7 @@ struct landlock_ruleset {
>>>                * layers are set once and never changed for the
>>>                * lifetime of the ruleset.
>>>                */
>>> -            u32 access_masks[];
>>> +            access_mask_t access_masks[];
>>>           };
>>>       };
>>>   };
>>> -- 
>>> 2.25.1
>>>
>> .