On Wed, Dec 03, 2014 at 02:24:50PM +0800, Duan Jiong wrote:
>
> Now if the copy_to_user() fail, __do_replace just silent error, because
> new table is alread replaced. But at the beginning of __do_replace(), if
> we notice that user supply no memory to store counters, we should not
> replace table, so we can just returen -EFAULT directly.
This is independent of the change you mention, right?
I mean, userspace can send us null counters and we'll crash.
Another nitpick comment below:
> Signed-off-by: Duan Jiong <duanj.fnst@xxxxxxxxxxxxxx>
> ---
> net/bridge/netfilter/ebtables.c | 3 +++
> net/ipv4/netfilter/arp_tables.c | 4 ++++
> net/ipv4/netfilter/ip_tables.c | 4 ++++
> net/ipv6/netfilter/ip6_tables.c | 4 ++++
> 4 files changed, 15 insertions(+)
>
> diff --git a/net/bridge/netfilter/ebtables.c b/net/bridge/netfilter/ebtables.c
> index d9a8c05..90ccc78 100644
> --- a/net/bridge/netfilter/ebtables.c
> +++ b/net/bridge/netfilter/ebtables.c
> @@ -983,6 +983,9 @@ static int do_replace_finish(struct net *net, struct ebt_replace *repl,
> struct ebt_table_info *table;
> struct ebt_table *t;
>
> + if (!repl->counters)
> + return -EFAULT;
> +
> /* the user wants counters back
> the check on the size is done later, when we have the lock */
> if (repl->num_counters) {
> diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c
> index f95b6f9..654debe 100644
> --- a/net/ipv4/netfilter/arp_tables.c
> +++ b/net/ipv4/netfilter/arp_tables.c
> @@ -999,6 +999,10 @@ static int __do_replace(struct net *net, const char *name,
> struct arpt_entry *iter;
>
> ret = 0;
> + if (!counters_ptr) {
> + ret = -EFAULT;
> + goto out;
> + }
Please move this check before 'ret = 0'. So we avoid the unnecessary
initialization. Same thing below.
> counters = vzalloc(num_counters * sizeof(struct xt_counters));
> if (!counters) {
> ret = -ENOMEM;
> diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c
> index 99e810f..0acdcc8 100644
> --- a/net/ipv4/netfilter/ip_tables.c
> +++ b/net/ipv4/netfilter/ip_tables.c
> @@ -1186,6 +1186,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
> struct ipt_entry *iter;
>
> ret = 0;
> + if (!counters_ptr) {
> + ret = -EFAULT;
> + goto out;
> + }
> counters = vzalloc(num_counters * sizeof(struct xt_counters));
> if (!counters) {
> ret = -ENOMEM;
> diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c
> index e080fbb..cb91f2b 100644
> --- a/net/ipv6/netfilter/ip6_tables.c
> +++ b/net/ipv6/netfilter/ip6_tables.c
> @@ -1196,6 +1196,10 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks,
> struct ip6t_entry *iter;
>
> ret = 0;
> + if (!counters_ptr) {
> + ret = -EFAULT;
> + goto out;
> + }
> counters = vzalloc(num_counters * sizeof(struct xt_counters));
> if (!counters) {
> ret = -ENOMEM;
> --
> 1.8.3.1
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
