netdev - Re: [PATCH net-next] ipv6: Reject routes configurations that specify dsfield (tos)

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220210220516.GA31389@pc-4.home>
Date:   Thu, 10 Feb 2022 23:05:16 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     Shuah Khan <skhan@...uxfoundation.org>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        David Ahern <dsahern@...nel.org>,
        Shuah Khan <shuah@...nel.org>, linux-kselftest@...r.kernel.org,
        Toke Høiland-Jørgensen <toke@...hat.com>
Subject: Re: [PATCH net-next] ipv6: Reject routes configurations that specify
 dsfield (tos)

On Thu, Feb 10, 2022 at 11:23:20AM -0700, Shuah Khan wrote:
> On 2/10/22 8:08 AM, Guillaume Nault wrote:
> > The ->rtm_tos option is normally used to route packets based on both
> > the destination address and the DS field. However it's ignored for
> > IPv6 routes. Setting ->rtm_tos for IPv6 is thus invalid as the route
> > is going to work only on the destination address anyway, so it won't
> > behave as specified.
> > 
> > Suggested-by: Toke Høiland-Jørgensen <toke@...hat.com>
> > Signed-off-by: Guillaume Nault <gnault@...hat.com>
> > ---
> > The same problem exists for ->rtm_scope. I'm working only on ->rtm_tos
> > here because IPv4 recently started to validate this option too (as part
> > of the DSCP/ECN clarification effort).
> > I'll give this patch some soak time, then send another one for
> > rejecting ->rtm_scope in IPv6 routes if nobody complains.
> > 
> >   net/ipv6/route.c                         |  6 ++++++
> >   tools/testing/selftests/net/fib_tests.sh | 13 +++++++++++++
> >   2 files changed, 19 insertions(+)
> > 
> > diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> > index f4884cda13b9..dd98a11fbdb6 100644
> > --- a/net/ipv6/route.c
> > +++ b/net/ipv6/route.c
> > @@ -5009,6 +5009,12 @@ static int rtm_to_fib6_config(struct sk_buff *skb, struct nlmsghdr *nlh,
> >   	err = -EINVAL;
> >   	rtm = nlmsg_data(nlh);
> > +	if (rtm->rtm_tos) {
> > +		NL_SET_ERR_MSG(extack,
> > +			       "Invalid dsfield (tos): option not available for IPv6");
> 
> Is this an expected failure on ipv6, in which case should this test report
> pass? Should it print "failed as expected" or is returning fail from errout
> is what should happen?

This is an expected failure. When ->rtm_tos is set, iproute2 fails with
error code 2 and prints
"Error: Invalid dsfield (tos): option not available for IPv6.".

The selftest redirects stderr to /dev/null by default (unless -v is
passed on the command line) and expects the command to fail and
return 2. So the default output is just:

IPv6 route with dsfield tests
    TEST: Reject route with dsfield                                     [ OK ]

Of course, on a kernel that accepts non-null ->rtm_tos, "[ OK ]"
becomes "[FAIL]", and the the failed tests couter is incremented.

> > +		goto errout;
> > +	}
> > +
> >   	*cfg = (struct fib6_config){
> >   		.fc_table = rtm->rtm_table,
> >   		.fc_dst_len = rtm->rtm_dst_len,
> > diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> > index bb73235976b3..e2690cc42da3 100755
> > --- a/tools/testing/selftests/net/fib_tests.sh
> > +++ b/tools/testing/selftests/net/fib_tests.sh
> > @@ -988,12 +988,25 @@ ipv6_rt_replace()
> >   	ipv6_rt_replace_mpath
> >   }
> > +ipv6_rt_dsfield()
> > +{
> > +	echo
> > +	echo "IPv6 route with dsfield tests"
> > +
> > +	run_cmd "$IP -6 route flush 2001:db8:102::/64"
> > +
> > +	# IPv6 doesn't support routing based on dsfield
> > +	run_cmd "$IP -6 route add 2001:db8:102::/64 dsfield 0x04 via 2001:db8:101::2"
> > +	log_test $? 2 "Reject route with dsfield"
> > +}
> > +
> >   ipv6_route_test()
> >   {
> >   	route_setup
> >   	ipv6_rt_add
> >   	ipv6_rt_replace
> > +	ipv6_rt_dsfield
> >   	route_cleanup
> >   }
> > 
> 
> With the above comment addressed or explained.
> 
> Reviewed-by: Shuah Khan <skhan@...uxfoundation.org>
> 
> thanks,
> -- Shuah
>