linux-kernel - Re: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter

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]
Message-ID: <20140319172557.GA31869@mcharleb3-linux.qualcomm.com>
Date:	Wed, 19 Mar 2014 10:25:57 -0700
From:	Mark Charlebois <charlebm@...il.com>
To:	David Laight <David.Laight@...LAB.COM>
Cc:	"'behanw@...verseincode.com'" <behanw@...verseincode.com>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"pablo@...filter.org" <pablo@...filter.org>,
	"kaber@...sh.net" <kaber@...sh.net>,
	"kadlec@...ckhole.kfki.hu" <kadlec@...ckhole.kfki.hu>,
	"netfilter-devel@...r.kernel.org" <netfilter-devel@...r.kernel.org>,
	"netfilter@...r.kernel.org" <netfilter@...r.kernel.org>,
	"coreteam@...filter.org" <coreteam@...filter.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"torvalds@...ux-foundation.org" <torvalds@...ux-foundation.org>,
	"dwmw2@...radead.org" <dwmw2@...radead.org>,
	"pageexec@...email.hu" <pageexec@...email.hu>,
	Vinícius Tinti <viniciustinti@...il.com>
Subject: Re: [PATCH v3] net: netfilter: LLVMLinux: vlais-netfilter

On Wed, Mar 19, 2014 at 09:52:40AM +0000, David Laight wrote:
> From: behanw@...verseincode.com 
> > From: Mark Charlebois <charlebm@...il.com>
> > 
> > Replaced non-standard C use of Variable Length Arrays In Structs (VLAIS) in
> > xt_repldata.h with a C99 compliant flexible array member and then calculated
> > offsets to the other struct members. These other members aren't referenced by
> > name in this code, however this patch maintains the same memory layout and
> > padding as was previously accomplished using VLAIS.
> > 
> > Had the original structure been ordered differently, with the entries VLA at
> > the end, then it could have been a flexible member, and this patch would have
> > been a lot simpler. However since the data stored in this structure is
> > ultimately exported to userspace, the order of this structure can't be changed.
> > 
> > This patch makes no attempt to change the existing behavior, merely the way in
> > which the current layout is accomplished using standard C99 constructs. As such
> > the code can now be compiled with either gcc or clang.
> > 
> > Author: Mark Charlebois <charlebm@...il.com>
> > Signed-off-by: Mark Charlebois <charlebm@...il.com>
> > Signed-off-by: Behan Webster <behanw@...verseincode.com>
> > Signed-off-by: Vinícius Tinti <viniciustinti@...il.com>
> > ---
> >  net/netfilter/xt_repldata.h | 27 ++++++++++++++++++++++-----
> >  1 file changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/net/netfilter/xt_repldata.h b/net/netfilter/xt_repldata.h
> > index 6efe4e5..343599e 100644
> > --- a/net/netfilter/xt_repldata.h
> > +++ b/net/netfilter/xt_repldata.h
> > @@ -5,23 +5,40 @@
> >   * they serve as the hanging-off data accessed through repl.data[].
> >   */
> > 
> > +/* tbl has the following structure equivalent, but is C99 compliant:
> > + * struct {
> > + *	struct type##_replace repl;
> > + *	struct type##_standard entries[nhooks];
> > + *	struct type##_error term;
> > + * } *tbl;
> > + */
> > +
> >  #define xt_alloc_initial_table(type, typ2) ({ \
> >  	unsigned int hook_mask = info->valid_hooks; \
> >  	unsigned int nhooks = hweight32(hook_mask); \
> >  	unsigned int bytes = 0, hooknum = 0, i = 0; \
> >  	struct { \
> >  		struct type##_replace repl; \
> > -		struct type##_standard entries[nhooks]; \
> > -		struct type##_error term; \
> > -	} *tbl = kzalloc(sizeof(*tbl), GFP_KERNEL); \
> > +		struct type##_standard entries[]; \
> > +	} *tbl; \
> > +	struct type##_error *term; \
> > +	size_t entries_end = offsetof(typeof(*tbl), \
> > +		entries[nhooks-1]) + sizeof(tbl->entries[0]); \
> 
> Is the compiler complaining about:
> 	offsetof(typeof(*tbl), entries[nhooks])
> If it does it is a PITA.
> 
> > +	size_t term_offset = (entries_end + __alignof__(*term) - 1) \
> > +		& ~(__alignof__(*term) - 1); \
> 
> You've not tested this - the () are in the wrong places.

I have tested it with both clang and gcc. Unit test is at http://git.linuxfoundation.org/?p=llvmlinux.git;a=blob;f=test/unit/vlais/netfilter.c;h=7adc255e47e15a252d2bda7af7ae217ac683c25e;hb=HEAD

Basic calulation of a new offset is:

new offset = (offset + align - 1) & ~(align - 1)

The parenthesis seem correct to me.
> 
> > +	size_t term_end = term_offset + sizeof(*term); \
> > +	size_t tbl_sz = (term_end + __alignof__(tbl->repl) - 1) \
> > +		& ~(__alignof__(tbl->repl) - 1); \
> > +	tbl = kzalloc(tbl_sz, GFP_KERNEL); \
> 
> The number of temporary variables make the above hard to read.
> I'm not at all sure you need to worry about the trailing alignment.
> It rather depends on how the final data is used.
> If the combined buffer is copied to userspace you may not
> be copying all of the required data.
> It might be easier to call copytouser() twice.

I can try to remove more variables if that is prefered.
The existing memory layout was preserved exactly so that however the
data currently used will be unaffected. I can remove the trailing
alignment but was not 100% sure it wasn't needed.

> 
> >  	if (tbl == NULL) \
> >  		return NULL; \
> > +	term = (struct type##_error *)&(((char *)tbl)[term_offset]); \
> >  	strncpy(tbl->repl.name, info->name, sizeof(tbl->repl.name)); \
> > -	tbl->term = (struct type##_error)typ2##_ERROR_INIT;  \
> > +	*term = (struct type##_error)typ2##_ERROR_INIT;  \
> 
> 	David
> 

Mark
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/