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

[Pablo Neira Ayuso <pablo@netfilter.org>]

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.