| 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
| ||
|
Message-ID: <CALx6S37ES1XotV4-Jtd8rLCBc+45mnyMObwtVdmNg=O8RE+OAQ@mail.gmail.com>
Date: Tue, 14 Jul 2015 08:22:00 -0700
From: Tom Herbert <tom@...bertland.com>
To: YOSHIFUJI Hideaki/吉藤英明
<hideaki.yoshifuji@...aclelinux.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Linux Kernel Network Developers <netdev@...r.kernel.org>,
Erik Kline <ek@...gle.com>, Hajime Tazaki <thehajime@...il.com>
Subject: Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().
On Tue, Jul 14, 2015 at 5:44 AM, YOSHIFUJI Hideaki/吉藤英明
<hideaki.yoshifuji@...aclelinux.com> wrote:
> Hi,
>
> Tom Herbert wrote:
>> I am testing this patch which may be a little simpler. Also idev needs
>> to be checked after __in6_dev_get
>
> We have to select source address on *given* interface for link-local/
> multicast destinations.
>
I mean check that idev is not NULL.
>>
>> Tom
>>
>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>> index 4ab74d5..d631ac3 100644
>> --- a/net/ipv6/addrconf.c
>> +++ b/net/ipv6/addrconf.c
>> @@ -1363,9 +1363,10 @@ static void __ipv6_dev_get_saddr(struct net *net,
>> unsigned int prefs,
>> const struct in6_addr *saddr,
>> struct inet6_dev *idev,
>> - struct ipv6_saddr_score *scores)
>> + struct ipv6_saddr_score **in_score,
>> + struct ipv6_saddr_score **in_hiscore)
>> {
>> - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1];
>> + struct ipv6_saddr_score *score = *in_score, *hiscore = *in_hiscore;
>>
>> read_lock_bh(&idev->lock);
>> list_for_each_entry(score->ifa, &idev->addr_list, if_list) {
>> @@ -1434,13 +1435,16 @@ static void __ipv6_dev_get_saddr(struct net *net,
>> }
>> out:
>> read_unlock_bh(&idev->lock);
>> + *in_hiscore = hiscore;
>> + *in_score = score;
>> }
>>
>> int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
>> const struct in6_addr *daddr, unsigned int prefs,
>> struct in6_addr *saddr)
>> {
>> - struct ipv6_saddr_score scores[2], *hiscore = &scores[1];
>> + struct ipv6_saddr_score scores[2];
>> + struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1];
>> struct ipv6_saddr_dst dst;
>> struct inet6_dev *idev;
>> struct net_device *dev;
>> @@ -1475,18 +1479,19 @@ int ipv6_dev_get_saddr(struct net *net, const
>> struct net_device *dst_dev,
>> if ((dst_type & IPV6_ADDR_MULTICAST) ||
>> dst.scope <= IPV6_ADDR_SCOPE_LINKLOCAL) {
>> idev = __in6_dev_get(dst_dev);
>> - use_oif_addr = true;
>> + if (idev)
>> + use_oif_addr = true;
>> }
>> }
>> if (use_oif_addr) {
>> - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores);
>> + __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev,
>> &score, &hiscore);
>> } else {
>> for_each_netdev_rcu(net, dev) {
>> idev = __in6_dev_get(dev);
>> if (!idev)
>> continue;
>> - __ipv6_dev_get_saddr(net, &dst, prefs, saddr,
>> idev, scores);
>> + __ipv6_dev_get_saddr(net, &dst, prefs, saddr,
>> idev, &score, &hiscore);
>> }
>> }
>> rcu_read_unlock();
>>
>> On Mon, Jul 13, 2015 at 7:28 AM, YOSHIFUJI Hideaki/吉藤英明
>> <hideaki.yoshifuji@...aclelinux.com> wrote:
>>> Commit 9131f3de2 ("ipv6: Do not iterate over all interfaces when
>>> finding source address on specific interface.") did not properly
>>> update best source address available. Plus, it introduced
>>> possible NULL pointer dereference.
>>>
>>> Bug was reported by Erik Kline <ek@...gle.com>.
>>> Based on patch proposed by Hajime Tazaki <thehajime@...il.com>.
>>>
>>> Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not
>>> iterate over all interfaces when finding source address
>>> on specific interface.")
>>> Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@...aclelinux.com>
>>> ---
>>> net/ipv6/addrconf.c | 30 ++++++++++++++++++------------
>>> 1 file changed, 18 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
>>> index 4ab74d5..4c9a024 100644
>>> --- a/net/ipv6/addrconf.c
>>> +++ b/net/ipv6/addrconf.c
>>> @@ -1358,14 +1358,15 @@ out:
>>> return ret;
>>> }
>>>
>>> -static void __ipv6_dev_get_saddr(struct net *net,
>>> - struct ipv6_saddr_dst *dst,
>>> - unsigned int prefs,
>>> - const struct in6_addr *saddr,
>>> - struct inet6_dev *idev,
>>> - struct ipv6_saddr_score *scores)
>>> +static int __ipv6_dev_get_saddr(struct net *net,
>>> + struct ipv6_saddr_dst *dst,
>>> + unsigned int prefs,
>>> + const struct in6_addr *saddr,
>>> + struct inet6_dev *idev,
>>> + struct ipv6_saddr_score *scores,
>>> + int hiscore_idx)
>>> {
>>> - struct ipv6_saddr_score *score = &scores[0], *hiscore = &scores[1];
>>> + struct ipv6_saddr_score *score = &scores[1 - hiscore_idx], *hiscore = &scores[hiscore_idx];
>>>
>>> read_lock_bh(&idev->lock);
>>> list_for_each_entry(score->ifa, &idev->addr_list, if_list) {
>>> @@ -1424,6 +1425,7 @@ static void __ipv6_dev_get_saddr(struct net *net,
>>> in6_ifa_hold(score->ifa);
>>>
>>> swap(hiscore, score);
>>> + hiscore_idx = 1 - hiscore_idx;
>>>
>>> /* restore our iterator */
>>> score->ifa = hiscore->ifa;
>>> @@ -1434,18 +1436,20 @@ static void __ipv6_dev_get_saddr(struct net *net,
>>> }
>>> out:
>>> read_unlock_bh(&idev->lock);
>>> + return hiscore_idx;
>>> }
>>>
>>> int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
>>> const struct in6_addr *daddr, unsigned int prefs,
>>> struct in6_addr *saddr)
>>> {
>>> - struct ipv6_saddr_score scores[2], *hiscore = &scores[1];
>>> + struct ipv6_saddr_score scores[2], *hiscore;
>>> struct ipv6_saddr_dst dst;
>>> struct inet6_dev *idev;
>>> struct net_device *dev;
>>> int dst_type;
>>> bool use_oif_addr = false;
>>> + int hiscore_idx = 0;
>>>
>>> dst_type = __ipv6_addr_type(daddr);
>>> dst.addr = daddr;
>>> @@ -1454,8 +1458,8 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
>>> dst.label = ipv6_addr_label(net, daddr, dst_type, dst.ifindex);
>>> dst.prefs = prefs;
>>>
>>> - hiscore->rule = -1;
>>> - hiscore->ifa = NULL;
>>> + scores[hiscore_idx].rule = -1;
>>> + scores[hiscore_idx].ifa = NULL;
>>>
>>> rcu_read_lock();
>>>
>>> @@ -1480,17 +1484,19 @@ int ipv6_dev_get_saddr(struct net *net, const struct net_device *dst_dev,
>>> }
>>>
>>> if (use_oif_addr) {
>>> - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores);
>>> + if (idev)
>>> + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx);
>>> } else {
>>> for_each_netdev_rcu(net, dev) {
>>> idev = __in6_dev_get(dev);
>>> if (!idev)
>>> continue;
>>> - __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores);
>>> + hiscore_idx = __ipv6_dev_get_saddr(net, &dst, prefs, saddr, idev, scores, hiscore_idx);
>>> }
>>> }
>>> rcu_read_unlock();
>>>
>>> + hiscore = &scores[hiscore_idx];
>>> if (!hiscore->ifa)
>>> return -EADDRNOTAVAIL;
>>>
>>> --
>>> 1.9.1
>>>
>>> --
>>> 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
>
> --
> 吉藤英明 <hideaki.yoshifuji@...aclelinux.com>
> ミラクル・リナックス株式会社 技術本部 サポート部
--
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
Powered by blists - more mailing lists