[PATCH net] net: sched: fix NULL pointer dereference when action calls some targets

As we know in some target's checkentry it may dereference par.entryinfo
to check entry stuff inside. But when sched action calls xt_check_target,
par.entryinfo is set with NULL. It would cause kernel panic when calling
some targets.

It can be reproduce with:
  # tc qd add dev eth1 ingress handle ffff:
  # tc filter add dev eth1 parent ffff: u32 match u32 0 0 action xt \
    -j ECN --ecn-tcp-remove

It could also crash kernel when using target CLUSTERIP or TPROXY.

By now there's no proper value for par.entryinfo in ipt_init_target,
but it can not be set with NULL. This patch is to void all these
panics by setting it with an ipt_entry obj with all members 0.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_ipt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 7c4816b..0f09f70 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c

@@ -41,6 +41,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
 {
 	struct xt_tgchk_param par;
 	struct xt_target *target;
+	struct ipt_entry e;
 	int ret = 0;
 
 	target = xt_request_find_target(AF_INET, t->u.user.name,

@@ -48,10 +49,11 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
 	if (IS_ERR(target))
 		return PTR_ERR(target);
 
+	memset(&e, 0, sizeof(e));
 	t->u.kernel.target = target;
 	par.net       = net;
 	par.table     = table;
-	par.entryinfo = NULL;
+	par.entryinfo = &e;
 	par.target    = target;
 	par.targinfo  = t->data;
 	par.hook_mask = hook;

-- 
2.1.0

(Cc'ing netfilter and Jamal)

On Sat, Aug 5, 2017 at 4:35 AM, Xin Long [off-list ref] wrote:

This looks like a completely API burden?

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

(Cc'ing netfilter and Jamal)

On Sat, Aug 5, 2017 at 4:35 AM, Xin Long [off-list ref] wrote:

quoted

As we know in some target's checkentry it may dereference par.entryinfo
to check entry stuff inside. But when sched action calls xt_check_target,
par.entryinfo is set with NULL. It would cause kernel panic when calling
some targets.

It can be reproduce with:
  # tc qd add dev eth1 ingress handle ffff:
  # tc filter add dev eth1 parent ffff: u32 match u32 0 0 action xt \
    -j ECN --ecn-tcp-remove

It could also crash kernel when using target CLUSTERIP or TPROXY.

[1]

quoted

By now there's no proper value for par.entryinfo in ipt_init_target,
but it can not be set with NULL. This patch is to void all these
panics by setting it with an ipt_entry obj with all members 0.

Signed-off-by: Xin Long <lucien.xin@gmail.com>
---
 net/sched/act_ipt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/net/sched/act_ipt.c b/net/sched/act_ipt.c
index 7c4816b..0f09f70 100644
--- a/net/sched/act_ipt.c
+++ b/net/sched/act_ipt.c

@@ -41,6 +41,7 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
 {
        struct xt_tgchk_param par;
        struct xt_target *target;
+       struct ipt_entry e;
        int ret = 0;

        target = xt_request_find_target(AF_INET, t->u.user.name,

@@ -48,10 +49,11 @@ static int ipt_init_target(struct net *net, struct xt_entry_target *t,
        if (IS_ERR(target))
                return PTR_ERR(target);

+       memset(&e, 0, sizeof(e));
        t->u.kernel.target = target;
        par.net       = net;
        par.table     = table;
-       par.entryinfo = NULL;
+       par.entryinfo = &e;

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

I am not familiar with this API, so just my 2 cents...

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

I am not familiar with this API, so just my 2 cents...

On Wed, Aug 16, 2017 at 1:39 AM, Xin Long [off-list ref] wrote:

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Yeah, no objections from me.

By the way, do you know how other callers of this API
use 'entryinfo'? Do they pass the address of the struct
on stack too?

On Thu, Aug 17, 2017 at 5:57 PM, Cong Wang [off-list ref] wrote:

On Wed, Aug 16, 2017 at 1:39 AM, Xin Long [off-list ref] wrote:

quoted

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Yeah, no objections from me.

By the way, do you know how other callers of this API
use 'entryinfo'? Do they pass the address of the struct
on stack too?

afaik, two places:
1. translate_table -> find_check_entry -> check_target -> xt_check_target
most iptables operations go there and .entryinfo is set in check_target
with struct ipt_entry *e, which is an iptable rule, so can't be NULL.
(as well as ip6table in netfilter/ip6_tables.c )

2. nft_target_init -> xt_check_target, where nft_target_set_tgchk_param
does the exact thing to set .entryinfo with a local varible union nft_entry e:
union nft_entry {
        struct ipt_entry e4;
        struct ip6t_entry e6;
        struct ebt_entry ebt;
        struct arpt_entry arp;
};

case 2 is actually what nft does to use xt targets, so net/sched
action should do
the same.

On Wed, Aug 16, 2017 at 08:39:44PM +1200, Xin Long wrote:

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Please, post the workaround so the kernel doesn't crash anymore.

This is going to be very hard to fix, it's broken since the very
beginning...

Thanks!

On Thu, Aug 17, 2017 at 12:02:20PM +0200, Pablo Neira Ayuso wrote:

On Wed, Aug 16, 2017 at 08:39:44PM +1200, Xin Long wrote:

quoted

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Please, post the workaround so the kernel doesn't crash anymore.

This is going to be very hard to fix, it's broken since the very
beginning...

Wait a second, you could rename par->nft_compat to par->no_entry. From
net/sched/ you can set this to 1, so the entry checks are ignored.

I'm refering to patch 55917a21d0cc0

On Thu, Aug 17, 2017 at 10:33 PM, Pablo Neira Ayuso [off-list ref] wrote:

On Thu, Aug 17, 2017 at 12:02:20PM +0200, Pablo Neira Ayuso wrote:

quoted

On Wed, Aug 16, 2017 at 08:39:44PM +1200, Xin Long wrote:

quoted

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Please, post the workaround so the kernel doesn't crash anymore.

This is going to be very hard to fix, it's broken since the very
beginning...

Wait a second, you could rename par->nft_compat to par->no_entry. From
net/sched/ you can set this to 1, so the entry checks are ignored.

I'm refering to patch 55917a21d0cc0

par->nft_compat wasn't used to ignore the entry checks, if we rename it
as no_entry, some other targets still needs to update. like the ones in
this patch's changelog:
ECN: ecn_tg_check()
TPROXY: tproxy_tg4_check()

which means these two checks will be ignored for nft as well, that is
not what we want.
As nft has e4.ip.proto and e4.ip.invflags being set which net/sched
gets nothing. they are still different.

On Thu, Aug 17, 2017 at 11:24:00PM +1200, Xin Long wrote:

On Thu, Aug 17, 2017 at 10:33 PM, Pablo Neira Ayuso [off-list ref] wrote:

quoted

On Thu, Aug 17, 2017 at 12:02:20PM +0200, Pablo Neira Ayuso wrote:

quoted

On Wed, Aug 16, 2017 at 08:39:44PM +1200, Xin Long wrote:

quoted

On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang [off-list ref] wrote:

quoted

On Mon, Aug 7, 2017 at 7:33 PM, Xin Long [off-list ref] wrote:

quoted

On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang [off-list ref] wrote:

quoted

This looks like a completely API burden?

netfilter xt targets are not really compatible with netsched action.
I've got to say, the patch is just a way to make checkentry return
false and avoid panic. like [1] said

I don't doubt you fix a crash, I am thinking if we can
"fix" the API instead of fixing the caller.

Hi, Cong,

For now, I don't think it's possible to change APIs or  some of their targets
for the panic caused by action xt calling.

The common way should be fixed in net_sched side.

Given that the issue is very easy to triggered,
let's wait for netfilter's replies for another few days,
otherwise I will repost the fix, agree ?

Please, post the workaround so the kernel doesn't crash anymore.

This is going to be very hard to fix, it's broken since the very
beginning...

Wait a second, you could rename par->nft_compat to par->no_entry. From
net/sched/ you can set this to 1, so the entry checks are ignored.

I'm refering to patch 55917a21d0cc0

par->nft_compat wasn't used to ignore the entry checks, if we rename it
as no_entry, some other targets still needs to update. like the ones in
this patch's changelog:
ECN: ecn_tg_check()
TPROXY: tproxy_tg4_check()

which means these two checks will be ignored for nft as well, that is
not what we want.
As nft has e4.ip.proto and e4.ip.invflags being set which net/sched
gets nothing. they are still different.

I see, then just fix this crash. It's been known among people that the
number of extensions that act_ipt supports is limited.

Thanks.