in6_selectroute should never get AF_INET filled struct route *

classic Classic list List threaded Threaded
2 messages Options
Reply | Threaded
Open this post in threaded view
|

in6_selectroute should never get AF_INET filled struct route *

Vincent Gross-5
in6_selectroute() checks whether the struct route it received contains
a valid route whose AF is not AF_INET6, "in case the cache is shared".
Well, is this cache shared or not ?

There's only two ways to get to in6_selectroute()
1) in6_pcbselsrc() -> in6_selectif() -> in6_selectroute()
It is trivial to check that only inet6 is handled here, and that any
other AF is obviously an error.

2) ip6_output() -> in6_selectroute()
  a. If the struct route * arg of ip6_output() is NULL, then
    ip6_output() zeroes a struct route from the stack, it will never
    be valid thus there is no need to check its AF.
  b. If the struct route * arg is not NULL, it is passed to
    ip6_output().

ip6_output() is called with a non-NULL struct route * in 5 places only:

netinet/tcp_output.c:1124:              error = ip6_output(m, tp->t_inpcb->inp_outputopts6,
netinet/tcp_output.c-1125-                        &tp->t_inpcb->inp_route6,
netinet/tcp_output.c-1126-                        0, NULL, tp->t_inpcb);

netinet/tcp_subr.c:399:         ip6_output(m, tp ? tp->t_inpcb->inp_outputopts6 : NULL,
netinet/tcp_subr.c-400-             tp ? &tp->t_inpcb->inp_route6 : NULL,
netinet/tcp_subr.c-401-             0, NULL,
netinet/tcp_subr.c-402-             tp ? tp->t_inpcb : NULL);

netinet/tcp_input.c:4386:               error = ip6_output(m, NULL /*XXX*/, &sc->sc_route6, 0,
netinet/tcp_input.c-4387-                   NULL, NULL);

netinet6/ip6_divert.c:167:              error = ip6_output(m, NULL, &inp->inp_route6,
netinet6/ip6_divert.c-168-                  IP_ALLOWBROADCAST | IP_RAWOUTPUT, NULL, NULL);

netinet6/raw_ip6.c:457: error = ip6_output(m, optp, &in6p->inp_route6, flags,
netinet6/raw_ip6.c-458-     in6p->inp_moptions6, in6p);

Each time, the struct route is only used in an inet6 context.

I think it is safe to add this KASSERT() to in6_selectroute(). A few
other things can be tightened, they will be addressed later.

Ok ?


