netdev - Re: [PATCH v2] xfrm: interface: Don't hide plain packets from netfilter

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: <20201208140013.GX4647@orbyte.nwl.cc>
Date:   Tue, 8 Dec 2020 15:00:13 +0100
From:   Phil Sutter <phil@....cc>
To:     Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc:     Steffen Klassert <steffen.klassert@...unet.com>,
        linux-crypto@...r.kernel.org, netfilter-devel@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [PATCH v2] xfrm: interface: Don't hide plain packets from
 netfilter

Hi Nicolas,

On Tue, Dec 08, 2020 at 10:02:16AM +0100, Nicolas Dichtel wrote:
> Le 07/12/2020 à 14:43, Phil Sutter a écrit :
[...]
> > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> > index aa4cdcf69d471..24af61c95b4d4 100644
> > --- a/net/xfrm/xfrm_interface.c
> > +++ b/net/xfrm/xfrm_interface.c
> > @@ -317,7 +317,8 @@ xfrmi_xmit2(struct sk_buff *skb, struct net_device *dev, struct flowi *fl)
> >  	skb_dst_set(skb, dst);
> >  	skb->dev = tdev;
> >  
> > -	err = dst_output(xi->net, skb->sk, skb);
> > +	err = NF_HOOK(skb_dst(skb)->ops->family, NF_INET_LOCAL_OUT, xi->net,
> skb->protocol must be correctly set, maybe better to use it instead of
> skb_dst(skb)->ops->family?

skb->protocol holds ETH_P_* values in network byte order, NF_HOOK()
expects an NFPROTO_* value, so this would at least not be a simple
replacement. Actually I copied the code from xfrm_output_resume() in
xfrm_output.c, where skb_dst(skb)->ops is dereferenced without checking
as well. Do you think this is risky?

> > +		      skb->sk, skb, NULL, skb_dst(skb)->dev, dst_output);
> And here, tdev instead of skb_dst(skb)->dev ?

Well yes, tdev was set to dst->dev earlier. Likewise I could use dst
directly instead of skb_dst(skb) to simplify the call a bit further.
OTOH I like how in the version above it is clear that skb's dst should
be used, irrespective of the code above (and any later changes that may
receive). No strong opinion though, so if your version is regarded the
preferred one, I'm fine with that.

Thanks, Phil