linux-kernel - Re: [PATCH] netfilter: ip_tables: remove useless variable assignment in get

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]

Date:   Mon, 26 Jun 2017 22:42:03 -0500
From:   "Gustavo A. R. Silva" <garsilva@...eddedor.com>
To:     Joe Perches <joe@...ches.com>
Cc:     Pablo Neira Ayuso <pablo@...filter.org>,
        Jozsef Kadlecsik <kadlec@...ckhole.kfki.hu>,
        Florian Westphal <fw@...len.de>,
        "David S. Miller" <davem@...emloft.net>,
        Alexey Kuznetsov <kuznet@....inr.ac.ru>,
        James Morris <jmorris@...ei.org>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Patrick McHardy <kaber@...sh.net>,
        netfilter-devel@...r.kernel.org, coreteam@...filter.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] netfilter: ip_tables: remove useless variable
 assignment in get_info()

Hi Joe,

Quoting Joe Perches <joe@...ches.com>:

> On Mon, 2017-06-26 at 17:34 -0500, Gustavo A. R. Silva wrote:
>> Value assigned to variable _ret_ at line 970 is overwritten either at
>> line 986 or 988, before it can be used. This makes such variable
>> assignment useless.
>>
>> Addresses-Coverity-ID: 1226932
> []
>> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> []
>> @@ -967,7 +967,7 @@ static int get_info(struct net *net, void __user *user,
>>  		struct xt_table_info tmp;
>>
>>  		if (compat) {
>> -			ret = compat_table_info(private, &tmp);
>> +			compat_table_info(private, &tmp);
>
> why isn't it more appropriate to test the return value?
>

Oh, in this particular case, based on git blame, the code has been  
like that for more than 10 years. So my reasoning was that if it  
hasn't been fixed yet, maybe that return value is not relevant.

But in case it turns out to actually be relevant, what do you think  
about the following patch:

--- a/net/ipv4/netfilter/ip_tables.c
+++ b/net/ipv4/netfilter/ip_tables.c
@@ -968,7 +968,8 @@ static int get_info(struct net *net, void __user *user,

                 if (compat) {
                         ret = compat_table_info(private, &tmp);
-                       xt_compat_flush_offsets(AF_INET);
+                       if (!ret)
+                               goto out;
                         private = &tmp;
                 }
  #endif
@@ -986,14 +987,20 @@ static int get_info(struct net *net, void __user *user,
                         ret = -EFAULT;
                 else
                         ret = 0;
+       } else
+               ret = -ENOENT;

+out:
+       if (t) {
                 xt_table_unlock(t);
                 module_put(t->me);
-       } else
-               ret = -ENOENT;
+       }
+
  #ifdef CONFIG_COMPAT
-       if (compat)
+       if (compat) {
+               xt_compat_flush_offsets(AF_INET);
                 xt_compat_unlock(AF_INET);
+       }
  #endif
         return ret;
  }


Thank you!
--
Gustavo A. R. Silva