From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.5 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2F5E0C169C4 for ; Tue, 29 Jan 2019 22:37:35 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EF49D20856 for ; Tue, 29 Jan 2019 22:37:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727657AbfA2Whd (ORCPT ); Tue, 29 Jan 2019 17:37:33 -0500 Received: from mail-wm1-f66.google.com ([209.85.128.66]:36806 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727332AbfA2Whd (ORCPT ); Tue, 29 Jan 2019 17:37:33 -0500 Received: by mail-wm1-f66.google.com with SMTP id p6so19519586wmc.1 for ; Tue, 29 Jan 2019 14:37:32 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=DTeTmkQeTFKnoUXBbeV14pclsGcFOiw49vnaLjrAGzY=; b=kgMSRVVKND3WNg7tVzFyeRUacb/LOKgzceYznqBPw31n8gL4pjUHoxKuAQa26/9pGN e6DBMK2G35bzCAdk6oBi8gRDXYiIWu/8BLd0jkt4+X+EQAlrpo8QcadQBUNVmmQmQbmM r5PcUIYI/E5xFlIGmc2l6RMkfWMA4HuZzwPDSa7Nb1OLN4KwRZPHGyeDxqOjm4aBgVMy mRDOJN8v92FKlC8ZSrcvviotaS4rOci826Fe4FP20uXapSDIPSPPHGkCemCEdEvtjUwR RLb/8MacUIO+/ParUY3ZuAF65mvtdYMwNhINHAux6txGNCpoHBqyXgJIB4Cb+dpbNtev p1uQ== X-Gm-Message-State: AJcUukeJH8cTzdQTz+gDTn7EOz2vZBweYFF0fCtaJdyX4Mp9KKyimw/R dJEz3ntlQzoYj3YpU67QwyCoZg== X-Google-Smtp-Source: ALg8bN5vhFZEJM5kED3yHro0IBruRs+p1o8M1pSBlE8DBKpYJ9ZEzM9CDVp27sKXvw2wQWRieZaU2g== X-Received: by 2002:a1c:9a4c:: with SMTP id c73mr23721647wme.35.1548801451461; Tue, 29 Jan 2019 14:37:31 -0800 (PST) Received: from linux.home (2a01cb058382ea004233e954b48ed30d.ipv6.abo.wanadoo.fr. [2a01:cb05:8382:ea00:4233:e954:b48e:d30d]) by smtp.gmail.com with ESMTPSA id v6sm99533311wro.57.2019.01.29.14.37.30 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 29 Jan 2019 14:37:30 -0800 (PST) Date: Tue, 29 Jan 2019 23:37:28 +0100 From: Guillaume Nault To: Jacob Wen Cc: netdev@vger.kernel.org, eric.dumazet@gmail.com Subject: Re: [PATCH net v3] net: l2tp: fix reading optional fields of L2TPv3 Message-ID: <20190129223727.GA4062@linux.home> References: <20190129061813.8097-1-jian.w.wen@oracle.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190129061813.8097-1-jian.w.wen@oracle.com> User-Agent: Mutt/1.10.1 (2018-07-13) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Tue, Jan 29, 2019 at 02:18:13PM +0800, Jacob Wen wrote: > Use pskb_may_pull() to make sure the optional fields are in skb linear > parts, so we can safely read them in l2tp_recv_common. > Looks fine to me. Just a few nitpicks. Not sure if they're worth a repost. But if you send a v4, you can: * Add the proper Fixes tag. * Drop 'net:' from the subsystem prefix ('l2tp:' is enough). * Move your patch history inside the commit description. * Keep my Acked-by tag. > diff --git a/net/l2tp/l2tp_core.c b/net/l2tp/l2tp_core.c > index 26f1d435696a..82c28008b438 100644 > --- a/net/l2tp/l2tp_core.c > +++ b/net/l2tp/l2tp_core.c > @@ -884,6 +884,10 @@ static int l2tp_udp_recv_core(struct l2tp_tunnel *tunnel, struct sk_buff *skb) > goto error; > } > > + if (tunnel->version != L2TP_HDR_VER_2 && > Using tunnel->version == L2TP_HDR_VER_3 would have been clearer. > diff --git a/net/l2tp/l2tp_core.h b/net/l2tp/l2tp_core.h > index 9c9afe94d389..870f8ccf95f7 100644 > --- a/net/l2tp/l2tp_core.h > +++ b/net/l2tp/l2tp_core.h > @@ -301,6 +301,27 @@ static inline bool l2tp_tunnel_uses_xfrm(const struct l2tp_tunnel *tunnel) > } > #endif > > +/* Pull optional fields of L2TPv3 */ > +static inline int l2tp_v3_pull_opt(struct l2tp_session *session, struct sk_buff *skb, > The comment and function name are a bit misleading: nothing is pulled here. > + unsigned char **ptr, unsigned char **optr) > +{ > + int opt_len = session->peer_cookie_len + l2tp_get_l2specific_len(session); > + > + if (opt_len > 0) { > + int off = *ptr - *optr; > + > + if (!pskb_may_pull(skb, off + opt_len)) > + return -1; > + > + if (skb->data != *optr) { > + *optr = skb->data; > + *ptr = skb->data + off; > + } > + } > + > + return 0; > +} > + > #define l2tp_printk(ptr, type, func, fmt, ...) \ > do { \ > if (((ptr)->debug) & (type)) \ Fixes: f7faffa3ff8e ("l2tp: Add L2TPv3 protocol support") Fixes: 0d76751fad77 ("l2tp: Add L2TPv3 IP encapsulation (no UDP) support") Fixes: a32e0eec7042 ("l2tp: introduce L2TPv3 IP encapsulation support for IPv6") Acked-by: Guillaume Nault Thanks for reporting and fixing this. BTW, Do you plan to also fix L2TPv2? It looks like defining L2TP_HDR_SIZE_MAX to 14 (size of L2TPv2 header with all optional fields) and using it in place of L2TP_HDR_SIZE_SEQ in l2tp_udp_recv_core() should be enough.