Commit 7d9e5f422150 removed references from certain dsts, but accounting
for this never translated down into the fib6 suppression code. This bug
was triggered by WireGuard users who use wg-quick(8), which uses the
"suppress-prefix" directive to ip-rule(8) for routing all of their
internet traffic without routing loops. The test case in the link of
this commit reliably triggers various crashes due to the use-after-free
caused by the reference underflow.
Cc: [email protected]
Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
Test-case: https://git.zx2c4.com/WireGuard/commit/?id=ad66532000f7a20b149e47c5eb3a957362c8e161
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
net/ipv6/fib6_rules.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index d22b6c140f23..f9e8fe3ff0c5 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
return false;
suppress_route:
- ip6_rt_put(rt);
+ if (!(arg->flags & FIB_LOOKUP_NOREF))
+ ip6_rt_put(rt);
return true;
}
--
2.21.0
From: "Jason A. Donenfeld" <[email protected]>
Date: Tue, 24 Sep 2019 09:36:15 +0200
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case in the link of
> this commit reliably triggers various crashes due to the use-after-free
> caused by the reference underflow.
>
> Cc: [email protected]
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Test-case: https://git.zx2c4.com/WireGuard/commit/?id=ad66532000f7a20b149e47c5eb3a957362c8e161
> Signed-off-by: Jason A. Donenfeld <[email protected]>
Please make such test cases integratabe into the selftests area for networking
and submit it along with this fix.
Otherwise this code will regress.
Thank you.
On Tue, Sep 24, 2019 at 2:53 PM David Miller <[email protected]> wrote:
> Please make such test cases integratabe into the selftests area for networking
> and submit it along with this fix.
That link is for a WireGuard test-case. When we get that upstream,
those will all live in selftests/ all the same as you'd like. For now,
it's running for every kernel on https://build.wireguard.com/ which in
turn runs for every new commit.
From: "Jason A. Donenfeld" <[email protected]>
Date: Tue, 24 Sep 2019 14:55:04 +0200
> On Tue, Sep 24, 2019 at 2:53 PM David Miller <[email protected]> wrote:
>> Please make such test cases integratabe into the selftests area for networking
>> and submit it along with this fix.
>
> That link is for a WireGuard test-case. When we get that upstream,
> those will all live in selftests/ all the same as you'd like. For now,
> it's running for every kernel on https://build.wireguard.com/ which in
> turn runs for every new commit.
I'm asking you to make a non-wireguard test that triggers the problem.
Or would you like a situation you're interested in to break from time
to time.
Jason, please don't be difficult about this and write a proper test
case just like I would ask anyone else fixing bugs like this to write.
Thank you.
On Tue, Sep 24, 2019 at 3:30 PM David Miller <[email protected]> wrote
> I'm asking you to make a non-wireguard test that triggers the problem.
Oh, gotcha. No problem.
>
> Or would you like a situation you're interested in to break from time
> to time.
My test suite should catch it in the future now that it's there, so I'm fine.
> Jason, please don't be difficult about this and write a proper test
> case just like I would ask anyone else fixing bugs like this to write.
I'm not being difficult. I just thought you didn't see what the test
case I linked to actually was. I have no qualms about reimplementing
it with the dummy interface, for a v2 of this patch. Coming your way
shortly.
Commit 7d9e5f422150 removed references from certain dsts, but accounting
for this never translated down into the fib6 suppression code. This bug
was triggered by WireGuard users who use wg-quick(8), which uses the
"suppress-prefix" directive to ip-rule(8) for routing all of their
internet traffic without routing loops. The test case added here
causes the reference underflow by causing packets to evaluate a suppress
rule.
Cc: [email protected]
Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
net/ipv6/fib6_rules.c | 3 ++-
tools/testing/selftests/net/fib_tests.sh | 17 ++++++++++++++++-
2 files changed, 18 insertions(+), 2 deletions(-)
diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
index d22b6c140f23..f9e8fe3ff0c5 100644
--- a/net/ipv6/fib6_rules.c
+++ b/net/ipv6/fib6_rules.c
@@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
return false;
suppress_route:
- ip6_rt_put(rt);
+ if (!(arg->flags & FIB_LOOKUP_NOREF))
+ ip6_rt_put(rt);
return true;
}
diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 4465fc2dae14..c2c5f2bf0f95 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -9,7 +9,7 @@ ret=0
ksft_skip=4
# all tests in this script. Can be overridden with -t option
-TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
+TESTS="unregister down carrier nexthop suppress ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
VERBOSE=0
PAUSE_ON_FAIL=no
@@ -614,6 +614,20 @@ fib_nexthop_test()
cleanup
}
+fib_suppress_test()
+{
+ $IP link add dummy1 type dummy
+ $IP link set dummy1 up
+ $IP -6 route add default dev dummy1
+ $IP -6 rule add table main suppress_prefixlength 0
+ ping -f -c 1000 -W 1 1234::1 || true
+ $IP -6 rule del table main suppress_prefixlength 0
+ $IP link del dummy1
+
+ # If we got here without crashing, we're good.
+ return 0
+}
+
################################################################################
# Tests on route add and replace
@@ -1591,6 +1605,7 @@ do
fib_carrier_test|carrier) fib_carrier_test;;
fib_rp_filter_test|rp_filter) fib_rp_filter_test;;
fib_nexthop_test|nexthop) fib_nexthop_test;;
+ fib_suppress_test|suppress) fib_suppress_test;;
ipv6_route_test|ipv6_rt) ipv6_route_test;;
ipv4_route_test|ipv4_rt) ipv4_route_test;;
ipv6_addr_metric) ipv6_addr_metric_test;;
--
2.21.0
On Tue, Sep 24, 2019 at 7:01 AM Jason A. Donenfeld <[email protected]> wrote:
>
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case added here
> causes the reference underflow by causing packets to evaluate a suppress
> rule.
>
> Cc: [email protected]
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
Acked-by: Wei Wang <[email protected]>
Good catch. Thanks for the fix.
> net/ipv6/fib6_rules.c | 3 ++-
> tools/testing/selftests/net/fib_tests.sh | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv6/fib6_rules.c b/net/ipv6/fib6_rules.c
> index d22b6c140f23..f9e8fe3ff0c5 100644
> --- a/net/ipv6/fib6_rules.c
> +++ b/net/ipv6/fib6_rules.c
> @@ -287,7 +287,8 @@ static bool fib6_rule_suppress(struct fib_rule *rule, struct fib_lookup_arg *arg
> return false;
>
> suppress_route:
> - ip6_rt_put(rt);
> + if (!(arg->flags & FIB_LOOKUP_NOREF))
> + ip6_rt_put(rt);
> return true;
> }
>
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 4465fc2dae14..c2c5f2bf0f95 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -9,7 +9,7 @@ ret=0
> ksft_skip=4
>
> # all tests in this script. Can be overridden with -t option
> -TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
> +TESTS="unregister down carrier nexthop suppress ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
>
> VERBOSE=0
> PAUSE_ON_FAIL=no
> @@ -614,6 +614,20 @@ fib_nexthop_test()
> cleanup
> }
>
> +fib_suppress_test()
> +{
> + $IP link add dummy1 type dummy
> + $IP link set dummy1 up
> + $IP -6 route add default dev dummy1
> + $IP -6 rule add table main suppress_prefixlength 0
> + ping -f -c 1000 -W 1 1234::1 || true
> + $IP -6 rule del table main suppress_prefixlength 0
> + $IP link del dummy1
> +
> + # If we got here without crashing, we're good.
> + return 0
> +}
> +
> ################################################################################
> # Tests on route add and replace
>
> @@ -1591,6 +1605,7 @@ do
> fib_carrier_test|carrier) fib_carrier_test;;
> fib_rp_filter_test|rp_filter) fib_rp_filter_test;;
> fib_nexthop_test|nexthop) fib_nexthop_test;;
> + fib_suppress_test|suppress) fib_suppress_test;;
> ipv6_route_test|ipv6_rt) ipv6_route_test;;
> ipv4_route_test|ipv4_rt) ipv4_route_test;;
> ipv6_addr_metric) ipv6_addr_metric_test;;
> --
> 2.21.0
>
From: "Jason A. Donenfeld" <[email protected]>
Date: Tue, 24 Sep 2019 16:01:28 +0200
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case added here
> causes the reference underflow by causing packets to evaluate a suppress
> rule.
>
> Cc: [email protected]
Please don't CC: stable for networking fixes, I handle these manually as per
the netdev FAQ.
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Signed-off-by: Jason A. Donenfeld <[email protected]>
Applied and queued up for -stable, thanks.
On 9/24/19 8:01 AM, Jason A. Donenfeld wrote:
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case added here
> causes the reference underflow by causing packets to evaluate a suppress
> rule.
>
> Cc: [email protected]
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> net/ipv6/fib6_rules.c | 3 ++-
> tools/testing/selftests/net/fib_tests.sh | 17 ++++++++++++++++-
> 2 files changed, 18 insertions(+), 2 deletions(-)
Thanks for adding the test case.
Reviewed-by: David Ahern <[email protected]>