xt_bpf: fix handling of pinned objects

PATCH In review Shmulik Ladkani 5 participants 12 messages Sep 17, 2017 – Oct 4, 2017 netfilter

lore

Share & tools

lore

Shmulik Ladkani

author

2017-09-17 11:20 UTC #0 lore raw

v2 cover xt_bpf: fix handling of pinned objects

From: Rafael Buchbinder <[email protected]>

Following set of commits fixes xt_bpf extension to correctly handle pinned eBPF programs.

The origin of the bug lies in the fact that xt_bpf_info_v1 structure requires an open file descriptor to create an eBPF match. This file descriptor is checked on every replace. However, as this file descriptor is valid only for the iptables invocation which loads the eBPF for the first time, all subsequent iptables invocations fail in bpf_mt_check (kernel) function.

See discussion in [1] for more details.

The following patches add a hook in extensions which is called immediately after TC_INIT to fixup whatever needs to be fixed up. In case of xt_bpf, the fixup function gets the eBPF object by path to populate xt_bpf_info_v1 structure with a valid file descriptor.

[1] https://marc.info/?l=netfilter-devel&m=150530909630143&w=2

Since v1:
- fixed From field

Rafael Buchbinder (2):
iptables: support match info fixup after tc_init
extensions: xt_bpf: get the pinned ebpf object when match is
initialized

 extensions/libxt_bpf.c |  9 +++++++++
 include/xtables.h      |  3 +++
 iptables/ip6tables.c   | 35 +++++++++++++++++++++++++++++++++++
 iptables/iptables.c    | 34 ++++++++++++++++++++++++++++++++++
 4 files changed, 81 insertions(+)

Signature · 1 line

2.14.1

Shmulik Ladkani

author

2017-09-17 11:20 UTC #1 lore raw

PATCH v2 1/2 iptables: support match info fixup after tc_init

From: Rafael Buchbinder <[email protected]>

This commit introduces a framework to fixup match info, which may be required by an extension.

Signed-off-by Rafael Buchbinder <[email protected]>

Signed-off-by Shmulik Ladkani <[email protected]>

---
 include/xtables.h    |  3 +++
 iptables/ip6tables.c | 35 +++++++++++++++++++++++++++++++++++
 iptables/iptables.c  | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

include/xtables.h +3 -0

index e9bc3b7d..687cfe9f 100644
@@ -273,6 +273,9 @@ struct xtables_match {
 	/* ip is struct ipt_ip * for example */
 	void (*save)(const void *ip, const struct xt_entry_match *match);
 
+	/* Fixes the match info after init. */
+	void (*tc_init_fixup)(struct xt_entry_match *match);
+
 	/* Print match name or alias */
 	const char *(*alias)(const struct xt_entry_match *match);

iptables/ip6tables.c +35 -0

index 49bd006f..0a6afa77 100644
@@ -925,6 +925,39 @@ delete_chain6(const xt_chainlabel chain, int verbose,
 	return ip6tc_delete_chain(chain, handle);
 }
 
+
+static int
+tc_init_fixup_match(struct xt_entry_match *m)
+{
+	const struct xtables_match *match =
+		xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL);
+
+	if (match) {
+		if (match->tc_init_fixup && m->u.user.revision == match->revision)
+			match->tc_init_fixup(m);
+	}
+
+	/* Don't stop iterating. */
+	return 0;
+}
+
+static void
+tc_init_fixup(struct xtc_handle *handle)
+{
+	const char *chain;
+
+	for (chain = ip6tc_first_chain(handle);
+	     chain;
+	     chain = ip6tc_next_chain(handle)) {
+		const struct ip6t_entry *entry = ip6tc_first_rule(chain, handle);
+
+		while (entry) {
+			IP6T_MATCH_ITERATE(entry, tc_init_fixup_match);
+			entry = ip6tc_next_rule(entry, handle);
+		}
+	}
+}
+
 static int
 list_entries(const xt_chainlabel chain, int rulenum, int verbose, int numeric,
 	     int expanded, int linenumbers, struct xtc_handle *handle)

@@ -1795,6 +1828,8 @@ int do_command6(int argc, char *argv[], char **table,
 			"can't initialize ip6tables table `%s': %s",
 			*table, ip6tc_strerror(errno));
 
