netdev - Re: [PATCH net] net: Fix icmp host relookup triggering ip_rt

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: <97456f7d-83f3-404c-bc42-0a5f00af023e@huawei.com>
Date: Wed, 13 Nov 2024 22:19:27 +0800
From: "dongchenchen (A)" <dongchenchen2@...wei.com>
To: Herbert Xu <herbert@...dor.apana.org.au>, Steffen Klassert
	<steffen.klassert@...unet.com>
CC: <davem@...emloft.net>, <dsahern@...nel.org>, <edumazet@...gle.com>,
	<pabeni@...hat.com>, <horms@...nel.org>, <netdev@...r.kernel.org>,
	<yuehaibing@...wei.com>, <zhangchangzhong@...wei.com>
Subject: Re: [PATCH net] net: Fix icmp host relookup triggering ip_rt_bug


On 2024/11/13 22:13, dongchenchen (A) wrote:
>>> If skb_in is outbound, fl4_dec.saddr is not nolocal. It may be no input
>>> route from B to A for
>>>
>>> first communication.
>> You're right.  So the problem here is that for the case of A
>> being local, we should not be taking the ip_route_input code
>> path (this is intended for forwarded packets).
>>
>> In fact if A is local, and we're sending an ICMP message to A,
>> then perhaps we could skip the IPsec lookup completely and just
>> do normal routing?
>>
>> Steffen, what do you think?
>>
>> So I think it boils down to two choices:
>>
>> 1) We could simply drop IPsec processing if we notice that A
>> (fl.fl4_dst) is local;
>
> Hi, Herbert! Thanks for your suggestions very much.
>
> maybe we can fix it as below:
> diff --git a/net/ipv4/icmp.c b/net/ipv4/icmp.c
> index e1384e7331d8..5f63c4dde4e9 100644
> --- a/net/ipv4/icmp.c
> +++ b/net/ipv4/icmp.c
> @@ -517,7 +517,9 @@ static struct rtable *icmp_route_lookup(struct net 
> *net,
>                           flowi4_to_flowi(fl4), NULL, 0);
>         rt = dst_rtable(dst);
>         if (!IS_ERR(dst)) {
> -               if (rt != rt2)
> +               unsigned int addr_type = inet_addr_type_dev_table(net, 
> route_lookup_dev, fl4->daddr);
> +               if (rt != rt2 || addr_type == RTN_LOCAL)
>                         return rt;
>         } else if (PTR_ERR(dst) == -EPERM) {
>                 rt = NULL;
>> 2) Or we could take the __ip_route_output_key code path and
>> continue to do the xfrm lookup when A is local.

sorry,  resend it due to format problems

If only skip input route lookup as below, xfrm_lookup check may return 
ENOENT
for no xfrm pol or dst nofrm flag.

@@ -543,6 +544,10 @@ static struct rtable *icmp_route_lookup(struct net 
*net,
                         err = PTR_ERR(rt2);
                         goto relookup_failed;
                 }
+
+               unsigned int addr_type = inet_addr_type_dev_table(net, 
route_lookup_dev, fl4->daddr);
+               if (addr_type == RTN_LOCAL)
+                       goto relookup;
                 /* Ugh! */
                 orefdst = skb_in->_skb_refdst; /* save old refdst */

xfrm_lookup
     xfrm_lookup_with_ifid
         if (!if_id && ((dst_orig->flags & DST_NOXFRM) ||
             !net->xfrm.policy_count[XFRM_POLICY_OUT]))
             goto nopol;

Thanks a lot!
Dong Chenchen

> If only skip input route lookup as below, xfrm_lookup check may return 
> ENOENT
> for no xfrm pol or dst nofrm flag.
>
> @@ -543,6 +544,10 @@ static struct rtable *icmp_route_lookup(struct 
> net *net, err = PTR_ERR(rt2); goto relookup_failed; } + + unsigned int 
> addr_type = inet_addr_type_dev_table(net, route_lookup_dev, 
> fl4->daddr); + if (addr_type == RTN_LOCAL) + goto relookup; /* Ugh! */ 
> orefdst = skb_in->_skb_refdst; /* save old refdst */
>
> xfrm_lookup
> xfrm_lookup_with_ifid
>       if (!if_id && ((dst_orig->flags & DST_NOXFRM) ||
> !net->xfrm.policy_count[XFRM_POLICY_OUT]))
>         goto nopol;
>
> Thanks a lot!
> Dong Chenchen
>
>> Thanks,