Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
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 > wrote: > > 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: > >> > On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang > >> > wrote: > >> > > On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: > >> > >> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang > >> > >> wrote: > >> > >>> 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.
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Thu, Aug 17, 2017 at 10:33 PM, Pablo Neira Ayuso wrote: > 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: >> > On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote: >> > > On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: >> > >> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang >> > >> wrote: >> > >>> 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.
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
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: > > On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote: > > > On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: > > >> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang > > >> wrote: > > >>> 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
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Wed, Aug 16, 2017 at 08:39:44PM +1200, Xin Long wrote: > On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote: > > On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: > >> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote: > >>> 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!
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Thu, Aug 17, 2017 at 5:57 PM, Cong Wang wrote:
> On Wed, Aug 16, 2017 at 1:39 AM, Xin Long wrote:
>> On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote:
>>> On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote:
On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote:
> 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.
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Wed, Aug 16, 2017 at 1:39 AM, Xin Long wrote: > On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote: >> On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: >>> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote: 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?
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Wed, Aug 9, 2017 at 7:33 AM, Cong Wang wrote: > On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: >> On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote: >>> 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...
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Mon, Aug 7, 2017 at 7:33 PM, Xin Long wrote: > On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote: >> 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...
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
On Tue, Aug 8, 2017 at 9:15 AM, Cong Wang wrote:
> (Cc'ing netfilter and Jamal)
>
> On Sat, Aug 5, 2017 at 4:35 AM, Xin Long wrote:
>> 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 :
>> # tc filter add dev eth1 parent : u32 match u32 0 0 action xt \
>> -j ECN --ecn-tcp-remove
>>
>> It could also crash kernel when using target CLUSTERIP or TPROXY.
>>
[1]
>> 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
>> ---
>> 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
Re: [PATCH net] net: sched: fix NULL pointer dereference when action calls some targets
(Cc'ing netfilter and Jamal)
On Sat, Aug 5, 2017 at 4:35 AM, Xin Long wrote:
> 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 :
> # tc filter add dev eth1 parent : 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
> ---
> 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?
[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 :
# tc filter add dev eth1 parent : 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
---
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
