iptables: open eBPF programs in read only mode

From: Maciej Żenczykowski <maze@google.com>

Adjust the mode eBPF programs are opened in so 0400 pinned bpf programs
work without requiring CAP_DAC_OVERRIDE.

This matches Linux 5.2's:
  commit e547ff3f803e779a3898f1f48447b29f43c54085
  Author: Chenbo Feng <fengc@google.com>
  Date:   Tue May 14 19:42:57 2019 -0700

    bpf: relax inode permission check for retrieving bpf program

    For iptable module to load a bpf program from a pinned location, it
    only retrieve a loaded program and cannot change the program content so
    requiring a write permission for it might not be necessary.
    Also when adding or removing an unrelated iptable rule, it might need to
    flush and reload the xt_bpf related rules as well and triggers the inode
    permission check. It might be better to remove the write premission
    check for the inode so we won't need to grant write access to all the
    processes that flush and restore iptables rules.

  kernel/bpf/inode.c:
  - int ret = inode_permission(inode, MAY_READ | MAY_WRITE);
  + int ret = inode_permission(inode, MAY_READ);

In practice, AFAICT, the xt_bpf match .fd field isn't even used by new
kernels, but I believe it might be needed for compatibility with old ones
(though I'm pretty sure table modifications on them will outright fail).

Test: builds, passes Android test suite (albeit on an older iptables base),
  git grep bpf_obj_get - finds no other users
Cc: Chenbo Feng <fengc@google.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Willem de Bruijn <willemb@google.com>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 extensions/libxt_bpf.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

On Thu, Mar 19, 2020 at 08:00:15PM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Adjust the mode eBPF programs are opened in so 0400 pinned bpf programs
> work without requiring CAP_DAC_OVERRIDE.

Unfortunately this is breaking stuff:

libxt_bpf.c: In function ‘bpf_obj_get_readonly’:
libxt_bpf.c:70:6: error: ‘union bpf_attr’ has no member named ‘file_flags’
   70 |  attr.file_flags = BPF_F_RDONLY;
      |      ^
libxt_bpf.c:70:20: error: ‘BPF_F_RDONLY’ undeclared (first use in this function)
   70 |  attr.file_flags = BPF_F_RDONLY;
      |                    ^~~~~~~~~~~~

I don't get it.  It builds for me.

And it doesn't if I insert an intentional syntax error in the same line,
so I'm definitely compiling exactly that code.

I think your build system's kernel headers are old.

Linux 4.15-rc1 commit 6e71b04a82248ccf13a94b85cbc674a9fefe53f5
Author: Chenbo Feng <fengc@google.com>
Date:   Wed Oct 18 13:00:22 2017 -0700

    bpf: Add file mode configuration into bpf maps
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -218,6 +218,10 @@ enum bpf_attach_type {

 #define BPF_OBJ_NAME_LEN 16U

+/* Flags for accessing BPF object */
+#define BPF_F_RDONLY           (1U << 3)
+#define BPF_F_WRONLY           (1U << 4)
+
 union bpf_attr {
        struct { /* anonymous struct used by BPF_MAP_CREATE command */
                __u32   map_type;       /* one of enum bpf_map_type */
@@ -260,6 +264,7 @@ union bpf_attr {
        struct { /* anonymous struct used by BPF_OBJ_* commands */
                __aligned_u64   pathname;
                __u32           bpf_fd;
+               __u32           file_flags;
        };

        struct { /* anonymous struct used by BPF_PROG_ATTACH/DETACH commands */

On Thu, Mar 26, 2020 at 7:08 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> I don't get it.  It builds for me.
>
> And it doesn't if I insert an intentional syntax error in the same line,
> so I'm definitely compiling exactly that code.

I guess maybe we could wrap it in a

#ifdef BPF_F_RDONLY
attr.file_flags = BPF_F_RDONLY;
#endif

if we want to continue supporting building against pre-4.15 kernel headers...

Ugh, and I guess that on a pre-4.15 kernel it would cause failures due
to unknown flag...

Maybe we need to try with flag and fallback to without...

On Thu, Mar 26, 2020 at 07:16:16AM -0700, Maciej Żenczykowski wrote:
> I guess maybe we could wrap it in a
> 
> #ifdef BPF_F_RDONLY
> attr.file_flags = BPF_F_RDONLY;
> #endif
> 
> if we want to continue supporting building against pre-4.15 kernel headers...

You can probably add a cached copy of this header file to the iptables
tree via your patch like. This is done in other existing extensions to
not rely on the available kernel headers.

There is no parity between userspace iptables and kernel version, it
is good if you make sure this compiles for older kernels are still
supported.

Thank you.