[PATCH nf] netfilter: ip6t_rt: fix rt0_hdr parsing in rt_mt6

In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
The access by ((const struct rt0_hdr *)rh)->reserved will overflow
the buffer. So this access should be moved below the 2nd call to
skb_header_pointer().

Besides, after the 2nd skb_header_pointer(), its return value should
also be checked, othersize, *rp may cause null-pointer-ref.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Reported-by: Dan Carpenter <redacted>
Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/ipv6/netfilter/ip6t_rt.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/net/ipv6/netfilter/ip6t_rt.c b/net/ipv6/netfilter/ip6t_rt.c
index 733c83d38b30..d25192949217 100644
--- a/net/ipv6/netfilter/ip6t_rt.c
+++ b/net/ipv6/netfilter/ip6t_rt.c

@@ -83,11 +83,7 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 		 !(rtinfo->flags & IP6T_RT_LEN) ||
 		  ((rtinfo->hdrlen == hdrlen) ^
 		   !!(rtinfo->invflags & IP6T_RT_INV_LEN)));
-	pr_debug("res %02X %02X %02X ",
-		 rtinfo->flags & IP6T_RT_RES,
-		 ((const struct rt0_hdr *)rh)->reserved,
-		 !((rtinfo->flags & IP6T_RT_RES) &&
-		   (((const struct rt0_hdr *)rh)->reserved)));
+	pr_debug("res flag %02X ", rtinfo->flags & IP6T_RT_RES);
 
 	ret = (segsleft_match(rtinfo->segsleft[0], rtinfo->segsleft[1],
 			      rh->segments_left,

@@ -107,7 +103,12 @@ static bool rt_mt6(const struct sk_buff *skb, struct xt_action_param *par)
 						       reserved),
 					sizeof(_reserved),
 					&_reserved);
+		if (!rp) {
+			par->hotdrop = true;
+			return false;
+		}
 
+		pr_debug("res value %02X ", *rp);
 		ret = (*rp == 0);
 	}
 

-- 
2.27.0

Xin Long [off-list ref] wrote:

In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
The access by ((const struct rt0_hdr *)rh)->reserved will overflow
the buffer. So this access should be moved below the 2nd call to
skb_header_pointer().

Besides, after the 2nd skb_header_pointer(), its return value should
also be checked, othersize, *rp may cause null-pointer-ref.

Patch looks good but I think you can just axe these pr_debug statements
instead of moving them.

Before pr_debug conversion these statments were #if-0 out, I don't think
they'll be missed if they are removed.

On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote:

Xin Long [off-list ref] wrote:

quoted

In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
The access by ((const struct rt0_hdr *)rh)->reserved will overflow
the buffer. So this access should be moved below the 2nd call to
skb_header_pointer().

Besides, after the 2nd skb_header_pointer(), its return value should
also be checked, othersize, *rp may cause null-pointer-ref.

Patch looks good but I think you can just axe these pr_debug statements
instead of moving them.

Before pr_debug conversion these statments were #if-0 out, I don't think
they'll be missed if they are removed.

Agreed.

On Tue, Oct 12, 2021 at 6:30 PM Pablo Neira Ayuso [off-list ref] wrote:

On Tue, Oct 12, 2021 at 12:02:04PM +0200, Florian Westphal wrote:

quoted

Xin Long [off-list ref] wrote:

quoted

In rt_mt6(), when it's a nonlinear skb, the 1st skb_header_pointer()
only copies sizeof(struct ipv6_rt_hdr) to _route that rh points to.
The access by ((const struct rt0_hdr *)rh)->reserved will overflow
the buffer. So this access should be moved below the 2nd call to
skb_header_pointer().

Besides, after the 2nd skb_header_pointer(), its return value should
also be checked, othersize, *rp may cause null-pointer-ref.

Patch looks good but I think you can just axe these pr_debug statements
instead of moving them.

Before pr_debug conversion these statments were #if-0 out, I don't think
they'll be missed if they are removed.

Agreed.

Posted v2. Thanks.