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

Message-ID: <06a0b358-d2f3-4248-82fb-7f599a2200a0@huawei.com>
Date: Thu, 14 Nov 2024 09:41:16 +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:19, dongchenchen (A) wrote:
>
> 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.

Sorry, I misunderstood earlier.
it's correct to return ENOENT here.
The difference from the above is:
If A is local, rt1->dst->dev is lo (fl4->dst is A),
rt2->dst->dev is actual dev selected by daddr B(fl4_2->dst is B)

>
> @@ -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;
>