netdev - Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

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]

Date:   Mon, 22 Oct 2018 14:51:56 +0200
From:   Paolo Abeni <pabeni@...hat.com>
To:     Steffen Klassert <steffen.klassert@...unet.com>
Cc:     netdev@...r.kernel.org, Willem de Bruijn <willemb@...gle.com>
Subject: Re: [RFC PATCH v2 06/10] udp: cope with UDP GRO packet misdirection

Hi,

On Mon, 2018-10-22 at 13:43 +0200, Steffen Klassert wrote:
> On Fri, Oct 19, 2018 at 04:25:16PM +0200, Paolo Abeni wrote:
> > +
> > +static inline struct sk_buff *udp_rcv_segment(struct sock *sk,
> > +					      struct sk_buff *skb)
> > +{
> > +	struct sk_buff *segs;
> > +
> > +	/* the GSO CB lays after the UDP one, no need to save and restore any
> > +	 * CB fragment, just initialize it
> > +	 */
> > +	segs = __skb_gso_segment(skb, NETIF_F_SG, false);
> > +	if (unlikely(IS_ERR(segs)))
> > +		kfree_skb(skb);
> > +	else if (segs)
> > +		consume_skb(skb);
> > +	return segs;
> > +}
> > +
> > +
> 
> One empty line too much.

Thank you, will handle in the next iteration.

> >  #define udp_portaddr_for_each_entry(__sk, list) \
> >  	hlist_for_each_entry(__sk, list, __sk_common.skc_portaddr_node)
> >  
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 2331ac9de954..0d55145ce9f5 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1909,7 +1909,7 @@ EXPORT_SYMBOL(udp_encap_enable);
> >   * Note that in the success and error cases, the skb is assumed to
> >   * have either been requeued or freed.
> >   */
> > -static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > +static int udp_queue_rcv_one_skb(struct sock *sk, struct sk_buff *skb)
> >  {
> >  	struct udp_sock *up = udp_sk(sk);
> >  	int is_udplite = IS_UDPLITE(sk);
> > @@ -2012,6 +2012,29 @@ static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> >  	return -1;
> >  }
> >  
> > +void ip_protocol_deliver_rcu(struct net *net, struct sk_buff *skb, int proto);
> > +
> > +static int udp_queue_rcv_skb(struct sock *sk, struct sk_buff *skb)
> > +{
> > +	struct sk_buff *next, *segs;
> > +	int ret;
> > +
> > +	if (likely(!udp_unexpected_gso(sk, skb)))
> > +		return udp_queue_rcv_one_skb(sk, skb);
> > +
> > +	BUILD_BUG_ON(sizeof(struct udp_skb_cb) > SKB_SGO_CB_OFFSET);
> > +	__skb_push(skb, -skb_mac_offset(skb));
> > +	segs = udp_rcv_segment(sk, skb);
> > +	for (skb = segs; skb; skb = next) {
> > +		next = skb->next;
> > +		__skb_pull(skb, skb_transport_offset(skb));
> > +		ret = udp_queue_rcv_one_skb(sk, skb);
> 
> udp_queue_rcv_one_skb() starts with doing a xfrm4_policy_check().
> Maybe we can do this on the GSO packet instead of the segments.
> So far this code is just for handling a corner case, but this might
> change.

I thought about keeping the policy check here, but then I preferred
what looked the safest option. Perhaps we can improve with a follow-up?

Cheers,

Paolo