netfilter: xt_bpf: ABI issue in xt_bpf_info_v1?

netfilter: xt_bpf: ABI issue in xt_bpf_info_v1?

[Shmulik Ladkani]

Hi,

Commit 2c16d60 'netfilter: xt_bpf: support ebpf' introduced
'xt_bpf_info_v1', to support attaching an eBPF object by fd.

Alas, seems this ABI is problematic, as the 'fd', which is local to the
process attaching the ebpf object (namely iptables) is stored in the
matchinfo structure.

This leads to subsequent invocation of iptables to fail:

# iptables -A INPUT -m bpf --object-pinned /sys/fs/bpf/xxx -j ACCEPT
# iptables -A INPUT -s 5.6.7.8 -j ACCEPT
iptables: Invalid argument. Run `dmesg' for more information.

Tracing 2nd invocation shows:

setsockopt(4, SOL_IP, IPT_SO_SET_REPLACE, "filter\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0\0"..., 5400) = -1 EINVAL

In the 2nd invocation, iptables loads existing rules from the kernel
using IPT_SO_GET_ENTRIES, adds the new innocent one, and then issues
IPT_SO_SET_REPLACE.
However the existing rules contain the former ebpf rule which contains
the fd number from the initial 'iptables -m bpf' invocation.

Thus, when IPT_SO_SET_REPLACE eventually hits 'bpf_mt_check_v1', and
'bpf_prog_get_type' is called, the arbitrary fd has either no associated
file, or has no associated bpf_prog - leading 'bpf_prog_get_type' to
fail.

One way to fix is to have iptables open the object (using the stored
xt_bpf_info_v1->path), gaining a new process local fd for the object,
just after getting the rules from IPT_SO_GET_ENTRIES.
However we didn't see any other extensions doing something like that in
iptables.

Another way to solve is to fix the ABI (or have a v2 one), that does NOT
pass the fd from userspace, only the path of the pinned object.
Then, 'bpf_mt_check_v1' will open the file from the given path in order
to get the bpf_prog.

Please advise.

Best,
Shmulik

Re: netfilter: xt_bpf: ABI issue in xt_bpf_info_v1?

[Jan Engelhardt]

On Wednesday 2017-09-13 15:24, Shmulik Ladkani wrote:
>
>One way to fix is to have iptables open the object (using the stored
>xt_bpf_info_v1->path), gaining a new process local fd for the object,
>just after getting the rules from IPT_SO_GET_ENTRIES.
>However we didn't see any other extensions doing something like that in
>iptables.
>
>Another way to solve is to fix the ABI (or have a v2 one), that does NOT
>pass the fd from userspace, only the path of the pinned object.
>Then, 'bpf_mt_check_v1' will open the file from the given path in order
>to get the bpf_prog.

But a path has a similar problem like a file descriptor - it is local to a
certain mount namespace.

To load "large" blobs into the kernel, a pointer to user memory is a possible
option. The downside is that such extra data is not retrievable from the kernel
via the iptables setsockopts anymore - one could work around it with procfs, or
just let it be.

https://sourceforge.net/p/xtables-addons/xtables-addons/ci/master/tree/extensions/xt_geoip.c
line 64+.

Re: netfilter: xt_bpf: ABI issue in xt_bpf_info_v1?

[Willem de Bruijn]

On Wed, Sep 13, 2017 at 10:22 AM, Jan Engelhardt <jengelh@inai.de> wrote:
>
> On Wednesday 2017-09-13 15:24, Shmulik Ladkani wrote:
>>
>>One way to fix is to have iptables open the object (using the stored
>>xt_bpf_info_v1->path), gaining a new process local fd for the object,
>>just after getting the rules from IPT_SO_GET_ENTRIES.
>>However we didn't see any other extensions doing something like that in
>>iptables.

The binary should call bpf_obj_get on the filepath each time. These are
not regular files, but references to a pinned object in the bpf filesystem.

Blindly passing back the fd received from the kernel is clearly wrong. I'm
really surprised that I did not run into this problem when I wrote the
feature.

>>
>>Another way to solve is to fix the ABI (or have a v2 one), that does NOT
>>pass the fd from userspace, only the path of the pinned object.
>>Then, 'bpf_mt_check_v1' will open the file from the given path in order
>>to get the bpf_prog.
>
> But a path has a similar problem like a file descriptor - it is local to a
> certain mount namespace.

Because these are pinned objects in the bpf filesystem, and there is
only one of those, it may be possible to lookup the object in the kernel
without relying on a process-local view of mount points.

>
> To load "large" blobs into the kernel, a pointer to user memory is a possible
> option. The downside is that such extra data is not retrievable from the kernel
> via the iptables setsockopts anymore - one could work around it with procfs, or
> just let it be.
>
> https://sourceforge.net/p/xtables-addons/xtables-addons/ci/master/tree/extensions/xt_geoip.c
> line 64+.

Re: netfilter: xt_bpf: ABI issue in xt_bpf_info_v1?

[Willem de Bruijn]

On Wed, Sep 13, 2017 at 11:05 AM, Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
> On Wed, Sep 13, 2017 at 10:22 AM, Jan Engelhardt <jengelh@inai.de> wrote:
>>
>> On Wednesday 2017-09-13 15:24, Shmulik Ladkani wrote:
>>>
>>>One way to fix is to have iptables open the object (using the stored
>>>xt_bpf_info_v1->path), gaining a new process local fd for the object,
>>>just after getting the rules from IPT_SO_GET_ENTRIES.
>>>However we didn't see any other extensions doing something like that in
>>>iptables.
>
> The binary should call bpf_obj_get on the filepath each time. These are
> not regular files, but references to a pinned object in the bpf filesystem.
>
> Blindly passing back the fd received from the kernel is clearly wrong. I'm
> really surprised that I did not run into this problem when I wrote the
> feature.
>
>>>
>>>Another way to solve is to fix the ABI (or have a v2 one), that does NOT
>>>pass the fd from userspace, only the path of the pinned object.
>>>Then, 'bpf_mt_check_v1' will open the file from the given path in order
>>>to get the bpf_prog.
>>
>> But a path has a similar problem like a file descriptor - it is local to a
>> certain mount namespace.
>
> Because these are pinned objects in the bpf filesystem, and there is
> only one of those, it may be possible to lookup the object in the kernel
> without relying on a process-local view of mount points.

It would be preferable to fix this for the existing v1 users as well.

That said, the new bpf identifier feature allows passing a globally
unique id instead of a filepath.

>
>>
>> To load "large" blobs into the kernel, a pointer to user memory is a possible
>> option. The downside is that such extra data is not retrievable from the kernel
>> via the iptables setsockopts anymore - one could work around it with procfs, or
>> just let it be.
>>
>> https://sourceforge.net/p/xtables-addons/xtables-addons/ci/master/tree/extensions/xt_geoip.c
>> line 64+.