From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guillaume Nault Subject: Re: [PATCH net-next 2/3] l2tp: configure l2specific_len according to l2specific_type Date: Wed, 10 Jan 2018 18:55:17 +0100 Message-ID: <20180110175517.GA1434@alphalink.fr> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: davem@davemloft.net, netdev@vger.kernel.org, jchapman@katalix.com To: Lorenzo Bianconi Return-path: Received: from zimbra.alphalink.fr ([217.15.80.77]:50556 "EHLO zimbra.alphalink.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751333AbeAJRzV (ORCPT ); Wed, 10 Jan 2018 12:55:21 -0500 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Jan 10, 2018 at 06:20:41PM +0100, Lorenzo Bianconi wrote: > Set l2specific_len according to the L2-Specific Sublayer type specified > by the userspace and not rely on a user supplied value for sublayer len > since invalid usage of L2TP_ATTR_L2SPEC_LEN allows leaking memory on the > network and sending corrupted frames. > Moreover add a sanity check on l2specific_type provided by userspace > > Signed-off-by: Lorenzo Bianconi > --- > net/l2tp/l2tp_netlink.c | 21 ++++++++++++++++----- > 1 file changed, 16 insertions(+), 5 deletions(-) > > diff --git a/net/l2tp/l2tp_netlink.c b/net/l2tp/l2tp_netlink.c > index 48b5bf30ec50..404e5482c4e7 100644 > --- a/net/l2tp/l2tp_netlink.c > +++ b/net/l2tp/l2tp_netlink.c > @@ -550,13 +550,24 @@ static int l2tp_nl_cmd_session_create(struct sk_buff *skb, struct genl_info *inf > if (info->attrs[L2TP_ATTR_DATA_SEQ]) > cfg.data_seq = nla_get_u8(info->attrs[L2TP_ATTR_DATA_SEQ]); > > - cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT; > - if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) > + if (info->attrs[L2TP_ATTR_L2SPEC_TYPE]) { > cfg.l2specific_type = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_TYPE]); > > - cfg.l2specific_len = 4; > - if (info->attrs[L2TP_ATTR_L2SPEC_LEN]) > - cfg.l2specific_len = nla_get_u8(info->attrs[L2TP_ATTR_L2SPEC_LEN]); > + switch (cfg.l2specific_type) { > + case L2TP_L2SPECTYPE_DEFAULT: > + cfg.l2specific_len = 4; > + break; > + case L2TP_L2SPECTYPE_NONE: > + cfg.l2specific_len = 0; > + break; > + default: > + ret = -EINVAL; > + goto out_tunnel; > + } > + } else { > + cfg.l2specific_type = L2TP_L2SPECTYPE_DEFAULT; > + cfg.l2specific_len = 4; > + } > > if (info->attrs[L2TP_ATTR_COOKIE]) { > u16 len = nla_len(info->attrs[L2TP_ATTR_COOKIE]); > I think we can go one step further and remove .l2specific_len from struct l2tp_session and struct l2tp_session_cfg. We never need this information. Then we can start ignoring the L2TP_ATTR_L2SPEC_LEN attribute just like we've done with L2TP_ATTR_OFFSET.