Re: [PATCH] ipv6: Fix finding best source address in ipv6_dev_get_saddr().

["YOSHIFUJI Hideaki/吉藤英明" <hideaki.yoshifuji@miraclelinux.com>]

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.

> 
> 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@miraclelinux.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@google.com>.
>> Based on patch proposed by Hajime Tazaki <thehajime@gmail.com>.
>>
>> Fixes: 9131f3de24db4dc12199aede7d931e6703e97f3b ("ipv6: Do not
>>         iterate over all interfaces when finding source address
>>         on specific interface.")
>> Signed-off-by: YOSHIFUJI Hideaki <hideaki.yoshifuji@miraclelinux.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@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
吉藤英明 <hideaki.yoshifuji@miraclelinux.com>
ミラクル・リナックス株式会社 技術本部 サポート部