netdev - Re: [PATCH v11 03/12] landlock: Refactor landlock_find_rule/insert

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]

Message-ID: <d1a8449a-cd55-90d5-8892-769ad6f702c1@huawei.com>
Date: Mon, 10 Jul 2023 15:30:35 +0300
From: "Konstantin Meskhidze (A)" <konstantin.meskhidze@...wei.com>
To: Mickaël Salaün <mic@...ikod.net>
CC: <willemdebruijn.kernel@...il.com>, <gnoack3000@...il.com>,
	<linux-security-module@...r.kernel.org>, <netdev@...r.kernel.org>,
	<netfilter-devel@...r.kernel.org>, <yusongping@...wei.com>,
	<artem.kuzin@...wei.com>
Subject: Re: [PATCH v11 03/12] landlock: Refactor
 landlock_find_rule/insert_rule



7/6/2023 5:34 PM, Mickaël Salaün пишет:
> 
> On 15/05/2023 18:13, Konstantin Meskhidze wrote:
>> Add a new landlock_key union and landlock_id structure to support
>> a socket port rule type. A struct landlock_id identifies a unique entry
>> in a ruleset: either a kernel object (e.g inode) or typed data (e.g TCP
>> port). There is one red-black tree per key type.
>> 
>> This patch also adds is_object_pointer() and get_root() helpers.
>> is_object_pointer() returns true if key type is LANDLOCK_KEY_INODE.
>> get_root() helper returns a red_black tree root pointer according to
>> a key type.
>> 
>> Refactor landlock_insert_rule() and landlock_find_rule() to support coming
>> network modifications. Adding or searching a rule in ruleset can now be
>> done thanks to a Landlock ID argument passed to these helpers.
>> 
>> Co-developed-by: Mickaël Salaün <mic@...ikod.net>
>> Signed-off-by: Mickaël Salaün <mic@...ikod.net>
>> Signed-off-by: Konstantin Meskhidze <konstantin.meskhidze@...wei.com>
>> ---
> 
> [...]
> 
>> diff --git a/security/landlock/ruleset.c b/security/landlock/ruleset.c
>> index 1f3188b4e313..deab37838f5b 100644
>> --- a/security/landlock/ruleset.c
>> +++ b/security/landlock/ruleset.c
>> @@ -35,7 +35,7 @@ static struct landlock_ruleset *create_ruleset(const u32 num_layers)
>>   		return ERR_PTR(-ENOMEM);
>>   	refcount_set(&new_ruleset->usage, 1);
>>   	mutex_init(&new_ruleset->lock);
>> -	new_ruleset->root = RB_ROOT;
>> +	new_ruleset->root_inode = RB_ROOT;
>>   	new_ruleset->num_layers = num_layers;
>>   	/*
>>   	 * hierarchy = NULL
>> @@ -68,8 +68,18 @@ static void build_check_rule(void)
>>   	BUILD_BUG_ON(rule.num_layers < LANDLOCK_MAX_NUM_LAYERS);
>>   }
>> 
>> +static bool is_object_pointer(const enum landlock_key_type key_type)
>> +{
>> +	switch (key_type) {
>> +	case LANDLOCK_KEY_INODE:
>> +		return true;
> 
>> +	}
> 
> 
> Because of enum change [1], could you please put the following block
> inside this commit's switch with a new "default:" case, and add a line
> break after the previous return like this:
> 
> \n
> default:
>> +	WARN_ON_ONCE(1);
>> +	return false;
> 
> break;
> }

   Ok. I will add "default: case.
   Thank you.
> 
>> +}
>> +
>>   static struct landlock_rule *
>> -create_rule(struct landlock_object *const object,
>> +create_rule(const struct landlock_id id,
>>   	    const struct landlock_layer (*const layers)[], const u32 num_layers,
>>   	    const struct landlock_layer *const new_layer)
>>   {
>> @@ -90,8 +100,13 @@ create_rule(struct landlock_object *const object,
>>   	if (!new_rule)
>>   		return ERR_PTR(-ENOMEM);
>>   	RB_CLEAR_NODE(&new_rule->node);
>> -	landlock_get_object(object);
>> -	new_rule->object = object;
>> +	if (is_object_pointer(id.type)) {
>> +		/* This should be catched by insert_rule(). */
>> +		WARN_ON_ONCE(!id.key.object);
>> +		landlock_get_object(id.key.object);
>> +	}
>> +
>> +	new_rule->key = id.key;
>>   	new_rule->num_layers = new_num_layers;
>>   	/* Copies the original layer stack. */
>>   	memcpy(new_rule->layers, layers,
>> @@ -102,12 +117,29 @@ create_rule(struct landlock_object *const object,
>>   	return new_rule;
>>   }
>> 
>> -static void free_rule(struct landlock_rule *const rule)
>> +static struct rb_root *get_root(struct landlock_ruleset *const ruleset,
>> +				const enum landlock_key_type key_type)
>> +{
> 
> Same here, you can remove the "root" variable:
> 
>> +	struct rb_root *root = NULL;
>> +
>> +	switch (key_type) {
>> +	case LANDLOCK_KEY_INODE:
>> +		root = &ruleset->root_inode;
>> +		break;
> 
> return &ruleset->root_inode;
> \n
> default:
>> +	if (WARN_ON_ONCE(!root))
>> +		return ERR_PTR(-EINVAL);
> break;
> }

   Ok. Will be fixed.
> 
>> +}
> 
> Actually, I've pushed this change here:
> https://git.kernel.org/mic/c/8c96c7eee3ff (landlock-net-v11 branch)

  Thank you.
> .