netdev - Re: [PATCH] sctp: check dst validity after IPsec operations

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: <5048D774.3020008@gmail.com>
Date:	Thu, 06 Sep 2012 13:03:48 -0400
From:	Vlad Yasevich <vyasevich@...il.com>
To:	nicolas.dichtel@...nd.com
CC:	sri@...ibm.com, linux-sctp@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCH] sctp: check dst validity after IPsec operations

On 09/06/2012 12:40 PM, Nicolas Dichtel wrote:
> Le 06/09/2012 18:04, Vlad Yasevich a écrit :
>> On 09/06/2012 01:40 PM, Nicolas Dichtel wrote:
>>> dst stored in struct sctp_transport needs to be recalculated when
>>> ipsec policy
>>> are updated. We use flow_cache_genid for that.
>>>
>>> For example, if a SCTP connection is established and then an IPsec
>>> policy is
>>> set, the old SCTP flow will not be updated and thus will not use the new
>>> IPsec policy.
>>>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@...nd.com>
>>
>> why doesn't this need to be done for TCP?  What makes SCTP special in
>> this case?
> Tests prove that the pb does not exist with TCP. I made the patch some
> times ago, I will look again deeply to find the difference.
>

TCP appears to cache the flowi and uses that to re-route the packet.
However, re-route still seems predicated on dst_check()...

>>
>> ip_queue_xmit does an __sk_dst_check() which is essentially what
>> sctp_transport_dst_check() does.  That should determine if the
>> currently cached
>> route is valid or not.
> The problem is that route will not be invalidated, because dst->check()
> has no xfrm path so xfrm_dst_check() will never be called.
>

Shouldn't the cache be invalidated in this case?  If the cache is 
invalidated, that should cause a new lookup.  If the cache isn't 
invalidated, then any established connections that may now be impacted 
by the policy will not pick it up.

-vlad

>
> Regards,
> Nicolas
>
>>
>> Looks like sctp may need to change to using ip_route_output_ports() call
>> as ip_route_output_key may not do all that is necessary
>>
>> -vlad
>>
>>> ---
>>>   include/net/sctp/sctp.h    | 3 ++-
>>>   include/net/sctp/structs.h | 3 +++
>>>   net/sctp/ipv6.c            | 1 +
>>>   net/sctp/protocol.c        | 1 +
>>>   4 files changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/net/sctp/sctp.h b/include/net/sctp/sctp.h
>>> index 9c6414f..3267246 100644
>>> --- a/include/net/sctp/sctp.h
>>> +++ b/include/net/sctp/sctp.h
>>> @@ -712,7 +712,8 @@ static inline void sctp_v4_map_v6(union sctp_addr
>>> *addr)
>>>    */
>>>   static inline struct dst_entry *sctp_transport_dst_check(struct
>>> sctp_transport *t)
>>>   {
>>> -    if (t->dst && !dst_check(t->dst, 0)) {
>>> +    if ((t->dst && !dst_check(t->dst, 0)) ||
>>> +        (t->flow_cache_genid != atomic_read(&flow_cache_genid))) {
>>>           dst_release(t->dst);
>>>           t->dst = NULL;
>>>       }
>>> diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
>>> index 0fef00f..9dab882 100644
>>> --- a/include/net/sctp/structs.h
>>> +++ b/include/net/sctp/structs.h
>>> @@ -948,6 +948,9 @@ struct sctp_transport {
>>>
>>>       /* 64-bit random number sent with heartbeat. */
>>>       __u64 hb_nonce;
>>> +
>>> +    /* used to check validity of dst */
>>> +    __u32 flow_cache_genid;
>>>   };
>>>
>>>   struct sctp_transport *sctp_transport_new(struct net *, const union
>>> sctp_addr *,
>>> diff --git a/net/sctp/ipv6.c b/net/sctp/ipv6.c
>>> index ea14cb4..2ed7410 100644
>>> --- a/net/sctp/ipv6.c
>>> +++ b/net/sctp/ipv6.c
>>> @@ -349,6 +349,7 @@ out:
>>>           struct rt6_info *rt;
>>>           rt = (struct rt6_info *)dst;
>>>           t->dst = dst;
>>> +        t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>>           SCTP_DEBUG_PRINTK("rt6_dst:%pI6 rt6_src:%pI6\n",
>>>               &rt->rt6i_dst.addr, &fl6->saddr);
>>>       } else {
>>> diff --git a/net/sctp/protocol.c b/net/sctp/protocol.c
>>> index 2d51842..4795a1a 100644
>>> --- a/net/sctp/protocol.c
>>> +++ b/net/sctp/protocol.c
>>> @@ -512,6 +512,7 @@ out_unlock:
>>>       rcu_read_unlock();
>>>   out:
>>>       t->dst = dst;
>>> +    t->flow_cache_genid = atomic_read(&flow_cache_genid);
>>>       if (dst)
>>>           SCTP_DEBUG_PRINTK("rt_dst:%pI4, rt_src:%pI4\n",
>>>                     &fl4->daddr, &fl4->saddr);
>>>
>>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html