diff mbox

[nf,2/5] netfilter: nfnl_cthelper: fix incorrect helper->expect_class_max

Message ID 1489934162-7415-3-git-send-email-zlpnobody@163.com
State Changes Requested
Delegated to: Pablo Neira
Headers show

Commit Message

Liping Zhang March 19, 2017, 2:35 p.m. UTC 
From: Liping Zhang <zlpnobody@gmail.com>

The helper->expect_class_max must be set to the total number of
expect_policy minus 1, since we will use the statement "if (class >
helper->expect_class_max)" to validate the CTA_EXPECT_CLASS attr in
ctnetlink_alloc_expect.

So for compatibility, set the helper->expect_class_max to the
NFCTH_POLICY_SET_NUM attr's value minus 1.

Also: it's invalid when the NFCTH_POLICY_SET_NUM attr's value is zero.
1. this will result "expect_policy = kzalloc(0, GFP_KERNEL);";
2. we cannot set the helper->expect_class_max to a proper value.

So if nla_get_be32(tb[NFCTH_POLICY_SET_NUM]) is zero, report -EINVAL to
the userspace.

Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
---
 net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

Comments

Pablo Neira Ayuso March 21, 2017, 10:27 a.m. UTC | #1 
On Sun, Mar 19, 2017 at 10:35:59PM +0800, Liping Zhang wrote:
> From: Liping Zhang <zlpnobody@gmail.com>
> 
> The helper->expect_class_max must be set to the total number of
> expect_policy minus 1, since we will use the statement "if (class >
> helper->expect_class_max)" to validate the CTA_EXPECT_CLASS attr in
> ctnetlink_alloc_expect.
> 
> So for compatibility, set the helper->expect_class_max to the
> NFCTH_POLICY_SET_NUM attr's value minus 1.
> 
> Also: it's invalid when the NFCTH_POLICY_SET_NUM attr's value is zero.
> 1. this will result "expect_policy = kzalloc(0, GFP_KERNEL);";
> 2. we cannot set the helper->expect_class_max to a proper value.
> 
> So if nla_get_be32(tb[NFCTH_POLICY_SET_NUM]) is zero, report -EINVAL to
> the userspace.
> 
> Signed-off-by: Liping Zhang <zlpnobody@gmail.com>
> ---
>  net/netfilter/nfnetlink_cthelper.c | 20 +++++++++++---------
>  1 file changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
> index 9c24301..cc70dd5 100644
> --- a/net/netfilter/nfnetlink_cthelper.c
> +++ b/net/netfilter/nfnetlink_cthelper.c
> @@ -161,6 +161,7 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  	int i, ret;
>  	struct nf_conntrack_expect_policy *expect_policy;
>  	struct nlattr *tb[NFCTH_POLICY_SET_MAX+1];
> +	unsigned int class_max;
>  
>  	ret = nla_parse_nested(tb, NFCTH_POLICY_SET_MAX, attr,
>  			       nfnl_cthelper_expect_policy_set);
> @@ -170,19 +171,18 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  	if (!tb[NFCTH_POLICY_SET_NUM])
>  		return -EINVAL;
>  
> -	helper->expect_class_max =
> -		ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> -
> -	if (helper->expect_class_max != 0 &&
> -	    helper->expect_class_max > NF_CT_MAX_EXPECT_CLASSES)
> +	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> +	if (class_max == 0)
> +		return -EINVAL;

I think this patch is just fixing up this case. We should always
provide a policy for the expectation.

> +	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
>  		return -EOVERFLOW;
>  
>  	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
> -				helper->expect_class_max, GFP_KERNEL);
> +				class_max, GFP_KERNEL);
>  	if (expect_policy == NULL)
>  		return -ENOMEM;
>  
> -	for (i=0; i<helper->expect_class_max; i++) {
> +	for (i = 0; i < class_max; i++) {
>  		if (!tb[NFCTH_POLICY_SET+i])
>  			goto err;
>  
> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>  		if (ret < 0)
>  			goto err;
>  	}
> +
> +	helper->expect_class_max = class_max - 1;

Why - 1 ?

class_max represents the array size, you can just keep this code the
way it is.

>  	helper->expect_policy = expect_policy;
>  	return 0;
>  err:
> @@ -375,10 +377,10 @@ nfnl_cthelper_dump_policy(struct sk_buff *skb,
>  		goto nla_put_failure;
>  
>  	if (nla_put_be32(skb, NFCTH_POLICY_SET_NUM,
> -			 htonl(helper->expect_class_max)))
> +			 htonl(helper->expect_class_max + 1)))
>  		goto nla_put_failure;
>  
> -	for (i=0; i<helper->expect_class_max; i++) {
> +	for (i = 0; i < helper->expect_class_max + 1; i++) {
>  		nest_parms2 = nla_nest_start(skb,
>  				(NFCTH_POLICY_SET+i) | NLA_F_NESTED);
>  		if (nest_parms2 == NULL)
> -- 
> 2.5.5
> 
> 
--
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
Liping Zhang March 21, 2017, 2:35 p.m. UTC | #2 
Hi Pablo,

2017-03-21 18:27 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
[...]
>> +     class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
>> +     if (class_max == 0)
>> +             return -EINVAL;
>
> I think this patch is just fixing up this case. We should always
> provide a policy for the expectation.

No, see comments below.

>
>> +     if (class_max > NF_CT_MAX_EXPECT_CLASSES)
>>               return -EOVERFLOW;
>>
>>       expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
>> -                             helper->expect_class_max, GFP_KERNEL);
>> +                             class_max, GFP_KERNEL);
>>       if (expect_policy == NULL)
>>               return -ENOMEM;
>>
>> -     for (i=0; i<helper->expect_class_max; i++) {
>> +     for (i = 0; i < class_max; i++) {
>>               if (!tb[NFCTH_POLICY_SET+i])
>>                       goto err;
>>
>> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
>>               if (ret < 0)
>>                       goto err;
>>       }
>> +
>> +     helper->expect_class_max = class_max - 1;
>
> Why - 1 ?
>
> class_max represents the array size, you can just keep this code the
> way it is.

Actually, expect_class_max represents the array size minus 1.

For sip, expect_class_max is set to SIP_EXPECT_MAX(3). For ftp,
expect_class_max is set to 0.

Also there's a "BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);"
in nf_conntrack_helper_register, so if we supply 4 expect_policys,
i.e. expect_class_max is 4, BUG_ON will happen.
--
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 March 21, 2017, 2:49 p.m. UTC | #3 
On Tue, Mar 21, 2017 at 10:35:43PM +0800, Liping Zhang wrote:
> Hi Pablo,
> 
> 2017-03-21 18:27 GMT+08:00 Pablo Neira Ayuso <pablo@netfilter.org>:
> [...]
> >> +     class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
> >> +     if (class_max == 0)
> >> +             return -EINVAL;
> >
> > I think this patch is just fixing up this case. We should always
> > provide a policy for the expectation.
> 
> No, see comments below.
> 
> >
> >> +     if (class_max > NF_CT_MAX_EXPECT_CLASSES)
> >>               return -EOVERFLOW;
> >>
> >>       expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
> >> -                             helper->expect_class_max, GFP_KERNEL);
> >> +                             class_max, GFP_KERNEL);
> >>       if (expect_policy == NULL)
> >>               return -ENOMEM;
> >>
> >> -     for (i=0; i<helper->expect_class_max; i++) {
> >> +     for (i = 0; i < class_max; i++) {
> >>               if (!tb[NFCTH_POLICY_SET+i])
> >>                       goto err;
> >>
> >> @@ -191,6 +191,8 @@ nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
> >>               if (ret < 0)
> >>                       goto err;
> >>       }
> >> +
> >> +     helper->expect_class_max = class_max - 1;
> >
> > Why - 1 ?
> >
> > class_max represents the array size, you can just keep this code the
> > way it is.
> 
> Actually, expect_class_max represents the array size minus 1.
> 
> For sip, expect_class_max is set to SIP_EXPECT_MAX(3). For ftp,
> expect_class_max is set to 0.
> 
> Also there's a "BUG_ON(me->expect_class_max >= NF_CT_MAX_EXPECT_CLASSES);"
> in nf_conntrack_helper_register, so if we supply 4 expect_policys,
> i.e. expect_class_max is 4, BUG_ON will happen.

Right, from the kernel side, all helpers assume that
me->expect_class_max represents the array size - 1.

While cthelper takes this as the array size.

This expect_class_max semantics is counterintuitive to me.

Applied, thanks.

P.S: I'm going to send a new version of:

http://patchwork.ozlabs.org/patch/741528/

that applies on top of this one.
--
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
diff mbox

Patch

diff --git a/net/netfilter/nfnetlink_cthelper.c b/net/netfilter/nfnetlink_cthelper.c
index 9c24301..cc70dd5 100644
--- a/net/netfilter/nfnetlink_cthelper.c
+++ b/net/netfilter/nfnetlink_cthelper.c
@@ -161,6 +161,7 @@  nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	int i, ret;
 	struct nf_conntrack_expect_policy *expect_policy;
 	struct nlattr *tb[NFCTH_POLICY_SET_MAX+1];
+	unsigned int class_max;
 
 	ret = nla_parse_nested(tb, NFCTH_POLICY_SET_MAX, attr,
 			       nfnl_cthelper_expect_policy_set);
@@ -170,19 +171,18 @@  nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 	if (!tb[NFCTH_POLICY_SET_NUM])
 		return -EINVAL;
 
-	helper->expect_class_max =
-		ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
-
-	if (helper->expect_class_max != 0 &&
-	    helper->expect_class_max > NF_CT_MAX_EXPECT_CLASSES)
+	class_max = ntohl(nla_get_be32(tb[NFCTH_POLICY_SET_NUM]));
+	if (class_max == 0)
+		return -EINVAL;
+	if (class_max > NF_CT_MAX_EXPECT_CLASSES)
 		return -EOVERFLOW;
 
 	expect_policy = kzalloc(sizeof(struct nf_conntrack_expect_policy) *
-				helper->expect_class_max, GFP_KERNEL);
+				class_max, GFP_KERNEL);
 	if (expect_policy == NULL)
 		return -ENOMEM;
 
-	for (i=0; i<helper->expect_class_max; i++) {
+	for (i = 0; i < class_max; i++) {
 		if (!tb[NFCTH_POLICY_SET+i])
 			goto err;
 
@@ -191,6 +191,8 @@  nfnl_cthelper_parse_expect_policy(struct nf_conntrack_helper *helper,
 		if (ret < 0)
 			goto err;
 	}
+
+	helper->expect_class_max = class_max - 1;
 	helper->expect_policy = expect_policy;
 	return 0;
 err:
@@ -375,10 +377,10 @@  nfnl_cthelper_dump_policy(struct sk_buff *skb,
 		goto nla_put_failure;
 
 	if (nla_put_be32(skb, NFCTH_POLICY_SET_NUM,
-			 htonl(helper->expect_class_max)))
+			 htonl(helper->expect_class_max + 1)))
 		goto nla_put_failure;
 
-	for (i=0; i<helper->expect_class_max; i++) {
+	for (i = 0; i < helper->expect_class_max + 1; i++) {
 		nest_parms2 = nla_nest_start(skb,
 				(NFCTH_POLICY_SET+i) | NLA_F_NESTED);
 		if (nest_parms2 == NULL)