[PATCH v2 0/2] xt_bpf: fix handling of pinned objects

[PATCH v2 0/2] xt_bpf: fix handling of pinned objects

[Shmulik Ladkani]

From: Rafael Buchbinder <rafi@rbk.ms>

From: Rafael Buchbinder <rafi@rbk.ms>

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(+)

-- 
2.14.1

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

[Shmulik Ladkani]

From: Rafael Buchbinder <rafi@rbk.ms>

From: Rafael Buchbinder <rafi@rbk.ms>

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

Signed-off-by: Rafael Buchbinder <rafi@rbk.ms>
Signed-off-by: Shmulik Ladkani <shmulik@nsof.io>
---
 include/xtables.h    |  3 +++
 iptables/ip6tables.c | 35 +++++++++++++++++++++++++++++++++++
 iptables/iptables.c  | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)

diff --git a/include/xtables.h b/include/xtables.h
index e9bc3b7d..687cfe9f 100644
--- a/include/xtables.h
+++ b/include/xtables.h
@@ -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);
 
diff --git a/iptables/ip6tables.c b/iptables/ip6tables.c
index 49bd006f..0a6afa77 100644
--- a/iptables/ip6tables.c
+++ b/iptables/ip6tables.c
@@ -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
diff --git a/iptables/iptables.c b/iptables/iptables.c
index 69d19fec..f220a8e4 100644
--- a/iptables/iptables.c
+++ b/iptables/iptables.c
@@ -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
-- 
2.14.1

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

[Pablo Neira Ayuso]

Hi Rafael,

On Sun, Sep 17, 2017 at 02:20:30PM +0300, Shmulik Ladkani wrote:
> From: Rafael Buchbinder <rafi@rbk.ms>
> 
> From: Rafael Buchbinder <rafi@rbk.ms>
> 
> This commit introduces a framework to fixup match info,
> which may be required by an extension.
> 
> Signed-off-by: Rafael Buchbinder <rafi@rbk.ms>
> Signed-off-by: Shmulik Ladkani <shmulik@nsof.io>
> ---
>  include/xtables.h    |  3 +++
>  iptables/ip6tables.c | 35 +++++++++++++++++++++++++++++++++++
>  iptables/iptables.c  | 34 ++++++++++++++++++++++++++++++++++
>  3 files changed, 72 insertions(+)
> 
> diff --git a/include/xtables.h b/include/xtables.h
> index e9bc3b7d..687cfe9f 100644
> --- a/include/xtables.h
> +++ b/include/xtables.h
> @@ -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?

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

[Shmulik Ladkani]

Hi Pablo,

On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> >  
> > +	/* 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

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

[Pablo Neira Ayuso]

On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote:
> Hi Pablo,
> 
> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> 
> > >  
> > > +	/* 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.

> 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.

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

[Willem de Bruijn]

On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote:
>> Hi Pablo,
>>
>> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:
>>
>> > >
>> > > + /* 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.
>
>> 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 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.

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

[Pablo Neira Ayuso]

On Mon, Sep 18, 2017 at 01:50:32PM -0400, Willem de Bruijn wrote:
> On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote:
> >> Hi Pablo,
> >>
> >> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> >>
> >> > >
> >> > > + /* 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.
> >
> >> 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 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.

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

[Pablo Neira Ayuso]

On Mon, Sep 18, 2017 at 07:54:24PM +0200, Pablo Neira Ayuso wrote:
> On Mon, Sep 18, 2017 at 01:50:32PM -0400, Willem de Bruijn wrote:
> > On Mon, Sep 18, 2017 at 1:23 PM, Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > > On Mon, Sep 18, 2017 at 08:00:42PM +0300, Shmulik Ladkani wrote:
> > >> Hi Pablo,
> > >>
> > >> On Mon, 18 Sep 2017 18:28:11 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:
> > >>
> > >> > >
> > >> > > + /* 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.
> > >
> > >> 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 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!

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

[Shmulik Ladkani]

Hi Pablo,

On Wed, 4 Oct 2017 16:33:01 +0200 Pablo Neira Ayuso <pablo@netfilter.org> wrote:

> > > 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

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

[Jan Engelhardt]

On Monday 2017-09-18 19:00, Shmulik Ladkani wrote:
>
>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?

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

[Shmulik Ladkani]

From: Rafael Buchbinder <rafi@rbk.ms>

From: Rafael Buchbinder <rafi@rbk.ms>

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 <rafi@rbk.ms>
Signed-off-by: Shmulik Ladkani <shmulik@nsof.io>
---
 extensions/libxt_bpf.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/extensions/libxt_bpf.c b/extensions/libxt_bpf.c
index 9510c190..16d6bc25 100644
--- a/extensions/libxt_bpf.c
+++ b/extensions/libxt_bpf.c
@@ -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,
-- 
2.14.1

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

[Willem de Bruijn]

On Sun, Sep 17, 2017 at 7:20 AM, Shmulik Ladkani <shmulik@nsof.io> wrote:
> From: Rafael Buchbinder <rafi@rbk.ms>
>
> From: Rafael Buchbinder <rafi@rbk.ms>
>
> 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 <rafi@rbk.ms>
> Signed-off-by: Shmulik Ladkani <shmulik@nsof.io>

Thanks a lot for fixing this.

Acked-by: Willem de Bruijn <willemb@google.com>

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.