+	tc_init_fixup(*handle);
+
 	if (command == CMD_APPEND
 	    || command == CMD_DELETE
 	    || command == CMD_CHECK

iptables/iptables.c +34 -0

index 69d19fec..f220a8e4 100644
@@ -909,6 +909,38 @@ delete_chain4(const xt_chainlabel chain, int verbose,
 	return iptc_delete_chain(chain, handle);
 }
 
+static int
+tc_init_fixup_match(struct xt_entry_match *m)
+{
+	const struct xtables_match *match =
+		xtables_find_match(m->u.user.name, XTF_TRY_LOAD, NULL);
+
+	if (match) {
+		if (match->tc_init_fixup && m->u.user.revision == match->revision)
+			match->tc_init_fixup(m);
+	}
+
+	/* Don't stop iterating. */
+	return 0;
+}
+
+static void
+tc_init_fixup(struct xtc_handle *handle)
+{
+	const char *chain;
+
+	for (chain = iptc_first_chain(handle);
+	     chain;
+	     chain = iptc_next_chain(handle)) {
+		const struct ipt_entry *entry = iptc_first_rule(chain, handle);
+
+		while (entry) {
+			IPT_MATCH_ITERATE(entry, tc_init_fixup_match);
+			entry = iptc_next_rule(entry, handle);
+		}
+	}
+}
+
 static int
 list_entries(const xt_chainlabel chain, int rulenum, int verbose, int numeric,
 	     int expanded, int linenumbers, struct xtc_handle *handle)

@@ -1781,6 +1813,8 @@ int do_command4(int argc, char *argv[], char **table,
 			   "can't initialize iptables table `%s': %s",
 			   *table, iptc_strerror(errno));
 
+	tc_init_fixup(*handle);
+
 	if (command == CMD_APPEND
 	    || command == CMD_DELETE
 	    || command == CMD_CHECK

Signature · 1 line

2.14.1

Pablo Neira Ayuso

maintainer

2017-09-18 16:28 UTC #2 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Hi Rafael,

Quoted 20 lines · Shmulik Ladkani

From: Rafael Buchbinder <[email protected]>
From: Rafael Buchbinder <[email protected]>
This commit introduces a framework to fixup match info, which may be required by an extension.
Signed-off-by Rafael Buchbinder <[email protected]>
Signed-off-by Shmulik Ladkani <[email protected]>
---
 include/xtables.h    |  3 +++
 iptables/ip6tables.c | 35 +++++++++++++++++++++++++++++++++++
 iptables/iptables.c  | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
include/xtables.h +0 -0
index e9bc3b7d..687cfe9f 100644

include/xtables.h struct xtables_match { · line 277

Quoted 5 lines

include/xtables.h +2 -0

@@ -273,6 +273,9 @@ struct xtables_match {
 	/* ip is struct ipt_ip * for example */
 	void (*save)(const void *ip, const struct xt_entry_match *match);
 
+	/* Fixes the match info after init. */
+	void (*tc_init_fixup)(struct xt_entry_match *match);

If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?

Shmulik Ladkani

author

2017-09-18 17:00 UTC #3 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Hi Pablo,

Quoted 6 lines · On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <[email protected]>…

include/xtables.h struct xtables_match { · line 277
Quoted 3 lines
include/xtables.h +2 -0
+	/* Fixes the match info after init. */
+	void (*tc_init_fixup)(struct xt_entry_match *match);  
If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?

No, this is not iproute2/tc specfic.

We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call.
If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'.

This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.

The problem lies in the xt_bpf_info_v1 ABI.
See:
https://marc.info/?l=netfilter-devel&m=150530909630143&w=2

Regards,
Shmulik

Pablo Neira Ayuso

maintainer

2017-09-18 17:23 UTC #4 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Quoted 5 lines · Shmulik Ladkani

Hi Pablo,
Quoted 1 line · On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <[email protected]>…
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
include/xtables.h struct xtables_match { · line 277
Quoted 2 lines
include/xtables.h +2 -0
+	/* Fixes the match info after init. */
+	void (*tc_init_fixup)(struct xt_entry_match *match);  
If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?
No, this is not iproute2/tc specfic.

OK.

Quoted 11 lines

We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call.
If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'.
This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.
The problem lies in the xt_bpf_info_v1 ABI.
See:
https://marc.info/?l=netfilter-devel&m=150530909630143&w=2

I see, can we get a v2 ABI that fixes this? Given this was included not long time ago, we can quickly deprecate this without this custom hook to address this.

We can include this in the next iptables release in the next weeks.

Willem de Bruijn

2017-09-18 17:51 UTC #5 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Quoted 24 lines · Pablo Neira Ayuso

Quoted 5 lines · Shmulik Ladkani
Hi Pablo,
Quoted 1 line · On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <[email protected]>…
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
+ /* Fixes the match info after init. */
+ void (*tc_init_fixup)(struct xt_entry_match *match);
If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?
No, this is not iproute2/tc specfic.
OK.
Quoted 8 lines
We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call.
If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'.
This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.

Quoted 7 lines

Quoted 3 lines
The problem lies in the xt_bpf_info_v1 ABI.
See:
https://marc.info/?l=netfilter-devel&m=150530909630143&w=2
I see, can we get a v2 ABI that fixes this? Given this was included not long time ago, we can quickly deprecate this without this custom hook to address this.

We can perhaps change the kernel module to ignore .fd and do a path lookup for .path directly inside the kernel. That would not require a v2, even.

Pablo Neira Ayuso

maintainer

2017-09-18 17:54 UTC #6 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Quoted 29 lines · Willem de Bruijn

Quoted 28 lines · Pablo Neira Ayuso
Quoted 5 lines · Shmulik Ladkani
Hi Pablo,
Quoted 1 line · On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <[email protected]>…
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
+ /* Fixes the match info after init. */
+ void (*tc_init_fixup)(struct xt_entry_match *match);
If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?
No, this is not iproute2/tc specfic.
OK.
Quoted 11 lines
We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call.
If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'.
This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.
The problem lies in the xt_bpf_info_v1 ABI.
See:
https://marc.info/?l=netfilter-devel&m=150530909630143&w=2

Quoted 7 lines

Quoted 3 lines
I see, can we get a v2 ABI that fixes this? Given this was included not long time ago, we can quickly deprecate this without this custom hook to address this.
We can perhaps change the kernel module to ignore .fd and do a path lookup for .path directly inside the kernel. That would not require a v2, even.

That sounds very reasonable, so we can just address this as a plain fix and pass it on to -stable.

Pablo Neira Ayuso

maintainer

2017-10-04 14:33 UTC #7 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Quoted 34 lines · Pablo Neira Ayuso

Quoted 33 lines · Willem de Bruijn
Quoted 31 lines · Pablo Neira Ayuso
Quoted 5 lines · Shmulik Ladkani
Hi Pablo,
Quoted 1 line · On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <[email protected]>…
Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
+ /* Fixes the match info after init. */
+ void (*tc_init_fixup)(struct xt_entry_match *match);
If this is only broken from tc ipt actions, could you fix this from iproute2/tc instead?
No, this is not iproute2/tc specfic.
OK.
Quoted 11 lines
We named it 'tc_init_fixup' as it occurs just after the TC_INIT (iptc_init/ip6tc_init) call.
If this is confusing, we can rename to 'init_fixup' or 'post_init_fixup' or 'iptc_init_fixup'.
This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.
The problem lies in the xt_bpf_info_v1 ABI.
See:
https://marc.info/?l=netfilter-devel&m=150530909630143&w=2
I see, can we get a v2 ABI that fixes this? Given this was included not long time ago, we can quickly deprecate this without this custom hook to address this.

Quoted 6 lines

Quoted 3 lines
We can perhaps change the kernel module to ignore .fd and do a path lookup for .path directly inside the kernel. That would not require a v2, even.
That sounds very reasonable, so we can just address this as a plain fix and pass it on to -stable.

Anyone following up with this?

Thanks!

Shmulik Ladkani

author

2017-10-04 14:38 UTC #8 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

Hi Pablo,

Quoted 8 lines · On Wed, 4 Oct 2017 16:33:01 +0200 Pablo Neira Ayuso <[email protected]> wrote:

Quoted 6 lines
Quoted 3 lines
We can perhaps change the kernel module to ignore .fd and do a path lookup for .path directly inside the kernel. That would not require a v2, even.
That sounds very reasonable, so we can just address this as a plain fix and pass it on to -stable.
Anyone following up with this?

I plan to work on a fix to the v1 abi, in which the given fd is ignored.

Best,
Shmulik

Jan Engelhardt

2017-09-18 18:04 UTC #9 lore raw

Re: [PATCH v2 1/2] iptables: support match info fixup after tc_init

In reply to Shmulik Ladkani

Quoted 3 lines · Shmulik Ladkani

This must occur after every load of entries, as the xt_bpf match needs a fixup once read from kernel.

So you could use the check function for it, do you not?

Shmulik Ladkani

author

2017-09-17 11:20 UTC #10 lore raw

PATCH v2 2/2 extensions: xt_bpf: get the pinned ebpf object when match is initialized

In reply to Shmulik Ladkani

From: Rafael Buchbinder <[email protected]>

xt_bpf_info_v1 structure requires an open file descriptor to create an eBPF match. This file descriptor is checked on every replace. However, as this file descriptor is valid only for the iptables invocation which loads the eBPF for the first time, all subsequent iptables invocations fail in bpf_mt_check (kernel) function.

This commit fixes handling of pinned ebpf objects.

The file descriptor saved in xt_bpf_info_v1 structure is being re-open in tc_init_fixup which is invoked immediately after tc_init.

Signed-off-by Rafael Buchbinder <[email protected]>

Signed-off-by Shmulik Ladkani <[email protected]>

---
 extensions/libxt_bpf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

extensions/libxt_bpf.c +9 -0

index 9510c190..16d6bc25 100644
@@ -247,6 +247,14 @@ static void bpf_print_v1(const void *ip, const struct xt_entry_match *match,
 		printf("unknown");
 }
 
+static void bpf_tc_init_fixup_v1(struct xt_entry_match *match)
+{
+	struct xt_bpf_info_v1 *info = (void *) match->data;
+
+	if (info->mode == XT_BPF_MODE_FD_PINNED)
+		bpf_parse_obj_pinned(info, info->path);
+}
+
 static struct xtables_match bpf_matches[] = {
 	{
 		.family		= NFPROTO_UNSPEC,

@@ -272,6 +280,7 @@ static struct xtables_match bpf_matches[] = {
 		.help		= bpf_help_v1,
 		.print		= bpf_print_v1,
 		.save		= bpf_save_v1,
+		.tc_init_fixup	= bpf_tc_init_fixup_v1,
 		.x6_parse	= bpf_parse_v1,
 		.x6_fcheck	= bpf_fcheck_v1,
 		.x6_options	= bpf_opts_v1,

Signature · 1 line

2.14.1

Willem de Bruijn

Acked-by on v2 2/2

2017-09-18 16:23 UTC #11 lore raw

Re: [PATCH v2 2/2] extensions: xt_bpf: get the pinned ebpf object when match is initialized

Quoted 10 lines · Shmulik Ladkani

From: Rafael Buchbinder <[email protected]>
From: Rafael Buchbinder <[email protected]>
xt_bpf_info_v1 structure requires an open file descriptor to create an eBPF match. This file descriptor is checked on every replace. However, as this file descriptor is valid only for the iptables invocation which loads the eBPF for the first time, all subsequent iptables invocations fail in bpf_mt_check (kernel) function.

Quoted 7 lines

This commit fixes handling of pinned ebpf objects.
The file descriptor saved in xt_bpf_info_v1 structure is being re-open in tc_init_fixup which is invoked immediately after tc_init.
Signed-off-by Rafael Buchbinder <[email protected]>
Signed-off-by Shmulik Ladkani <[email protected]>

Thanks a lot for fixing this.

Acked-by Willem de Bruijn <[email protected]>

The pinned object at that filepath can change between iptables invocations. This is not very obvious when inserting a new unrelated rule, but an unavoidable effect of iptables reading and re-inserting the entire table on each operation. Even switching to the bpf identifier would not help, as those ids can be recycled, too. Admins just have to be diligent and not rely on objects pinned by unprivileged users.