netdev - Re: [PATCHv3] net/ipv6: add sysctl option accept_ra_min_hop

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: <55B973A6.1060607@miraclelinux.com>
Date:	Thu, 30 Jul 2015 09:45:26 +0900
From:	YOSHIFUJI Hideaki <hideaki.yoshifuji@...aclelinux.com>
To:	Hangbin Liu <liuhangbin@...il.com>
CC:	hideaki.yoshifuji@...aclelinux.com,
	network dev <netdev@...r.kernel.org>,
	Hannes Frederic Sowa <hannes@...essinduktion.org>
Subject: Re: [PATCHv3] net/ipv6: add sysctl option accept_ra_min_hop_limit

Hi,

Hangbin Liu wrote:
>>> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
>>> index 0a05b35..acda056 100644
>>> --- a/net/ipv6/ndisc.c
>>> +++ b/net/ipv6/ndisc.c
>>> @@ -1226,13 +1226,11 @@ static void ndisc_router_discovery(struct sk_buff *skb)
>>>       if (rt)
>>>               rt6_set_expires(rt, jiffies + (HZ * lifetime));
>>>       if (ra_msg->icmph.icmp6_hop_limit) {
>>> -             /* Only set hop_limit on the interface if it is higher than
>>> -              * the current hop_limit.
>>> -              */
>>> -             if (in6_dev->cnf.hop_limit < ra_msg->icmph.icmp6_hop_limit) {
>>> +             if (in6_dev->cnf.accept_ra_min_hop_limit <= ra_msg->icmph.icmp6_hop_limit &&
>>> +                 ra_msg->icmph.icmp6_hop_limit != 0) {
>>>                       in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
>>>               } else {
>>> -                     ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than current\n");
>>> +                     ND_PRINTK(2, warn, "RA: Got route advertisement with lower hop_limit than minimum\n");
>>>               }
>>>               if (rt)
>>>                       dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
>>>
>>
>> Please see my comments against your previous patch.
> 
> I pasted you comments here so we don't need to discuss in two mails :)
> 
>>
>> ra_msg->icmph.icmp6_hop_limit != 0 is checkd by outer "if".
> 
> Yes, thanks for your reminding :)
> 
>>
>> You do not need to update cnf.hop_limit if it is already equal to
>> hop limit received.
> 
> We need to update cnf.hop_limit if min_hop_limit <= icmp6_hop_limit. e.g.
> current hop limit is 64, min hop limit is 1 and ra hop limit is 32, then we need
> update current hop limit to 32.

OK

> 
>>
>> How about ignoring hop limit without message is configured value is
>> larger than 255, BTW?
> 
> Although set accept_ra_min_hop_limit great than 255 is meaningless,  there
> is also no need to check it since icmp6_hop_limit will not larger than 255. so
> 
> +               if (in6_dev->cnf.accept_ra_min_hop_limit <= 255 &&
> +                   in6_dev->cnf.accept_ra_min_hop_limit <=
> ra_msg->icmph.icmp6_hop_limit )
>                         in6_dev->cnf.hop_limit = ra_msg->icmph.icmp6_hop_limit;
> 
> is  duplicated check. How do you think?

How about checking in6_dev->cnf.accept_ra_min_hop_limit by outer if, then?


if (in6_dev->cnf.accept_ra_min_hop_limit < 256 &&
    ra_msg->icmph.icmp6_hop_limit) {
...
}

> 
>>
>>>               if (rt)
>>>                       dst_metric_set(&rt->dst, RTAX_HOPLIMIT,
>>>
>>
>> This can be inside the inner "if".
> 
> OK, will move it.
> 
> 
> Best Regards
> Hangbin
> 

Regards,

-- 
Hideaki Yoshifuji <hideaki.yoshifuji@...aclelinux.com>
Technical Division, MIRACLE LINUX CORPORATION
--
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