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

[Dmitry Safonov]

On 12/14/2015 12:32 PM, Kirill Tkhai wrote:
>
> On 14.12.2015 13:43, Dmitry Safonov wrote:
>> 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?
> And what about this question?
Oh well, it should be
if (ret >= 0)
so module will not be allowed if module_payload_iptable_allowed returned 
zero.
Thanks, will send updated version now.
>
>>>> +
>>>> +    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