netdev - Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it

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] [day] [month] [year] [list]

Date:   Wed, 26 Jun 2019 20:02:54 +0300
From:   nikolay@...ulusnetworks.com
To:     Eyal Birger <eyal.birger@...il.com>
CC:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        pablo@...filter.org, xiyou.wangcong@...il.com, davem@...emloft.net,
        jiri@...nulli.us, jhs@...atatu.com
Subject: Re: [PATCH net-next v2 3/4] net: sched: em_ipt: keep the user-specified nfproto and use it

On 26 June 2019 19:33:48 EEST, nikolay@...ulusnetworks.com wrote:
>On 26 June 2019 19:18:35 EEST, Eyal Birger <eyal.birger@...il.com>
>wrote:
>>Hi Nik,
>>
>>On Wed, 26 Jun 2019 18:56:14 +0300
>>Nikolay Aleksandrov <nikolay@...ulusnetworks.com> wrote:
>>
>>> For NFPROTO_UNSPEC xt_matches there's no way to restrict the
>matching
>>> to a specific family, in order to do so we record the user-specified
>>> family and later enforce it while doing the match.
>>> 
>>> v2: adjust changes to missing patch, was patch 04 in v1
>>> 
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@...ulusnetworks.com>
>>> ---
>>>  net/sched/em_ipt.c | 17 +++++++++++++++--
>>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>> 
>>..snip..
>>> @@ -182,8 +195,8 @@ static int em_ipt_match(struct sk_buff *skb,
>>> struct tcf_ematch *em, const struct em_ipt_match *im = (const void
>>> *)em->data; struct xt_action_param acpar = {};
>>>  	struct net_device *indev = NULL;
>>> -	u8 nfproto = im->match->family;
>>>  	struct nf_hook_state state;
>>> +	u8 nfproto = im->nfproto;
>>
>>Maybe I'm missing something now - but it's not really clear to me now
>>why keeping im->nfproto would be useful:
>>
>>If NFPROTO_UNSPEC was provided by userspace then the actual nfproto
>>used
>>will be taken from the packet, and if NFPROTO_IPV4/IPV6 was specified
>>from userspace then it will equal im->match->family.
>>
>>Is there any case where the resulting nfproto would differ as a result
>>of this patch?
>>
>>Otherwise the patchset looks excellent to me.
>>
>>Thanks!
>>Eyal.
>
>Hi,
>It's needed to limit the match only to the user-specified family
>for unspec xt matches. The problem is otherwise im->match->family
>stays at nfproto_unspec regardless of the user choice.
>
>Thanks for reviewing the set. 
>
>Cheers,
>  Nik

Hm, while that is true, thinking more about it - mixing the user proto and the real proto
could be problematic since we no longer enforce them to be equal, but we check
the network header len based on the packet only and we can end up checking v4
len and parsing it as nfproto v6. 

I'll spin v3 with unspec only and we can restrict it later if needed.