[nf-next,2/4] netfilter: add BUILD_BUG_ON asserts for hook array sizes

Message ID 20171113164107.11259-3-fw@strlen.de
State Superseded
Delegated to: Pablo Neira
Headers show
Series
  • netfilter: reduce hook sizes in struct net
Related show

Commit Message

Florian Westphal Nov. 13, 2017, 4:41 p.m.
Check that the array hooks are not accessed out-of-bounds.
Next patch will then reduce their sizes to reflect the number
of hooks implemented for each family.

Signed-off-by: Florian Westphal <fw@strlen.de>
---
 include/linux/netfilter.h | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Pablo Neira Ayuso Nov. 22, 2017, 12:20 p.m. | #1
Hi Florian,

On Mon, Nov 13, 2017 at 05:41:05PM +0100, Florian Westphal wrote:
> Check that the array hooks are not accessed out-of-bounds.
> Next patch will then reduce their sizes to reflect the number
> of hooks implemented for each family.

I'm hitting this here.

In file included from ./include/linux/kernel.h:10:0,
                 from ./include/linux/uio.h:12,
                 from ./include/linux/socket.h:8,
                 from net/decnet/dn_route.c:60:
In function ‘nf_hook.constprop’,
    inlined from ‘NF_HOOK.constprop’ at
./include/linux/netfilter.h:279:6:
./include/linux/compiler.h:319:38: error: call to
‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
                                      ^
./include/linux/compiler.h:299:4: note: in definition of macro
‘__compiletime_assert’
    prefix ## suffix();    \
    ^
./include/linux/compiler.h:319:2: note: in expansion of macro
‘_compiletime_assert’
  _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
  ^
./include/linux/build_bug.h:47:37: note: in expansion of macro
‘compiletime_assert’
 #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
                                     ^
./include/linux/build_bug.h:71:2: note: in expansion of macro
‘BUILD_BUG_ON_MSG’
  BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
  ^
./include/linux/netfilter.h:221:3: note: in expansion of macro
‘BUILD_BUG_ON’
   BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_decnet));
   ^
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Westphal Nov. 22, 2017, 12:44 p.m. | #2
Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> Hi Florian,
> 
> On Mon, Nov 13, 2017 at 05:41:05PM +0100, Florian Westphal wrote:
> > Check that the array hooks are not accessed out-of-bounds.
> > Next patch will then reduce their sizes to reflect the number
> > of hooks implemented for each family.
> 
> I'm hitting this here.
> 
> In file included from ./include/linux/kernel.h:10:0,
>                  from ./include/linux/uio.h:12,
>                  from ./include/linux/socket.h:8,
>                  from net/decnet/dn_route.c:60:
> In function ‘nf_hook.constprop’,
>     inlined from ‘NF_HOOK.constprop’ at
> ./include/linux/netfilter.h:279:6:
> ./include/linux/compiler.h:319:38: error: call to
> ‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
> failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
>   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)

Thanks for the report, I guess gcc doesn't follow inline trail
or argument isn't known at compile time.

I'll add a contant_p() test as well in v2.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Pablo Neira Ayuso Nov. 22, 2017, 12:54 p.m. | #3
On Wed, Nov 22, 2017 at 01:44:10PM +0100, Florian Westphal wrote:
> Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > Hi Florian,
> > 
> > On Mon, Nov 13, 2017 at 05:41:05PM +0100, Florian Westphal wrote:
> > > Check that the array hooks are not accessed out-of-bounds.
> > > Next patch will then reduce their sizes to reflect the number
> > > of hooks implemented for each family.
> > 
> > I'm hitting this here.
> > 
> > In file included from ./include/linux/kernel.h:10:0,
> >                  from ./include/linux/uio.h:12,
> >                  from ./include/linux/socket.h:8,
> >                  from net/decnet/dn_route.c:60:
> > In function ‘nf_hook.constprop’,
> >     inlined from ‘NF_HOOK.constprop’ at
> > ./include/linux/netfilter.h:279:6:
> > ./include/linux/compiler.h:319:38: error: call to
> > ‘__compiletime_assert_221’ declared with attribute error: BUILD_BUG_ON
> > failed: hook >= ARRAY_SIZE(net->nf.hooks_decnet)
> >   _compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
> 
> Thanks for the report, I guess gcc doesn't follow inline trail
> or argument isn't known at compile time.
> 
> I'll add a contant_p() test as well in v2.

Side note: It's only happening with decnet.
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/linux/netfilter.h b/include/linux/netfilter.h
index 80aa9a0b3d10..2e9896d42f96 100644
--- a/include/linux/netfilter.h
+++ b/include/linux/netfilter.h
@@ -197,18 +197,23 @@  static inline int nf_hook(u_int8_t pf, unsigned int hook, struct net *net,
 	rcu_read_lock();
 	switch (pf) {
 	case NFPROTO_IPV4:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_ipv4));
 		hook_head = rcu_dereference(net->nf.hooks_ipv4[hook]);
 		break;
 	case NFPROTO_IPV6:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_ipv6));
 		hook_head = rcu_dereference(net->nf.hooks_ipv6[hook]);
 		break;
 	case NFPROTO_ARP:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_arp));
 		hook_head = rcu_dereference(net->nf.hooks_arp[hook]);
 		break;
 	case NFPROTO_BRIDGE:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_bridge));
 		hook_head = rcu_dereference(net->nf.hooks_bridge[hook]);
 		break;
 	case NFPROTO_DECNET:
+		BUILD_BUG_ON(hook >= ARRAY_SIZE(net->nf.hooks_decnet));
 		hook_head = rcu_dereference(net->nf.hooks_decnet[hook]);
 		break;
 	default: