linux-kernel - Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying default helper assignment

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: <CA+55aFxiM=GscZtvGkWnSfo3L4MTK0J6OFR5VbisZ1feTCBW9g@mail.gmail.com>
Date:   Wed, 25 Jan 2017 11:13:03 -0800
From:   Linus Torvalds <torvalds@...ux-foundation.org>
To:     Jiri Kosina <jikos@...nel.org>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
        Florian Westphal <fw@...len.de>,
        NetFilter <netfilter-devel@...r.kernel.org>,
        coreteam@...filter.org,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        info@...lonka.cz, eric@...it.org
Subject: Re: [PATCH v2] netfilter: nf_ct_helper: warn when not applying
 default helper assignment

On Tue, Jan 24, 2017 at 2:17 AM, Jiri Kosina <jikos@...nel.org> wrote:
> +       if (!helper) {
> +               if (unlikely(!net->ct.sysctl_auto_assign_helper &&
> +                               !net->ct.auto_assign_helper_warned &&
> +                               __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple))) {
> +                       pr_info("nf_conntrack: default automatic helper assignment "
> +                               "has been turned off for security reasons "
> +                               "and CT-based firewall rule not found. Use the "
> +                               "iptables CT target to attach helpers instead.\n");
> +                       net->ct.auto_assign_helper_warned = true;
> +               } else {
> +                       helper = __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple);
> +                       if (unlikely(!net->ct.auto_assign_helper_warned && helper &&
> +                                       !net->ct.auto_assign_helper_warned)) {
>                         pr_info("nf_conntrack: automatic helper "
>                                 "assignment is deprecated and it will "
>                                 "be removed soon. Use the iptables CT target "
>                                 "to attach helpers instead.\n");
>                         net->ct.auto_assign_helper_warned = true;
> +                       }
>                 }
>         }

I don't disagree that this kind of warning might be useful, but that
code makes my eyes bleed, and is really really hard to follow.

Please make it a helper function. And don't have crazy conditionals
with else statements and other crazy conditionals. With random
likely/unlikely things that are not necessariyl even true.

For example, you can rewrite the logic something like

    static struct nf_conntrack_helper *find_auto_helper(struct nf_conn *ct)
    {
            return __nf_ct_helper_find(&ct->tuplehash[IP_CT_DIR_REPLY].tuple;
    }

    static struct nf_conntrack_helper *ct_lookup_helper(struct nf_conn
*ct, struct net *net)
    {
        struct nf_conntrack_helper *ret;

        if (!net->ct.sysctl_auto_assign_helper) {
                if (net->ct.auto_assign_helper_warned)
                        return NULL;
                if (!find_auto_helper(ct))
                        return NULL;

                .. warn about helper existing but not used ..

                net->ct.auto_assign_helper_warned = 1;
                return NULL;
        }

        ret = find_auto_helper(ct);
        if (!ret || net->ct.auto_assign_helper_warned)
                return ret;

        ... warn about helper existing but automatic helpers deprecated..

        net->ct.auto_assign_helper_warned = 1;
        return ret;
    }

and now each particular case is a lot easier to follow. Then you just have

        if (!helper) {
                helper = ct_lookup_helper(ct, net);
                if (!helper) {
                        if (help)
                                RCU_INIT_POINTER(help->helper, NULL);
                        return 0;
                }
         }

in __nf_ct_try_assign_helper()

All of the above is entirely untested and just written in my email
client. It may be garbage. It's not meant to be used, it's meant to
just illustrate avoiding complex nested conditionals. It's a few more
lines, but now each part has simple logic and is much more
understandable.

                       Linus