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  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]
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

Powered by blists - more mailing lists