[Devel] [PATCH] kmod: list of allowed to autoload in CT modules

[Dmitry Safonov]

On 12/11/2015 07:19 PM, Kirill Tkhai wrote:
> On 11.12.2015 16:18, Dmitry Safonov wrote:
>> Now modules that are allowed to autoload in CT are inside ve0_allowed_mod.
>> Added binfmt_misc to this list.
>> Moved module_payload_allowed function outside CONFIG_VE_IPTABLES, so it
>> will compile with config disabled.
>> Renamed ve0_am to ve0_ipt_am for better description.
>> Old logic saved as-is in module_payload_iptable_allowed, which now
>> returns int to distinguish if module name equals some in ve0_ipt_am[],
>> but module isn't allowed to load.
>> Fix misspelled enviroment -> environment in comment.
>>
>> Signed-off-by: Dmitry Safonov <dsafonov at odin.com>
> Please, see inline comments below.
>
>> ---
>>   kernel/kmod.c | 70 ++++++++++++++++++++++++++++++++++++++++++++---------------
>>   1 file changed, 53 insertions(+), 17 deletions(-)
>>
>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>> index d0cdf36..369ee64 100644
>> --- a/kernel/kmod.c
>> +++ b/kernel/kmod.c
>> @@ -199,11 +199,11 @@ static int ___request_module(bool wait, bool blacklist, char *module_name)
>>   
>>   #ifdef CONFIG_VE_IPTABLES
>>   
>> -/* ve0 allowed modules */
>> +/* ve0 allowed iptables modules */
>>   static struct {
>>   	const char *name;
>>   	u64 perm;
>> -} ve0_am[] = {
>> +} ve0_ipt_am[] = {
>>   	{ "ip_tables",		VE_IP_IPTABLES	},
>>   	{ "ip6_tables",		VE_IP_IPTABLES6	},
>>   	{ "iptable_filter",	VE_IP_FILTER	},
>> @@ -318,7 +318,7 @@ static bool nft_expr_allowed(const char *name)
>>   	/*
>>   	 * We are interested in modules like nft-expr-xxx.
>>   	 * Expressions like nft-expr-xxx-yyy currently are
>> -	 * handled in ve0_am table. So expr does not cointain
>> +	 * handled in ve0_ipt_am table. So expr does not cointain
>>   	 * minus
>>   	 */
>>   	if (!strchr(name, '-'))
>> @@ -328,23 +328,23 @@ static bool nft_expr_allowed(const char *name)
>>   }
>>   
>>   /*
>> - * module_payload_allowed - check if module functionality is allowed
>> - * 			    to be used inside current virtual enviroment.
>> + * module_payload_iptable_allowed - check if iptables functionality is allowed
>> + *			    to be used inside current virtual environment.
>>    *
>> - * Returns true if it is allowed or we're in ve0, false otherwise.
>> + * Returns:
>> + *   0 if iptable module is disallowed to load
>> + *   1 if it is allowed or we're in ve0
>> + *   -1 if module isn't iptables module
> Do we really need these 3 return values? Can't we simple return "true" or "false" here?
I suppose yes: if iptables module is disallowed to probe for CT in it's
cgroup, it will return 0 here, regardless of content in newly introduced
ve0_allowed_mod.
>
>>    */
>> -bool module_payload_allowed(const char *module)
>> +static inline int module_payload_iptable_allowed(const char *module)
>>   {
>>   	u64 permitted = get_exec_env()->ipt_mask;
>>   	int i;
>>   
>> -	if (ve_is_super(get_exec_env()))
>> -		return true;
>> -
>> -	/* Look for full module name in ve0_am table */
>> -	for (i = 0; i < ARRAY_SIZE(ve0_am); i++) {
>> -		if (!strcmp(ve0_am[i].name, module))
>> -			return mask_ipt_allow(permitted, ve0_am[i].perm);
>> +	/* Look for full module name in ve0_ipt_am table */
>> +	for (i = 0; i < ARRAY_SIZE(ve0_ipt_am); i++) {
>> +		if (!strcmp(ve0_ipt_am[i].name, module))
>> +			return mask_ipt_allow(permitted, ve0_ipt_am[i].perm);
>>   	}
>>   
>>   	/* The rest of xt_* modules is allowed in both ipv4 and ipv6 modes */
>> @@ -362,20 +362,56 @@ bool module_payload_allowed(const char *module)
>>   
>>   	/* The rest of arpt_* modules */
>>   	if (!strncmp("arpt_", module, 5))
>> -		return true;
>> +		return 1;
>>   
>>   	/* The rest of ebt_* modules */
>>   	if (!strncmp("ebt_", module, 4))
>> -		return true;
>> +		return 1;
>>   
>>   	/* The rest of nft- modules */
>>   	if (!strncmp("nft-expr-", module, 9))
>>   		return nft_expr_allowed(module + 9);
>>   
>> -	return false;
>> +	return -1;
>>   }
>> +
>> +#else  /* CONFIG_VE_IPTABLES */
>> +
>> +#define module_payload_iptable_allowed(module) -1
>> +
>>   #endif /* CONFIG_VE_IPTABLES */
>>   
>> +/* ve0 allowed modules */
>> +static const char * const ve0_allowed_mod[] = {
>> +	"binfmt_misc"
>> +};
>> +
>> +/*
>> + * module_payload_allowed - check if module functionality is allowed
>> + *			    to be used inside current virtual environment.
>> + *
>> + * Returns true if it is allowed or we're in ve0, false otherwise.
>> + */
>> +bool module_payload_allowed(const char *module)
>> +{
>> +	int i;
>> +	int ret;
>> +
>> +	if (ve_is_super(get_exec_env()))
>> +		return true;
>> +
>> +	ret = module_payload_iptable_allowed(module);
>> +	if (ret > 0)
>> +		return !!ret;
> Can't we simple return "true" here?
>
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ve0_allowed_mod); i++) {
>> +		if (!strcmp(ve0_allowed_mod[i], module))
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>   int __request_module(bool wait, const char *fmt, ...)
>>   {
>>   	char module_name[MODULE_NAME_LEN];
>>
> Thanks,
> Kirill


-- 
Regards,
Dmitry Safonov