netdev - Re: [PATCH net,v2] vrf: check forwarding on the original netdevice when generating ICMP dest unreachable

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHapkUisJkj9NNXyBsxn1uom-DekBZpKhq6k-=A=np3Fiv0dwg@mail.gmail.com>
Date:   Wed, 28 Feb 2018 11:55:14 -0500
From:   Stephen Suryaputra <ssuryaextr@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [PATCH net,v2] vrf: check forwarding on the original netdevice
 when generating ICMP dest unreachable

The concern only applies when the skb->dev is an l3mdev master, right?
After I sent v2, I'm worried that rt shouldn't be derefrenced if
in_dev is NULL. Even though I think it should be ok, it's better to
keep the original execution order. So, how about this before I put
another patch? The net for the iif is derived from skb->dev.

diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index a4f44d8..9a29225 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -930,14 +930,23 @@ void ip_rt_send_redirect(struct sk_buff *skb)

 static int ip_error(struct sk_buff *skb)
 {
-       struct in_device *in_dev = __in_dev_get_rcu(skb->dev);
        struct rtable *rt = skb_rtable(skb);
+       struct net_device *dev = skb->dev;
+       struct in_device *in_dev;
        struct inet_peer *peer;
        unsigned long now;
        struct net *net;
        bool send;
        int code;

+       if (netif_is_l3_master(skb->dev)) {
+               dev = __dev_get_by_index(dev_net(skb->dev), IPCB(skb)->iif);
+               if (!dev)
+                       goto out;
+       }
+
+       in_dev = __in_dev_get_rcu(dev);
+
        /* IP on this device is disabled. */
        if (!in_dev)
                goto out;


On Wed, Feb 28, 2018 at 10:49 AM, David Ahern <dsahern@...il.com> wrote:
> On 2/28/18 7:46 AM, Stephen Suryaputra wrote:
>> When ip_error() is called the device is the l3mdev master instead of the
>> original device. So the forwarding check should be on the original one.
>>
>> Changes from v1:
>> - Only need to reset the device on which __in_dev_get_rcu() is done (per
>>   David Ahern).
>>
>> Signed-off-by: Stephen Suryaputra <ssuryaextr@...il.com>
>> ---
>>  net/ipv4/route.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
>> index a4f44d8..89c020f 100644
>> --- a/net/ipv4/route.c
>> +++ b/net/ipv4/route.c
>> @@ -930,19 +930,26 @@ void ip_rt_send_redirect(struct sk_buff *skb)
>>
>>  static int ip_error(struct sk_buff *skb)
>>  {
>> -     struct in_device *in_dev = __in_dev_get_rcu(skb->dev);
>>       struct rtable *rt = skb_rtable(skb);
>> +     struct net_device *dev = skb->dev;
>> +     struct in_device *in_dev;
>>       struct inet_peer *peer;
>>       unsigned long now;
>>       struct net *net;
>>       bool send;
>>       int code;
>>
>> +     net = dev_net(rt->dst.dev);
>> +
>> +     if (netif_is_l3_master(skb->dev))
>> +             dev = __dev_get_by_index(net, IPCB(skb)->iif);
>
> Do need to handle the device disappearing.
> if (!dev)
>         goto out;
>
>> +
>> +     in_dev = __in_dev_get_rcu(dev);
>> +
>>       /* IP on this device is disabled. */
>>       if (!in_dev)
>>               goto out;
>>
>> -     net = dev_net(rt->dst.dev);
>>       if (!IN_DEV_FORWARD(in_dev)) {
>>               switch (rt->dst.error) {
>>               case EHOSTUNREACH:
>>
>