Index: netinet6/in6_src.c
===================================================================
RCS file: /cvs/src/sys/netinet6/in6_src.c,v
retrieving revision 1.79
diff -u -p -r1.79 in6_src.c
--- netinet6/in6_src.c 4 Aug 2016 20:46:24 -0000 1.79
+++ netinet6/in6_src.c 2 Sep 2016 09:17:10 -0000
@@ -302,13 +302,13 @@ in6_selectroute(struct sockaddr_in6 *dst
 
  /*
  * Use a cached route if it exists and is valid, else try to allocate
- * a new one.  Note that we should check the address family of the
- * cached destination, in case of sharing the cache with IPv4.
+ * a new one.
  */
  if (ro) {
+ if (rtisvalid(ro->ro_rt))
+ KASSERT(sin6tosa(&ro->ro_dst)->sa_family == AF_INET6);
  if (!rtisvalid(ro->ro_rt) ||
-     sin6tosa(&ro->ro_dst)->sa_family != AF_INET6 ||
-     !IN6_ARE_ADDR_EQUAL(&ro->ro_dst.sin6_addr, dst)) {
+    !IN6_ARE_ADDR_EQUAL(&ro->ro_dst.sin6_addr, dst)) {
  rtfree(ro->ro_rt);
  ro->ro_rt = NULL;
  }

Reply | Threaded
Open this post in threaded view
|

Re: in6_selectroute should never get AF_INET filled struct route *

Florian Obser-2
OK florian@

On Fri, Sep 02, 2016 at 11:21:33AM +0200, Vincent Gross wrote:

> in6_selectroute() checks whether the struct route it received contains
> a valid route whose AF is not AF_INET6, "in case the cache is shared".
> Well, is this cache shared or not ?
>
> There's only two ways to get to in6_selectroute()
> 1) in6_pcbselsrc() -> in6_selectif() -> in6_selectroute()
> It is trivial to check that only inet6 is handled here, and that any
> other AF is obviously an error.
>
> 2) ip6_output() -> in6_selectroute()
>   a. If the struct route * arg of ip6_output() is NULL, then
>     ip6_output() zeroes a struct route from the stack, it will never
>     be valid thus there is no need to check its AF.
>   b. If the struct route * arg is not NULL, it is passed to
>     ip6_output().
>
> ip6_output() is called with a non-NULL struct route * in 5 places only:
>
> netinet/tcp_output.c:1124:              error = ip6_output(m, tp->t_inpcb->inp_outputopts6,
> netinet/tcp_output.c-1125-                        &tp->t_inpcb->inp_route6,
> netinet/tcp_output.c-1126-                        0, NULL, tp->t_inpcb);
>
> netinet/tcp_subr.c:399:         ip6_output(m, tp ? tp->t_inpcb->inp_outputopts6 : NULL,
> netinet/tcp_subr.c-400-             tp ? &tp->t_inpcb->inp_route6 : NULL,
> netinet/tcp_subr.c-401-             0, NULL,
> netinet/tcp_subr.c-402-             tp ? tp->t_inpcb : NULL);
>
> netinet/tcp_input.c:4386:               error = ip6_output(m, NULL /*XXX*/, &sc->sc_route6, 0,
> netinet/tcp_input.c-4387-                   NULL, NULL);
>
> netinet6/ip6_divert.c:167:              error = ip6_output(m, NULL, &inp->inp_route6,
> netinet6/ip6_divert.c-168-                  IP_ALLOWBROADCAST | IP_RAWOUTPUT, NULL, NULL);
>
> netinet6/raw_ip6.c:457: error = ip6_output(m, optp, &in6p->inp_route6, flags,
> netinet6/raw_ip6.c-458-     in6p->inp_moptions6, in6p);
>
> Each time, the struct route is only used in an inet6 context.
>
> I think it is safe to add this KASSERT() to in6_selectroute(). A few
> other things can be tightened, they will be addressed later.
>
> Ok ?
>
>
> Index: netinet6/in6_src.c
> ===================================================================
> RCS file: /cvs/src/sys/netinet6/in6_src.c,v
> retrieving revision 1.79
> diff -u -p -r1.79 in6_src.c
> --- netinet6/in6_src.c 4 Aug 2016 20:46:24 -0000 1.79
> +++ netinet6/in6_src.c 2 Sep 2016 09:17:10 -0000
> @@ -302,13 +302,13 @@ in6_selectroute(struct sockaddr_in6 *dst
>  
>   /*
>   * Use a cached route if it exists and is valid, else try to allocate
> - * a new one.  Note that we should check the address family of the
> - * cached destination, in case of sharing the cache with IPv4.
> + * a new one.
>   */
>   if (ro) {
> + if (rtisvalid(ro->ro_rt))
> + KASSERT(sin6tosa(&ro->ro_dst)->sa_family == AF_INET6);
>   if (!rtisvalid(ro->ro_rt) ||
> -     sin6tosa(&ro->ro_dst)->sa_family != AF_INET6 ||
> -     !IN6_ARE_ADDR_EQUAL(&ro->ro_dst.sin6_addr, dst)) {
> +    !IN6_ARE_ADDR_EQUAL(&ro->ro_dst.sin6_addr, dst)) {
>   rtfree(ro->ro_rt);
>   ro->ro_rt = NULL;
>   }
>

--
I'm not entirely sure you are real.