| Message ID | 20141020115049.27980.35459.stgit@nfdev.cica.es |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
On Mon, Oct 20, 2014 at 01:52:18PM +0200, Arturo Borrero Gonzalez wrote: > NFPROTO_ARP = 3 in kernel space. > > We need the same value here in userspace in order to correctly communicate > with the kernel. > > The failure solved by this patch made that {XML|JSON}-parsed tables of ARP > family unable to be directly injected into kernel. > > To prevent future errors, this patch changes raw and AF_* values by the mathing > NFPROTO_* couterpart as seen in linux/netfilter.h in both functions: > * nft_family2str() > * nft_str2family() Applied, thanks. BTW, it would be good to see a similar refactor in nft_verdict2str(). -- 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 21 October 2014 09:59, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > BTW, it would be good to see a similar refactor in nft_verdict2str(). I don't see a clean way to do it, given some verdicts are negative numbers (enum nft_verdicts in nf_tables.h). We may end accessing a negative index, out of bounds of the array.
On Tue, Oct 21, 2014 at 10:53:19AM +0200, Arturo Borrero Gonzalez wrote: > On 21 October 2014 09:59, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > BTW, it would be good to see a similar refactor in nft_verdict2str(). > > I don't see a clean way to do it, given some verdicts are negative > numbers (enum nft_verdicts in nf_tables.h). > We may end accessing a negative index, out of bounds of the array. I see, you mean: enum nft_verdicts { NFT_CONTINUE = -1, NFT_BREAK = -2, NFT_JUMP = -3, NFT_GOTO = -4, NFT_RETURN = -5, }; You can add some function to shift the values: #define nft_verdict_index(base) (base + 5) ... nft_verdict_array[] = { [nft_verdict_index(NFT_RETURN)] = "return", ... }; -- 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, Oct 21, 2014 at 11:56:49AM +0200, Pablo Neira Ayuso wrote: > On Tue, Oct 21, 2014 at 10:53:19AM +0200, Arturo Borrero Gonzalez wrote: > > On 21 October 2014 09:59, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > > > > > BTW, it would be good to see a similar refactor in nft_verdict2str(). > > > > I don't see a clean way to do it, given some verdicts are negative > > numbers (enum nft_verdicts in nf_tables.h). > > We may end accessing a negative index, out of bounds of the array. > > I see, you mean: > > enum nft_verdicts { > NFT_CONTINUE = -1, > NFT_BREAK = -2, > NFT_JUMP = -3, > NFT_GOTO = -4, > NFT_RETURN = -5, > }; > > You can add some function to shift the values: > > #define nft_verdict_index(base) (base + 5) BTW, instead of 5, add: #define NFT_VERDICT_BASE NFT_RETURN and use it. > > ... nft_verdict_array[] = { > [nft_verdict_index(NFT_RETURN)] = "return", > ... > }; -- 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 21 October 2014 11:57, Pablo Neira Ayuso <pablo@netfilter.org> wrote: > On Tue, Oct 21, 2014 at 11:56:49AM +0200, Pablo Neira Ayuso wrote: >> On Tue, Oct 21, 2014 at 10:53:19AM +0200, Arturo Borrero Gonzalez wrote: >> > On 21 October 2014 09:59, Pablo Neira Ayuso <pablo@netfilter.org> wrote: >> > > >> > > BTW, it would be good to see a similar refactor in nft_verdict2str(). >> > >> > I don't see a clean way to do it, given some verdicts are negative >> > numbers (enum nft_verdicts in nf_tables.h). >> > We may end accessing a negative index, out of bounds of the array. >> >> I see, you mean: >> >> enum nft_verdicts { >> NFT_CONTINUE = -1, >> NFT_BREAK = -2, >> NFT_JUMP = -3, >> NFT_GOTO = -4, >> NFT_RETURN = -5, >> }; >> >> You can add some function to shift the values: >> >> #define nft_verdict_index(base) (base + 5) > > BTW, instead of 5, add: > > #define NFT_VERDICT_BASE NFT_RETURN > > and use it. > >> >> ... nft_verdict_array[] = { >> [nft_verdict_index(NFT_RETURN)] = "return", >> ... >> }; Ok, following your idea, I end with something like this: /* * NF_DROP = 0 * NF_ACCEPT = 1 * NFT_JUMP = -3 * NFT_GOTO = -4 * NFT_RETURN = -5 */ #define NFT_VERDICT_BASE -NFT_RETURN #define nft_verdict_index(base) (base + NFT_VERDICT_BASE) #define NFT_VERDICT_ARRAY_LEN nft_verdict_index(NF_ACCEPT) static const char *const nft_verdict_str[NFT_VERDICT_ARRAY_LEN + 1] = { [nft_verdict_index(NFT_RETURN)] = "return", /* 0 */ [nft_verdict_index(NFT_GOTO)] = "goto", /* 1 */ [nft_verdict_index(NFT_JUMP)] = "jump", /* 2 */ [nft_verdict_index(NF_DROP)] = "drop", /* 5 */ [nft_verdict_index(NF_ACCEPT)] = "accept", /* 6 */ }; const char *nft_verdict2str(int verdict) { if (nft_verdict_str[nft_verdict_index(verdict)] == NULL) return "unknown"; return nft_verdict_str[nft_verdict_index(verdict)]; } int nft_str2verdict(const char *verdict, int *verdict_num) { int i; for (i = 0; i < NFT_VERDICT_ARRAY_LEN; i++) { if (nft_verdict_str[i] == NULL) continue; if (strcmp(nft_verdict_str[i], verdict) == 0) { *verdict_num = nft_verdict_index(i); return 0; } } return -1; } I think the current code (the switch based one) is better, don't you? That array seems very error prone.
diff --git a/src/utils.c b/src/utils.c index d70fbf1..9013b68 100644 --- a/src/utils.c +++ b/src/utils.c @@ -20,36 +20,33 @@ #include <linux/netfilter.h> #include <linux/netfilter/nf_tables.h> +static const char *const nft_family_str[NFPROTO_NUMPROTO] = { + [NFPROTO_INET] = "inet", + [NFPROTO_IPV4] = "ip", + [NFPROTO_ARP] = "arp", + [NFPROTO_BRIDGE] = "bridge", + [NFPROTO_IPV6] = "ip6", +}; + const char *nft_family2str(uint32_t family) { - switch (family) { - case AF_INET: - return "ip"; - case AF_INET6: - return "ip6"; - case 1: - return "inet"; - case AF_BRIDGE: - return "bridge"; - case 3: /* NFPROTO_ARP */ - return "arp"; - default: + if (nft_family_str[family] == NULL) return "unknown"; - } + + return nft_family_str[family]; } int nft_str2family(const char *family) { - if (strcmp(family, "ip") == 0) - return AF_INET; - else if (strcmp(family, "ip6") == 0) - return AF_INET6; - else if (strcmp(family, "inet") == 0) - return 1; - else if (strcmp(family, "bridge") == 0) - return AF_BRIDGE; - else if (strcmp(family, "arp") == 0) - return 0; + int i; + + for (i = 0; i < NFPROTO_NUMPROTO; i++) { + if (nft_family_str[i] == NULL) + continue; + + if (strcmp(nft_family_str[i], family) == 0) + return i; + } errno = EAFNOSUPPORT; return -1;
NFPROTO_ARP = 3 in kernel space. We need the same value here in userspace in order to correctly communicate with the kernel. The failure solved by this patch made that {XML|JSON}-parsed tables of ARP family unable to be directly injected into kernel. To prevent future errors, this patch changes raw and AF_* values by the mathing NFPROTO_* couterpart as seen in linux/netfilter.h in both functions: * nft_family2str() * nft_str2family() Signed-off-by: Arturo Borrero Gonzalez <arturo.borrero.glez@gmail.com> --- v2: rework+fix using the array-matching approach suggested by Pablo. v3: constify both the pointer and the data, suggested by Jan. Keep setting errno to EAFNOSUPPORT in nft_str2family(). src/utils.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-) -- 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