linux-kernel - Re: [PATCH v2 1/7] netfilter: fix IS_ERR

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] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 17 Feb 2016 16:40:50 +0100
From:	Arnd Bergmann <arnd@...db.de>
To:	Andrzej Hajda <a.hajda@...sung.com>
Cc:	linux-kernel@...r.kernel.org,
	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	viro@...iv.linux.org.uk
Subject: Re: [PATCH v2 1/7] netfilter: fix IS_ERR_VALUE usage

On Wednesday 17 February 2016 15:54:11 Andrzej Hajda wrote:
> > However, could you put the union into the three users (struct arpt_entry
> > etc) to avoid having to cast the inner structure into the union using
> > container_of()? It doesn't feel right to use container_of() in this
> > way here.
> >
> >
> I am not sure if you are aware of the fact these structures are exposed
> to user
> space. Is it OK to add such unions to them?
> 

You are right, that would be odd. My first idea was actually to put
a union into struct xt_counters, and I did notice that this was
exposed to user space so I did not mention it.

Putting a union into arpt_entry etc would be worse then. The only
alternative I see would be to define xt_counters as

struct xt_counters {
#ifndef __KERNEL__
        __u64 pcnt, bcnt;                       /* Packet and byte counters */
#else
	union {
		__u64 pcnt;
		struct xt_counters __percpu *xt_smp_counters;
	};
	__u64 bcnt;
#endif
};

but that is still really ugly, and no real improvement over your
approach.

One really simple fix would be to basically open-code a correct
version of IS_ERR_VALUE specifically for xt_counters and leave
everything using the __u64 hack:

-static inline u64 xt_percpu_counter_alloc(void)
+static inline int xt_percpu_counter_alloc(struct xt_counters *cnt)
 {
        if (nr_cpu_ids > 1) {
                void __percpu *res = __alloc_percpu(sizeof(struct xt_counters),
                                                    sizeof(struct xt_counters));
 
                if (res == NULL)
-                       return (u64) -ENOMEM;
+                       return -ENOMEM;
 
-               return (u64) (__force unsigned long) res;
+               cnt->pcnt = (u64)(uintptr_t)res;
        }
 
        return 0;
 }

that avoids the union but keeps the implicit overloading of the
pcnt field, just local to the alloc/free functions.

	Arnd