| Message ID | 1395670237-3841-1-git-send-email-pablo@netfilter.org |
|---|---|
| State | Superseded |
| Headers | show |
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > index 33045a5..43ae487 100644 > --- a/net/netfilter/nf_tables_api.c > +++ b/net/netfilter/nf_tables_api.c > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = { > [NFTA_SET_TABLE] = { .type = NLA_STRING }, > - [NFTA_SET_NAME] = { .type = NLA_STRING }, > + [NFTA_SET_NAME] = { .type = NLA_STRING, > + .len = IFNAMSIZ - 1 }, Not related to your patch, but it looks to me as if all of these should be NLA_NUL_STRING, no? -- 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 Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > index 33045a5..43ae487 100644 > > --- a/net/netfilter/nf_tables_api.c > > +++ b/net/netfilter/nf_tables_api.c > > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const > > > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = { > > [NFTA_SET_TABLE] = { .type = NLA_STRING }, > > - [NFTA_SET_NAME] = { .type = NLA_STRING }, > > + [NFTA_SET_NAME] = { .type = NLA_STRING, > > + .len = IFNAMSIZ - 1 }, > > Not related to your patch, but it looks to me as if all of these > should be NLA_NUL_STRING, no? This is what I originally thought. But all of the nla_* functions use the nla_len to know length of the string coming from userspace, so they don't rely on the trailing nul-termination. Anyway, I'd appreciate if you can do a second look at this to make sure I'm not overlooking anything. Thanks. -- 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 <pablo@netfilter.org> wrote: > On Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote: > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > > index 33045a5..43ae487 100644 > > > --- a/net/netfilter/nf_tables_api.c > > > +++ b/net/netfilter/nf_tables_api.c > > > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const > > > > > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = { > > > [NFTA_SET_TABLE] = { .type = NLA_STRING }, > > > - [NFTA_SET_NAME] = { .type = NLA_STRING }, > > > + [NFTA_SET_NAME] = { .type = NLA_STRING, > > > + .len = IFNAMSIZ - 1 }, > > > > Not related to your patch, but it looks to me as if all of these > > should be NLA_NUL_STRING, no? > > This is what I originally thought. But all of the nla_* functions use > the nla_len to know length of the string coming from userspace, so > they don't rely on the trailing nul-termination. Indeed. I reviewed this again but only 'bug' I spotted: nf_tables_chain_type_lookup() does: request_module("nft-chain-%u-%*.s", .... ('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is NLA_NUL_STR so the erroneous fmt could also be changed to plain %s. -- 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 Wed, Mar 26, 2014 at 01:32:50AM +0100, Florian Westphal wrote: > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > On Mon, Mar 24, 2014 at 11:41:05PM +0100, Florian Westphal wrote: > > > Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > diff --git a/net/netfilter/nf_tables_api.c b/net/netfilter/nf_tables_api.c > > > > index 33045a5..43ae487 100644 > > > > --- a/net/netfilter/nf_tables_api.c > > > > +++ b/net/netfilter/nf_tables_api.c > > > > @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const > > > > > > > > static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = { > > > > [NFTA_SET_TABLE] = { .type = NLA_STRING }, > > > > - [NFTA_SET_NAME] = { .type = NLA_STRING }, > > > > + [NFTA_SET_NAME] = { .type = NLA_STRING, > > > > + .len = IFNAMSIZ - 1 }, > > > > > > Not related to your patch, but it looks to me as if all of these > > > should be NLA_NUL_STRING, no? > > > > This is what I originally thought. But all of the nla_* functions use > > the nla_len to know length of the string coming from userspace, so > > they don't rely on the trailing nul-termination. > > Indeed. I reviewed this again but only 'bug' I spotted: > > nf_tables_chain_type_lookup() does: > request_module("nft-chain-%u-%*.s", .... > > ('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is > NLA_NUL_STR so the erroneous fmt could also be changed to plain %s. I think we can change that to NLA_STRING to make it consistent with other attributes. libnftnl is anyway sending the nul-termination all the time, so nothing should break IMO. -- 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 <pablo@netfilter.org> wrote: > > > This is what I originally thought. But all of the nla_* functions use > > > the nla_len to know length of the string coming from userspace, so > > > they don't rely on the trailing nul-termination. > > > > Indeed. I reviewed this again but only 'bug' I spotted: > > > > nf_tables_chain_type_lookup() does: > > request_module("nft-chain-%u-%*.s", .... > > > > ('%.*s' was probably intended). Its harmless though as NFTA_CHAIN_TYPE is > > NLA_NUL_STR so the erroneous fmt could also be changed to plain %s. > > I think we can change that to NLA_STRING to make it consistent with > other attributes. libnftnl is anyway sending the nul-termination all > the time, so nothing should break IMO. Right, but the kernel shouldn't rely on that :) I see no problem converting it to NLA_STRING (if request_module args are fixed). -- 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 33045a5..43ae487 100644 --- a/net/netfilter/nf_tables_api.c +++ b/net/netfilter/nf_tables_api.c @@ -1946,7 +1946,8 @@ static const struct nft_set_ops *nft_select_set_ops(const struct nlattr * const static const struct nla_policy nft_set_policy[NFTA_SET_MAX + 1] = { [NFTA_SET_TABLE] = { .type = NLA_STRING }, - [NFTA_SET_NAME] = { .type = NLA_STRING }, + [NFTA_SET_NAME] = { .type = NLA_STRING, + .len = IFNAMSIZ - 1 }, [NFTA_SET_FLAGS] = { .type = NLA_U32 }, [NFTA_SET_KEY_TYPE] = { .type = NLA_U32 }, [NFTA_SET_KEY_LEN] = { .type = NLA_U32 },
Currently, nf_tables trims off the set name if it exceeeds 15 bytes, so explicitly reject set names that are too large. Reported-by: Giuseppe Longo <giuseppelng@gmail.com> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org> --- net/netfilter/nf_tables_api.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)