From patchwork Wed Jul 25 22:36:47 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Nathan Harold X-Patchwork-Id: 949402 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming-netdev@ozlabs.org Delivered-To: patchwork-incoming-netdev@ozlabs.org Authentication-Results: ozlabs.org; spf=none (mailfrom) smtp.mailfrom=vger.kernel.org (client-ip=209.132.180.67; helo=vger.kernel.org; envelope-from=netdev-owner@vger.kernel.org; receiver=) Authentication-Results: ozlabs.org; dmarc=pass (p=reject dis=none) header.from=google.com Authentication-Results: ozlabs.org; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="k7i5ycXj"; dkim-atps=neutral Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 41bVTt0PW0z9s2g for ; Thu, 26 Jul 2018 08:37:22 +1000 (AEST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731710AbeGYXvH (ORCPT ); Wed, 25 Jul 2018 19:51:07 -0400 Received: from mail-qt0-f201.google.com ([209.85.216.201]:46339 "EHLO mail-qt0-f201.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729182AbeGYXvH (ORCPT ); Wed, 25 Jul 2018 19:51:07 -0400 Received: by mail-qt0-f201.google.com with SMTP id i10-v6so7235473qtp.13 for ; Wed, 25 Jul 2018 15:37:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:message-id:mime-version:subject:from:to:cc; bh=iLEapf7XVDcUCncLjtBHawF17w9LSBcASGRrLwfihMQ=; b=k7i5ycXjwbRN5/I8GkWEPX/E0OaBn2qZC6GA/nPK4HJx/x+YnQrE4LeV4uIxIurso1 6lw+Fa05exOJwql3Q8ay+MajHxyXEJv9/84gqPmdI3XPSUTOCGseDS9zSxVS7Gkj9lvP QdYIH/zsczSyMLMssSwUBYgErdHLhe4+4brTG2A1svhpLRDE5Bg6qysyqCN0cXzcrmwB HM+eQVw6UjF537W472Mt3z8KisXHoHGFDLqSPw4oBPq73PmDk5yDt4eR/PXrNeco1jus sJ6+q0m0rK2PKu/JwScCvBItysEnDP61XaAE05rp/EKakwX4s+1nmCQtT/EswSGSXPZT DgZA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:message-id:mime-version:subject:from:to:cc; bh=iLEapf7XVDcUCncLjtBHawF17w9LSBcASGRrLwfihMQ=; b=mMLwpBTpSBh/jESIwQY/lja578C3mkIivVbJAkB5wbmlnE+evLEbhb9Rwj4bHw4yO1 W2nySxjBUYbQOsygdqNU3K+L6NjD83islX9duQLduLDeaITamRTQM2+Df4AvOMxxTit3 n3WVG/WiIv54i3ZsAPYcRL1k2BoaMJ7KNUL8x5eQWl4EEPfK18mw+od58P10Ex/keqJb QCmwxqkaiBoeJ3h3sJgj/7Z5L4pMjl8DYQglGZHVPt/E2B+DFTRhP/+KlUgv561sFb4q xR/aEucv6kkSU8IB90HJDQEViKQzaDT3Cg3yXGB5ROQead4zJDEzSHaMSHp9FOAtvBZ4 Rvqw== X-Gm-Message-State: AOUpUlEVzAVBP1H65tjltpVlvp1KpIegiX2T0/jPY/PtrVOIqKoDleqC sRV+jhyOXlWv7G5a+QnO/+szqyyc/6SYhG4k6dSuK/LhUTCNJFaMmBUyucX4dj3xkRvWPbOFGfZ btqDfevGicYvt8ceDUEFRoKRZPQwVjuGcxSXJMpVEK/gn4BYIeMXOoPexBZj2DD0M X-Google-Smtp-Source: AAOMgpcMk1iri0d4QtGuABTL9XvNjuMifvJkwYqFULuX7eklVCmc9hUCbAnwvnjmfFvtPobCmcphUxZBaeUW X-Received: by 2002:a0c:e745:: with SMTP id g5-v6mr12022580qvn.6.1532558239072; Wed, 25 Jul 2018 15:37:19 -0700 (PDT) Date: Wed, 25 Jul 2018 15:36:47 -0700 Message-Id: <20180725223647.141180-1-nharold@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.18.0.345.g5c9ce644c3-goog Subject: [PATCH RFC ipsec-next] xfrm: Check Reverse-Mark Lookup Before ADDSA/DELSA From: Nathan Harold To: netdev@vger.kernel.org Cc: Nathan Harold Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org It's possible to insert an SA into the SADB that will preclude the lookup of other SAs when using the MARK attribute. The problem occurs based on a particular sequencing with the marks where a new mark matches requests for the SA mark of an existing SA but the inverse is not true. For an example: 1) Add SA with mark=0, mask=0 2) Add an otherwise-identical SA with mark=0x1234, mask=0xFFFFFFFF This will fail; however, if done in the reverse order the second add will succeed: 1) Add an SA with mark=0x1234, mask=0xFFFFFFFF 2) Add an otherwise-identical SA except with mark=0, mask=0 Then: 3) Delete the SA using mark=0x1234, mask=0xFFFFFFF 4) Dump the SADB, and there will be one SA, and it will have mark=0x1234, mask=0xFFFFFFFF; the 0/0 SA will be deleted This patch addresses the problem by performing a reverse-match on the mark and preventing ADDSA for any SA that would 'shadow' an existing SA in the SADB. This patch also address a bug where it was possible to add an SA with a mark broader than its mask, which could never be deleted: 1) ADDSA with mark=0x1234, mask=0xFF 2) DELSA with mark=0x1234, mask=ZZZZ; error=ESRCH By applying both masks to each mark, bits outside the mask of a given SA mark will be ignored, and the match will succeed. This patch does not make any changes to the 'data' path, so SAs with such oddly-defined marks will still be unmatch-able. Signed-off-by: Nathan Harold --- net/xfrm/xfrm_state.c | 44 +++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c index b669262682c9..ee212a7c91a9 100644 --- a/net/xfrm/xfrm_state.c +++ b/net/xfrm/xfrm_state.c @@ -815,10 +815,10 @@ xfrm_init_tempstate(struct xfrm_state *x, const struct flowi *fl, afinfo->init_temprop(x, tmpl, daddr, saddr); } -static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark, - const xfrm_address_t *daddr, - __be32 spi, u8 proto, - unsigned short family) +static struct xfrm_state * +__xfrm_state_lookup(struct net *net, u32 mark, u32 mask, + const xfrm_address_t *daddr, + __be32 spi, u8 proto, unsigned short family) { unsigned int h = xfrm_spi_hash(net, daddr, spi, proto, family); struct xfrm_state *x; @@ -830,7 +830,7 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark, !xfrm_addr_equal(&x->id.daddr, daddr, family)) continue; - if ((mark & x->mark.m) != x->mark.v) + if ((mark ^ x->mark.v) & mask & x->mark.m) continue; if (!xfrm_state_hold_rcu(x)) continue; @@ -840,10 +840,11 @@ static struct xfrm_state *__xfrm_state_lookup(struct net *net, u32 mark, return NULL; } -static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark, - const xfrm_address_t *daddr, - const xfrm_address_t *saddr, - u8 proto, unsigned short family) +static struct xfrm_state * +__xfrm_state_lookup_byaddr(struct net *net, u32 mark, u32 mask, + const xfrm_address_t *daddr, + const xfrm_address_t *saddr, + u8 proto, unsigned short family) { unsigned int h = xfrm_src_hash(net, daddr, saddr, family); struct xfrm_state *x; @@ -855,7 +856,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(struct net *net, u32 mark, !xfrm_addr_equal(&x->props.saddr, saddr, family)) continue; - if ((mark & x->mark.m) != x->mark.v) + if ((mark ^ x->mark.v) & mask & x->mark.m) continue; if (!xfrm_state_hold_rcu(x)) continue; @@ -869,15 +870,14 @@ static inline struct xfrm_state * __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family) { struct net *net = xs_net(x); - u32 mark = x->mark.v & x->mark.m; if (use_spi) - return __xfrm_state_lookup(net, mark, &x->id.daddr, - x->id.spi, x->id.proto, family); + return __xfrm_state_lookup(net, x->mark.v, x->mark.m, + &x->id.daddr, x->id.spi, + x->id.proto, family); else - return __xfrm_state_lookup_byaddr(net, mark, - &x->id.daddr, - &x->props.saddr, + return __xfrm_state_lookup_byaddr(net, x->mark.v, x->mark.m, + &x->id.daddr, &x->props.saddr, x->id.proto, family); } @@ -985,8 +985,10 @@ xfrm_state_find(const xfrm_address_t *daddr, const xfrm_address_t *saddr, x = best; if (!x && !error && !acquire_in_progress) { if (tmpl->id.spi && - (x0 = __xfrm_state_lookup(net, mark, daddr, tmpl->id.spi, - tmpl->id.proto, encap_family)) != NULL) { + (x0 = __xfrm_state_lookup(net, mark, 0xFFFFFFFF, + daddr, tmpl->id.spi, + tmpl->id.proto, + encap_family)) != NULL) { to_put = x0; error = -EEXIST; goto out; @@ -1617,7 +1619,8 @@ xfrm_state_lookup(struct net *net, u32 mark, const xfrm_address_t *daddr, __be32 struct xfrm_state *x; rcu_read_lock(); - x = __xfrm_state_lookup(net, mark, daddr, spi, proto, family); + x = __xfrm_state_lookup(net, mark, 0xFFFFFFFF, + daddr, spi, proto, family); rcu_read_unlock(); return x; } @@ -1631,7 +1634,8 @@ xfrm_state_lookup_byaddr(struct net *net, u32 mark, struct xfrm_state *x; spin_lock_bh(&net->xfrm.xfrm_state_lock); - x = __xfrm_state_lookup_byaddr(net, mark, daddr, saddr, proto, family); + x = __xfrm_state_lookup_byaddr(net, mark, 0xFFFFFFFF, + daddr, saddr, proto, family); spin_unlock_bh(&net->xfrm.xfrm_state_lock); return x; }