| Message ID | 1425413060-6187-3-git-send-email-kaber@trash.net |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On 05.03, Pablo Neira Ayuso wrote: > On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote: > > Check that the space required for the expressions doesn't exceed the > > size of the dlen field, which would lead to the iterators crashing. > > > > Signed-off-by: Patrick McHardy <kaber@trash.net> > > --- > > net/netfilter/nf_tables_api.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 6fb532b..7baafd5 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb, > > n++; > > } > > } > > + /* Check for overflow of dlen field */ > > + err = -EFBIG; > > Should we use -EOVERFLOW instead as we use in other nf_tables spots? > > The error in userspace will be: "Value too large for defined data type". I think the difference here is that we don't use a userspace provided value but overflow because of the size of kernel internal structures and the data type limit. I don't have any strong feelings either way, but I think its different from the cases where we use EOVERFLOW so far. -- 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
On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote: > Check that the space required for the expressions doesn't exceed the > size of the dlen field, which would lead to the iterators crashing. > > Signed-off-by: Patrick McHardy <kaber@trash.net> > --- > net/netfilter/nf_tables_api.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 6fb532b..7baafd5 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb, > n++; > } > } > + /* Check for overflow of dlen field */ > + err = -EFBIG; Should we use -EOVERFLOW instead as we use in other nf_tables spots? The error in userspace will be: "Value too large for defined data type". > + if (size >= 1 << 12) > + goto err1; > > if (nla[NFTA_RULE_USERDATA]) > ulen = nla_len(nla[NFTA_RULE_USERDATA]); > -- > 2.1.0 > -- 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
On Thu, Mar 05, 2015 at 04:31:22PM +0000, Patrick McHardy wrote: > On 05.03, Pablo Neira Ayuso wrote: > > On Tue, Mar 03, 2015 at 08:04:19PM +0000, Patrick McHardy wrote: > > > Check that the space required for the expressions doesn't exceed the > > > size of the dlen field, which would lead to the iterators crashing. > > > > > > Signed-off-by: Patrick McHardy <kaber@trash.net> > > > --- > > > net/netfilter/nf_tables_api.c | 4 ++++ > > > 1 file changed, 4 insertions(+) > > > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > > index 6fb532b..7baafd5 100644 > > > --- a/net/netfilter/nf_tables_api.c > > > +++ b/net/netfilter/nf_tables_api.c > > > @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb, > > > n++; > > > } > > > } > > > + /* Check for overflow of dlen field */ > > > + err = -EFBIG; > > > > Should we use -EOVERFLOW instead as we use in other nf_tables spots? > > > > The error in userspace will be: "Value too large for defined data type". > > I think the difference here is that we don't use a userspace provided > value but overflow because of the size of kernel internal structures and > the data type limit. OK, we also have it when we pass too large amount of data for a register IIRC. > I don't have any strong feelings either way, but I think its different > from the cases where we use EOVERFLOW so far. I don't have any strong opinion, just asking. You know we shouldn't change this afterwards. Let me know. -- 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
On 05.03, Pablo Neira Ayuso wrote: > > > Should we use -EOVERFLOW instead as we use in other nf_tables spots? > > > > > > The error in userspace will be: "Value too large for defined data type". > > > > I think the difference here is that we don't use a userspace provided > > value but overflow because of the size of kernel internal structures and > > the data type limit. > > OK, we also have it when we pass too large amount of data for a > register IIRC. Sure. But that's data we pass, in that size. In this case its data the kernel allocates, the amount of which is defined purely by the kernel. > > I don't have any strong feelings either way, but I think its different > > from the cases where we use EOVERFLOW so far. > > I don't have any strong opinion, just asking. You know we shouldn't > change this afterwards. > > Let me know. I think this case shouldn't exist at all since it codifies kernel internals into the API. Today we might accept 128 expressions, the other day just 100, all depending on 32 or 64 bit systems. Its not good. The problem is I also don't want to increase the maximum rule size to more than 4k since this will run into issues with memory fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its unreasonable large, and then add a per expression maximum size. For now I'd say apply it as it is, keep the different errno code since its a really different case, and we'll fix up the underlying problem after a bit more of thought. -- 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
On Thu, Mar 05, 2015 at 05:13:53PM +0000, Patrick McHardy wrote: > I think this case shouldn't exist at all since it codifies kernel > internals into the API. Today we might accept 128 expressions, the > other day just 100, all depending on 32 or 64 bit systems. Its not > good. > > The problem is I also don't want to increase the maximum rule size > to more than 4k since this will run into issues with memory > fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its > unreasonable large, and then add a per expression maximum size. Yes, that's more than anyone would need I guess. We used to have a couple of crazy tests in iptables with lots of matches in one rule (maps to one expression each) that were hitting the limit. So my motivation was to avoid hitting that it from nft_compat. Assuming that the info area consumes ~32 bytes and 128 expressions, we would exactly reach the limit. But there are xtables extensions consuming more than that for the info area, so we may hit the limit before those 128 expressions with this check. I think that's OK, we should consider realistic scenarios, not too crazy tests. > For now I'd say apply it as it is, keep the different errno code > since its a really different case, and we'll fix up the underlying > problem after a bit more of thought. Sure, thanks for the explanation. -- 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
On 05.03, Pablo Neira Ayuso wrote: > On Thu, Mar 05, 2015 at 05:13:53PM +0000, Patrick McHardy wrote: > > I think this case shouldn't exist at all since it codifies kernel > > internals into the API. Today we might accept 128 expressions, the > > other day just 100, all depending on 32 or 64 bit systems. Its not > > good. > > > > The problem is I also don't want to increase the maximum rule size > > to more than 4k since this will run into issues with memory > > fragmentation. I think we need to decrease NFT_EXPR_MAXNUM, its > > unreasonable large, and then add a per expression maximum size. > > Yes, that's more than anyone would need I guess. We used to have a > couple of crazy tests in iptables with lots of matches in one rule > (maps to one expression each) that were hitting the limit. So my > motivation was to avoid hitting that it from nft_compat. Assuming that > the info area consumes ~32 bytes and 128 expressions, we would exactly > reach the limit. But there are xtables extensions consuming more than > that for the info area, so we may hit the limit before those 128 > expressions with this check. I think that's OK, we should consider > realistic scenarios, not too crazy tests. Agreed, it doesn't really matter. But I still believe the API should not potentially fail because of kernel internal changes. I'd rather have strict limits that kick in *before* we use up all our space and those crazy test cases simply won't work with nftables. > > For now I'd say apply it as it is, keep the different errno code > > since its a really different case, and we'll fix up the underlying > > problem after a bit more of thought. > > Sure, thanks for the explanation. -- 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 --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c index 6fb532b..7baafd5 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -1968,6 +1968,10 @@ static int nf_tables_newrule(struct sock *nlsk, struct sk_buff *skb, n++; } } + /* Check for overflow of dlen field */ + err = -EFBIG; + if (size >= 1 << 12) + goto err1; if (nla[NFTA_RULE_USERDATA]) ulen = nla_len(nla[NFTA_RULE_USERDATA]);
Check that the space required for the expressions doesn't exceed the size of the dlen field, which would lead to the iterators crashing. Signed-off-by: Patrick McHardy <kaber@trash.net> --- net/netfilter/nf_tables_api.c | 4 ++++ 1 file changed, 4 insertions(+)