diff mbox series

[v2] iptables: open eBPF programs in read only mode

Message ID 20200326142803.239183-1-zenczykowski@gmail.com
State Not Applicable
Delegated to: David Miller
Headers show
Series [v2] iptables: open eBPF programs in read only mode | expand

Commit Message

Maciej Żenczykowski March 26, 2020, 2:28 p.m. UTC 
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 | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Jakub Kicinski March 26, 2020, 6:30 p.m. UTC | #1 
On Thu, 26 Mar 2020 07:28:03 -0700 Maciej Żenczykowski wrote:
> diff --git a/extensions/libxt_bpf.c b/extensions/libxt_bpf.c
> index 92958247..44cdd5cb 100644
> --- a/extensions/libxt_bpf.c
> +++ b/extensions/libxt_bpf.c
> @@ -61,11 +61,22 @@ static const struct xt_option_entry bpf_opts_v1[] = {
>  	XTOPT_TABLEEND,
>  };
>  
> -static int bpf_obj_get(const char *filepath)
> +static int bpf_obj_get_readonly(const char *filepath)
>  {
>  #if defined HAVE_LINUX_BPF_H && defined __NR_bpf && defined BPF_FS_MAGIC
>  	union bpf_attr attr;
> +	// file_flags && BPF_F_RDONLY requires Linux 4.15+ uapi kernel headers
> +#ifdef BPF_F_RDONLY

FWIW the BPF subsystem is about to break uAPI backward-compat and
replace the defines with enums. See commit 1aae4bdd7879 ("bpf: Switch
BPF UAPI #define constants used from BPF program side to enums").

> +	int fd;
>  
> +	memset(&attr, 0, sizeof(attr));
> +	attr.pathname = (__u64) filepath;
> +	attr.file_flags = BPF_F_RDONLY;
> +	fd = syscall(__NR_bpf, BPF_OBJ_GET, &attr, sizeof(attr));
> +	if (fd >= 0) return fd;
> +
> +	// on any error fallback to default R/W access for pre-4.15-rc1 kernels
> +#endif
Maciej Żenczykowski March 26, 2020, 6:34 p.m. UTC | #2 
> FWIW the BPF subsystem is about to break uAPI backward-compat and
> replace the defines with enums. See commit 1aae4bdd7879 ("bpf: Switch
> BPF UAPI #define constants used from BPF program side to enums").

Shouldn't it do what is normally done in such a case?
#define BPF_F_RDONLY BPF_F_RDONLY
Alexei Starovoitov March 26, 2020, 7:22 p.m. UTC | #3 
On Thu, Mar 26, 2020 at 11:34 AM Maciej Żenczykowski
<zenczykowski@gmail.com> wrote:
>
> > FWIW the BPF subsystem is about to break uAPI backward-compat and
> > replace the defines with enums. See commit 1aae4bdd7879 ("bpf: Switch
> > BPF UAPI #define constants used from BPF program side to enums").
>
> Shouldn't it do what is normally done in such a case?
> #define BPF_F_RDONLY BPF_F_RDONLY

No. just update the headers.
diff mbox series

Patch

diff --git a/extensions/libxt_bpf.c b/extensions/libxt_bpf.c
index 92958247..44cdd5cb 100644
--- a/extensions/libxt_bpf.c
+++ b/extensions/libxt_bpf.c
@@ -61,11 +61,22 @@  static const struct xt_option_entry bpf_opts_v1[] = {
 	XTOPT_TABLEEND,
 };
 
-static int bpf_obj_get(const char *filepath)
+static int bpf_obj_get_readonly(const char *filepath)
 {
 #if defined HAVE_LINUX_BPF_H && defined __NR_bpf && defined BPF_FS_MAGIC
 	union bpf_attr attr;
+	// file_flags && BPF_F_RDONLY requires Linux 4.15+ uapi kernel headers
+#ifdef BPF_F_RDONLY
+	int fd;
 
+	memset(&attr, 0, sizeof(attr));
+	attr.pathname = (__u64) filepath;
+	attr.file_flags = BPF_F_RDONLY;
+	fd = syscall(__NR_bpf, BPF_OBJ_GET, &attr, sizeof(attr));
+	if (fd >= 0) return fd;
+
+	// on any error fallback to default R/W access for pre-4.15-rc1 kernels
+#endif
 	memset(&attr, 0, sizeof(attr));
 	attr.pathname = (__u64) filepath;
 
@@ -125,7 +136,7 @@  static void bpf_parse_string(struct sock_filter *pc, __u16 *lenp, __u16 len_max,
 static void bpf_parse_obj_pinned(struct xt_bpf_info_v1 *bi,
 				 const char *filepath)
 {
-	bi->fd = bpf_obj_get(filepath);
+	bi->fd = bpf_obj_get_readonly(filepath);
 	if (bi->fd < 0)
 		xtables_error(PARAMETER_PROBLEM,
 			      "bpf: failed to get bpf object");