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-next>] [day] [month] [year] [list]
Message-Id: <20220318202138.37161-1-justin.iurman@uliege.be>
Date:   Fri, 18 Mar 2022 21:21:38 +0100
From:   Justin Iurman <justin.iurman@...ege.be>
To:     netdev@...r.kernel.org
Cc:     davem@...emloft.net, yoshfuji@...ux-ipv6.org, dsahern@...nel.org,
        kuba@...nel.org, pabeni@...hat.com,
        Justin Iurman <justin.iurman@...ege.be>
Subject: [RFC net] Discuss seg6 potential wrong behavior

This thread aims to discuss a potential wrong behavior regarding seg6
(as well as rpl). I'm curious to know if there is a specific reason for
discarding the packet when seg6 is not enabled on an interface and when
segments_left == 0. Indeed, reading RFC8754, I'm not sure this is the
right thing to do. I think it would be more correct to process the next
header in the packet. It does not make any sense to prevent further
processing when the SRv6 node has literally nothing to do in that
specific case. For that, we need to postpone the check of accept_seg6.
And, in order to avoid a breach, we also check for accept_seg6 before
decapsulation when segments_left == 0. Any comments on this?

Also, I'm not sure why accept_seg6 is set the current way. Are we not
suppose to prioritize devconf_all? If "all" is set to 1, then it is
enabled for all interfaces. Therefore, having an OR condition looks more
correct. Right now, we need to set both "all" and the interface to 1 so
that seg6 is enabled on this specific interface. Looks odd, thoughts?

Note: this patch could also be applied to rpl, in the same way.

Signed-off-by: Justin Iurman <justin.iurman@...ege.be>
---
 net/ipv6/exthdrs.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/net/ipv6/exthdrs.c b/net/ipv6/exthdrs.c
index 77e34aec7e82..a47c05ac504e 100644
--- a/net/ipv6/exthdrs.c
+++ b/net/ipv6/exthdrs.c
@@ -374,13 +374,7 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 	idev = __in6_dev_get(skb->dev);
 
 	accept_seg6 = net->ipv6.devconf_all->seg6_enabled;
-	if (accept_seg6 > idev->cnf.seg6_enabled)
-		accept_seg6 = idev->cnf.seg6_enabled;
-
-	if (!accept_seg6) {
-		kfree_skb(skb);
-		return -1;
-	}
+	accept_seg6 |= idev->cnf.seg6_enabled;
 
 #ifdef CONFIG_IPV6_SEG6_HMAC
 	if (!seg6_hmac_validate_skb(skb)) {
@@ -392,6 +386,11 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 looped_back:
 	if (hdr->segments_left == 0) {
 		if (hdr->nexthdr == NEXTHDR_IPV6 || hdr->nexthdr == NEXTHDR_IPV4) {
+			if (!accept_seg6) {
+				kfree_skb(skb);
+				return -1;
+			}
+
 			int offset = (hdr->hdrlen + 1) << 3;
 
 			skb_postpull_rcsum(skb, skb_network_header(skb),
@@ -431,6 +430,11 @@ static int ipv6_srh_rcv(struct sk_buff *skb)
 		return -1;
 	}
 
+	if (!accept_seg6) {
+		kfree_skb(skb);
+		return -1;
+	}
+
 	if (skb_cloned(skb)) {
 		if (pskb_expand_head(skb, 0, 0, GFP_ATOMIC)) {
 			__IP6_INC_STATS(net, ip6_dst_idev(skb_dst(skb)),
-- 
2.25.1

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