[v3] BPF: Regression test for 64bit arithmetic
diff mbox series

Message ID 20190911095422.11767-1-rpalethorpe@suse.com
State New
Headers show
Series
  • [v3] BPF: Regression test for 64bit arithmetic
Related show

Commit Message

Richard Palethorpe Sept. 11, 2019, 9:54 a.m. UTC
Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
Reviewed-by: Jan Stancek <jstancek@redhat.com>
Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
---

V3: Rebased on master and included line numbers in instructions

Capability patch has not been applied to master at time of rebase.

 include/lapi/bpf.h                         |  27 +++
 runtest/syscalls                           |   1 +
 testcases/kernel/syscalls/bpf/.gitignore   |   1 +
 testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
 4 files changed, 211 insertions(+)
 create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c

Comments

Cyril Hrubis Sept. 12, 2019, 2:56 p.m. UTC | #1
Hi!
A few minor points below, I guess I can fix these before pushing as
well.

> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reviewed-by: Jan Stancek <jstancek@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
> 
> V3: Rebased on master and included line numbers in instructions
> 
> Capability patch has not been applied to master at time of rebase.
> 
>  include/lapi/bpf.h                         |  27 +++
>  runtest/syscalls                           |   1 +
>  testcases/kernel/syscalls/bpf/.gitignore   |   1 +
>  testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
>  4 files changed, 211 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
> 
> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
> index 122eb5469..03073b45e 100644
> --- a/include/lapi/bpf.h
> +++ b/include/lapi/bpf.h
> @@ -18,7 +18,9 @@
>  /* Start copy from linux/bpf_(common).h */
>  #define BPF_CLASS(code) ((code) & 0x07)
>  #define		BPF_LD		0x00
> +#define		BPF_LDX		0x01
>  #define		BPF_ST		0x02
> +#define		BPF_STX		0x03
>  #define		BPF_JMP		0x05
>  
>  #define BPF_SIZE(code)  ((code) & 0x18)
> @@ -30,6 +32,7 @@
>  
>  #define BPF_OP(code)    ((code) & 0xf0)
>  #define		BPF_ADD		0x00
> +#define		BPF_SUB		0x10
>  
>  #define		BPF_JEQ		0x10
>  
> @@ -432,6 +435,14 @@ enum bpf_func_id {
>  
>  /* Start copy from tools/include/filter.h */
>  
> +#define BPF_ALU64_REG(OP, DST, SRC)				\
> +	((struct bpf_insn) {					\
> +		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
> +		.dst_reg = DST,					\
> +		.src_reg = SRC,					\
> +		.off   = 0,					\
> +		.imm   = 0 })
> +
>  #define BPF_ALU64_IMM(OP, DST, IMM)				\
>  	((struct bpf_insn) {					\
>  		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_K,	\
> @@ -477,6 +488,22 @@ enum bpf_func_id {
>  		.off   = OFF,					\
>  		.imm   = IMM })
>  
> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF)			\
> +	((struct bpf_insn) {					\
> +		.code  = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM,	\
> +		.dst_reg = DST,					\
> +		.src_reg = SRC,					\
> +		.off   = OFF,					\
> +		.imm   = 0 })
> +
> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF)			\
> +	((struct bpf_insn) {					\
> +		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM,	\
> +		.dst_reg = DST,					\
> +		.src_reg = SRC,					\
> +		.off   = OFF,					\
> +		.imm   = 0 })
> +
>  #define BPF_JMP_IMM(OP, DST, IMM, OFF)				\
>  	((struct bpf_insn) {					\
>  		.code  = BPF_JMP | BPF_OP(OP) | BPF_K,		\
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 874ae4d4f..4e6310193 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -34,6 +34,7 @@ bind03 bind03
>  
>  bpf_map01 bpf_map01
>  bpf_prog01 bpf_prog01
> +bpf_prog02 bpf_prog02
>  
>  brk01 brk01
>  
> diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
> index 7eb5f7c92..1704f9841 100644
> --- a/testcases/kernel/syscalls/bpf/.gitignore
> +++ b/testcases/kernel/syscalls/bpf/.gitignore
> @@ -1,2 +1,3 @@
>  bpf_map01
>  bpf_prog01
> +bpf_prog02
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> new file mode 100644
> index 000000000..dc8b92f00
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
> + *
> + * Check if eBPF can do arithmetic with 64bits. This targets a specific
> + * regression which only effects unprivileged users who are subject to extra
> + * pointer arithmetic checks during verification.
> + *
> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
> + * https://new.blog.cloudflare.com/ebpf-cant-count/
> + *
> + * This test is very similar in structure to bpf_prog01 which is better
> + * annotated.
> + */
> +
> +#include <limits.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "tst_capability.h"
> +#include "lapi/socket.h"
> +#include "lapi/bpf.h"
> +
> +#define A64INT (((uint64_t)1) << 60)
> +
> +const char MSG[] = "Ahoj!";
> +static char *msg;
> +
> +static char *log;
> +static uint32_t *key;
> +static uint64_t *val;
> +static union bpf_attr *attr;
> +
> +static int load_prog(int fd)
> +{
> +	struct bpf_insn *prog;
> +	struct bpf_insn insn[] = {
> +		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1 */
> +
> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 1: r1 = &fd */
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 3: r2 = fp */
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 4: r2 = r2 - 8 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* 5: *r2 = 0 */
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 8: r3 = r0 */
> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 9: r4 = 2^61 */
> +		BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
> +
> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 13: r1 = &fd */
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 15: r2 = fp */
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 16: r2 = r2 - 8 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1),    /* 17: *r2 = 1 */
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),  /* 19: if(!r0) goto 25 */
> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 20: r3 = r0 */
> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 = 2^61 */
> +		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
> +
> +		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 = 0 */
> +		BPF_EXIT_INSN(),		        /* 26: return r0 */
> +	};
> +
> +	/* Leaks memory when -i is specified */
> +	prog = tst_alloc(sizeof(insn));

Why not just declare the prog static? Then we could do:

	if (!prog)
		prog = tst_alloc(sizeof(insn));

> +	memcpy(prog, insn, sizeof(insn));
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +	attr->insns = ptr_to_u64(prog);
> +	attr->insn_cnt = ARRAY_SIZE(insn);
> +	attr->license = ptr_to_u64("GPL");
> +	attr->log_buf = ptr_to_u64(log);
> +	attr->log_size = BUFSIZ;
> +	attr->log_level = 1;
> +
> +	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		if (log[0] != 0) {
> +			tst_res(TINFO, "Verification log:");
> +			fputs(log, stderr);

I guess that we can do tst_res(TINFO, "Verification log:\n%s", log); instead.

> +			tst_brk(TBROK | TTERRNO, "Failed verification");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to load program");
> +		}
> +	}
> +
> +	return TST_RET;
> +}
> +
> +static void setup(void)
> +{
> +	memcpy(msg, MSG, sizeof(MSG));
> +}
> +
> +static void run(void)
> +{
> +	int map_fd, prog_fd;
> +	int sk[2];
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->map_type = BPF_MAP_TYPE_ARRAY;
> +	attr->key_size = 4;
> +	attr->value_size = 8;
> +	attr->max_entries = 2;
> +
> +	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		if (TST_ERR == EPERM) {
> +			tst_brk(TCONF | TTERRNO,
> +				"bpf() requires CAP_SYS_ADMIN on this system");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to create array map");
> +		}
> +	}
> +	map_fd = TST_RET;
> +
> +	prog_fd = load_prog(map_fd);
> +
> +	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> +	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> +			&prog_fd, sizeof(prog_fd));
> +
> +	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->map_fd = map_fd;
> +	attr->key = ptr_to_u64(key);
> +	attr->value = ptr_to_u64(val);
> +	*key = 0;
> +
> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "array map lookup");
> +	} else if (*val != A64INT + 1) {
> +		tst_res(TFAIL,
> +			"val = %lu, but should be val = %lu + 1",
> +			*val, A64INT);
> +        } else {
> +	        tst_res(TPASS, "val = %lu + 1", A64INT);
> +	}

Wrong indentation, also I do not fancy this if else maze. I guess that
we can can safely do goto exit; if map lookup fails that would point
right before the SAFE_CLOSE() block.

> +	*key = 1;
> +
> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "array map lookup");
> +	} else if (*val != A64INT - 1) {
> +		tst_res(TFAIL,
> +			"val = %lu, but should be val = %lu - 1",
> +			*val, A64INT);
> +        } else {
> +	        tst_res(TPASS, "val = %lu - 1", A64INT);
> +	}


Here as well.

> +	SAFE_CLOSE(prog_fd);
> +	SAFE_CLOSE(map_fd);
> +	SAFE_CLOSE(sk[0]);
> +	SAFE_CLOSE(sk[1]);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = run,
> +	.min_kver = "3.18",
> +	.caps = (struct tst_cap []) {
> +		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
> +		{}
> +	},
> +	.bufs = (struct tst_buffers []) {
> +		{&key, .size = sizeof(*key)},
> +		{&val, .size = sizeof(*val)},
> +		{&log, .size = BUFSIZ},
> +		{&attr, .size = sizeof(*attr)},
> +		{&msg, .size = sizeof(MSG)},
> +		{},
> +	}
> +};
> -- 
> 2.22.1
>
Clemens Famulla-Conrad Sept. 13, 2019, 10:55 a.m. UTC | #2
Hi,
agree with comments from Cyril. And add two tiny one.

Reviewed-by: Clemens Famulla-Conrad<cfamullaconrad@suse.de>

On Wed, 2019-09-11 at 11:54 +0200, Richard Palethorpe wrote:
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> Reviewed-by: Jan Stancek <jstancek@redhat.com>
> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
> ---
> 
> V3: Rebased on master and included line numbers in instructions
> 
> Capability patch has not been applied to master at time of rebase.
> 
>  include/lapi/bpf.h                         |  27 +++
>  runtest/syscalls                           |   1 +
>  testcases/kernel/syscalls/bpf/.gitignore   |   1 +
>  testcases/kernel/syscalls/bpf/bpf_prog02.c | 182
> +++++++++++++++++++++
>  4 files changed, 211 insertions(+)
>  create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
> 
> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
> index 122eb5469..03073b45e 100644
> --- a/include/lapi/bpf.h
> +++ b/include/lapi/bpf.h
> @@ -18,7 +18,9 @@
>  /* Start copy from linux/bpf_(common).h */
>  #define BPF_CLASS(code) ((code) & 0x07)
>  #define		BPF_LD		0x00
> +#define		BPF_LDX		0x01
>  #define		BPF_ST		0x02
> +#define		BPF_STX		0x03
>  #define		BPF_JMP		0x05
>  
>  #define BPF_SIZE(code)  ((code) & 0x18)
> @@ -30,6 +32,7 @@
>  
>  #define BPF_OP(code)    ((code) & 0xf0)
>  #define		BPF_ADD		0x00
> +#define		BPF_SUB		0x10
>  
>  #define		BPF_JEQ		0x10
>  
> @@ -432,6 +435,14 @@ enum bpf_func_id {
>  
>  /* Start copy from tools/include/filter.h */
>  
> +#define BPF_ALU64_REG(OP, DST, SRC)				\
> +	((struct bpf_insn) {					
> \
> +		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
> +		.dst_reg = DST,					
> \
> +		.src_reg = SRC,					
> \
> +		.off   = 0,					\
> +		.imm   = 0 })
> +
>  #define BPF_ALU64_IMM(OP, DST, IMM)				\
>  	((struct bpf_insn) {					
> \
>  		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_K,	\
> @@ -477,6 +488,22 @@ enum bpf_func_id {
>  		.off   = OFF,					
> \
>  		.imm   = IMM })
>  
> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF)			\
> +	((struct bpf_insn) {					
> \
> +		.code  = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM,	
> \
> +		.dst_reg = DST,					
> \
> +		.src_reg = SRC,					
> \
> +		.off   = OFF,					
> \
> +		.imm   = 0 })
> +
> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF)			\
> +	((struct bpf_insn) {					
> \
> +		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM,	
> \
> +		.dst_reg = DST,					
> \
> +		.src_reg = SRC,					
> \
> +		.off   = OFF,					
> \
> +		.imm   = 0 })
> +
>  #define BPF_JMP_IMM(OP, DST, IMM, OFF)				
> \
>  	((struct bpf_insn) {					
> \
>  		.code  = BPF_JMP | BPF_OP(OP) | BPF_K,		
> \
> diff --git a/runtest/syscalls b/runtest/syscalls
> index 874ae4d4f..4e6310193 100644
> --- a/runtest/syscalls
> +++ b/runtest/syscalls
> @@ -34,6 +34,7 @@ bind03 bind03
>  
>  bpf_map01 bpf_map01
>  bpf_prog01 bpf_prog01
> +bpf_prog02 bpf_prog02
>  
>  brk01 brk01
>  
> diff --git a/testcases/kernel/syscalls/bpf/.gitignore
> b/testcases/kernel/syscalls/bpf/.gitignore
> index 7eb5f7c92..1704f9841 100644
> --- a/testcases/kernel/syscalls/bpf/.gitignore
> +++ b/testcases/kernel/syscalls/bpf/.gitignore
> @@ -1,2 +1,3 @@
>  bpf_map01
>  bpf_prog01
> +bpf_prog02
> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c
> b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> new file mode 100644
> index 000000000..dc8b92f00
> --- /dev/null
> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
> + *
> + * Check if eBPF can do arithmetic with 64bits. This targets a
> specific
> + * regression which only effects unprivileged users who are subject
> to extra
> + * pointer arithmetic checks during verification.
> + *
> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
> + * https://new.blog.cloudflare.com/ebpf-cant-count/
> + *
> + * This test is very similar in structure to bpf_prog01 which is
> better
> + * annotated.
> + */
> +
> +#include <limits.h>
> +#include <string.h>
> +#include <stdio.h>
> +
> +#include "config.h"
> +#include "tst_test.h"
> +#include "tst_capability.h"
> +#include "lapi/socket.h"
> +#include "lapi/bpf.h"
> +
> +#define A64INT (((uint64_t)1) << 60)
> +
> +const char MSG[] = "Ahoj!";
> +static char *msg;
> +
> +static char *log;
> +static uint32_t *key;
> +static uint64_t *val;
> +static union bpf_attr *attr;
> +
> +static int load_prog(int fd)
> +{
> +	struct bpf_insn *prog;
> +	struct bpf_insn insn[] = {
> +		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1
> */
> +
> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 1:
> r1 = &fd */
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 3: r2 =
> fp */
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 4: r2 =
> r2 - 8 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* 5: *r2 =
> 0 */
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6:
> map_lookup_elem */
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7:
> if(!r0) goto 25 */
> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 8: r3 =
> r0 */
> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 9: r4 =
> 2^61 */
> +		BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11:
> r4 += r6 */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12:
> *r3 = r4 */
> +
> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 13:
> r1 = &fd */
> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 15: r2 =
> fp */
> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 16: r2 =
> r2 - 8 */
> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1),    /* 17: *r2 =
> 1 */
> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18:
> map_lookup_elem */
> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),  /* 19:
> if(!r0) goto 25 */
> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 20: r3 =
> r0 */
> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 =
> 2^61 */
  ^
  I think 2^61 is different to 1<<60

> +		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
> r4 -= r6 */
> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
> *r3 = r4 */
> +
> +		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 =
> 0 */
> +		BPF_EXIT_INSN(),		        /* 26:
> return r0 */
> +	};
> +
> +	/* Leaks memory when -i is specified */
> +	prog = tst_alloc(sizeof(insn));
> +	memcpy(prog, insn, sizeof(insn));
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> +	attr->insns = ptr_to_u64(prog);
> +	attr->insn_cnt = ARRAY_SIZE(insn);
> +	attr->license = ptr_to_u64("GPL");
> +	attr->log_buf = ptr_to_u64(log);
> +	attr->log_size = BUFSIZ;
> +	attr->log_level = 1;
> +
> +	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		if (log[0] != 0) {
> +			tst_res(TINFO, "Verification log:");
> +			fputs(log, stderr);
> +			tst_brk(TBROK | TTERRNO, "Failed
> verification");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to load
> program");
> +		}
> +	}
> +
> +	return TST_RET;
> +}
> +
> +static void setup(void)
> +{
> +	memcpy(msg, MSG, sizeof(MSG));
> +}
> +
> +static void run(void)
> +{
> +	int map_fd, prog_fd;
> +	int sk[2];
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->map_type = BPF_MAP_TYPE_ARRAY;
> +	attr->key_size = 4;
> +	attr->value_size = 8;
> +	attr->max_entries = 2;
> +
> +	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		if (TST_ERR == EPERM) {
> +			tst_brk(TCONF | TTERRNO,
> +				"bpf() requires CAP_SYS_ADMIN on
> this system");
> +		} else {
> +			tst_brk(TBROK | TTERRNO, "Failed to create
> array map");
> +		}
> +	}
> +	map_fd = TST_RET;
> +
> +	prog_fd = load_prog(map_fd);
> +
> +	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> +	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> +			&prog_fd, sizeof(prog_fd));
> +
> +	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> +
> +	memset(attr, 0, sizeof(*attr));
> +	attr->map_fd = map_fd;
> +	attr->key = ptr_to_u64(key);
> +	attr->value = ptr_to_u64(val);
> +	*key = 0;
> +
> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "array map lookup");
> +	} else if (*val != A64INT + 1) {
> +		tst_res(TFAIL,
> +			"val = %lu, but should be val = %lu + 1",
                                ^
Not sure if it is really needed but I would use %llu here. If so, other
places as well.

> +			*val, A64INT);
> +        } else {
> +	        tst_res(TPASS, "val = %lu + 1", A64INT);
> +	}
> +
> +	*key = 1;
> +
> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> +	if (TST_RET == -1) {
> +		tst_res(TFAIL | TTERRNO, "array map lookup");
> +	} else if (*val != A64INT - 1) {
> +		tst_res(TFAIL,
> +			"val = %lu, but should be val = %lu - 1",
> +			*val, A64INT);
> +        } else {
> +	        tst_res(TPASS, "val = %lu - 1", A64INT);
> +	}
> +
> +	SAFE_CLOSE(prog_fd);
> +	SAFE_CLOSE(map_fd);
> +	SAFE_CLOSE(sk[0]);
> +	SAFE_CLOSE(sk[1]);
> +}
> +
> +static struct tst_test test = {
> +	.setup = setup,
> +	.test_all = run,
> +	.min_kver = "3.18",
> +	.caps = (struct tst_cap []) {
> +		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
> +		{}
> +	},
> +	.bufs = (struct tst_buffers []) {
> +		{&key, .size = sizeof(*key)},
> +		{&val, .size = sizeof(*val)},
> +		{&log, .size = BUFSIZ},
> +		{&attr, .size = sizeof(*attr)},
> +		{&msg, .size = sizeof(MSG)},
> +		{},
> +	}
> +};
> -- 
> 2.22.1
> 
>
Cyril Hrubis Sept. 13, 2019, 11:46 a.m. UTC | #3
Hi!
> > +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 =
> > 2^61 */
>   ^
>   I think 2^61 is different to 1<<60

Agreed, 1<<60 is 2^60

> > +		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
> > r4 -= r6 */
> > +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
> > *r3 = r4 */
> > +
> > +		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 =
> > 0 */
> > +		BPF_EXIT_INSN(),		        /* 26:
> > return r0 */
> > +	};
> > +
> > +	/* Leaks memory when -i is specified */
> > +	prog = tst_alloc(sizeof(insn));
> > +	memcpy(prog, insn, sizeof(insn));
> > +
> > +	memset(attr, 0, sizeof(*attr));
> > +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
> > +	attr->insns = ptr_to_u64(prog);
> > +	attr->insn_cnt = ARRAY_SIZE(insn);
> > +	attr->license = ptr_to_u64("GPL");
> > +	attr->log_buf = ptr_to_u64(log);
> > +	attr->log_size = BUFSIZ;
> > +	attr->log_level = 1;
> > +
> > +	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
> > +	if (TST_RET == -1) {
> > +		if (log[0] != 0) {
> > +			tst_res(TINFO, "Verification log:");
> > +			fputs(log, stderr);
> > +			tst_brk(TBROK | TTERRNO, "Failed
> > verification");
> > +		} else {
> > +			tst_brk(TBROK | TTERRNO, "Failed to load
> > program");
> > +		}
> > +	}
> > +
> > +	return TST_RET;
> > +}
> > +
> > +static void setup(void)
> > +{
> > +	memcpy(msg, MSG, sizeof(MSG));
> > +}
> > +
> > +static void run(void)
> > +{
> > +	int map_fd, prog_fd;
> > +	int sk[2];
> > +
> > +	memset(attr, 0, sizeof(*attr));
> > +	attr->map_type = BPF_MAP_TYPE_ARRAY;
> > +	attr->key_size = 4;
> > +	attr->value_size = 8;
> > +	attr->max_entries = 2;
> > +
> > +	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
> > +	if (TST_RET == -1) {
> > +		if (TST_ERR == EPERM) {
> > +			tst_brk(TCONF | TTERRNO,
> > +				"bpf() requires CAP_SYS_ADMIN on
> > this system");
> > +		} else {
> > +			tst_brk(TBROK | TTERRNO, "Failed to create
> > array map");
> > +		}
> > +	}
> > +	map_fd = TST_RET;
> > +
> > +	prog_fd = load_prog(map_fd);
> > +
> > +	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
> > +	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
> > +			&prog_fd, sizeof(prog_fd));
> > +
> > +	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
> > +
> > +	memset(attr, 0, sizeof(*attr));
> > +	attr->map_fd = map_fd;
> > +	attr->key = ptr_to_u64(key);
> > +	attr->value = ptr_to_u64(val);
> > +	*key = 0;
> > +
> > +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
> > +	if (TST_RET == -1) {
> > +		tst_res(TFAIL | TTERRNO, "array map lookup");
> > +	} else if (*val != A64INT + 1) {
> > +		tst_res(TFAIL,
> > +			"val = %lu, but should be val = %lu + 1",
>                                 ^
> Not sure if it is really needed but I would use %llu here. If so, other
> places as well.

Technically we should be using PRIu64 which expands to %lu on 64 bit and
to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
64 bit on Linux %llu should work as well.
Richard Palethorpe Sept. 17, 2019, 7:26 a.m. UTC | #4
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
> A few minor points below, I guess I can fix these before pushing as
> well.
>
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> Reviewed-by: Jan Stancek <jstancek@redhat.com>
>> Reviewed-by: Cyril Hrubis <chrubis@suse.cz>
>> ---
>> 
>> V3: Rebased on master and included line numbers in instructions
>> 
>> Capability patch has not been applied to master at time of rebase.
>> 
>>  include/lapi/bpf.h                         |  27 +++
>>  runtest/syscalls                           |   1 +
>>  testcases/kernel/syscalls/bpf/.gitignore   |   1 +
>>  testcases/kernel/syscalls/bpf/bpf_prog02.c | 182 +++++++++++++++++++++
>>  4 files changed, 211 insertions(+)
>>  create mode 100644 testcases/kernel/syscalls/bpf/bpf_prog02.c
>> 
>> diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
>> index 122eb5469..03073b45e 100644
>> --- a/include/lapi/bpf.h
>> +++ b/include/lapi/bpf.h
>> @@ -18,7 +18,9 @@
>>  /* Start copy from linux/bpf_(common).h */
>>  #define BPF_CLASS(code) ((code) & 0x07)
>>  #define		BPF_LD		0x00
>> +#define		BPF_LDX		0x01
>>  #define		BPF_ST		0x02
>> +#define		BPF_STX		0x03
>>  #define		BPF_JMP		0x05
>>  
>>  #define BPF_SIZE(code)  ((code) & 0x18)
>> @@ -30,6 +32,7 @@
>>  
>>  #define BPF_OP(code)    ((code) & 0xf0)
>>  #define		BPF_ADD		0x00
>> +#define		BPF_SUB		0x10
>>  
>>  #define		BPF_JEQ		0x10
>>  
>> @@ -432,6 +435,14 @@ enum bpf_func_id {
>>  
>>  /* Start copy from tools/include/filter.h */
>>  
>> +#define BPF_ALU64_REG(OP, DST, SRC)				\
>> +	((struct bpf_insn) {					\
>> +		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
>> +		.dst_reg = DST,					\
>> +		.src_reg = SRC,					\
>> +		.off   = 0,					\
>> +		.imm   = 0 })
>> +
>>  #define BPF_ALU64_IMM(OP, DST, IMM)				\
>>  	((struct bpf_insn) {					\
>>  		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_K,	\
>> @@ -477,6 +488,22 @@ enum bpf_func_id {
>>  		.off   = OFF,					\
>>  		.imm   = IMM })
>>  
>> +#define BPF_LDX_MEM(SIZE, DST, SRC, OFF)			\
>> +	((struct bpf_insn) {					\
>> +		.code  = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM,	\
>> +		.dst_reg = DST,					\
>> +		.src_reg = SRC,					\
>> +		.off   = OFF,					\
>> +		.imm   = 0 })
>> +
>> +#define BPF_STX_MEM(SIZE, DST, SRC, OFF)			\
>> +	((struct bpf_insn) {					\
>> +		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM,	\
>> +		.dst_reg = DST,					\
>> +		.src_reg = SRC,					\
>> +		.off   = OFF,					\
>> +		.imm   = 0 })
>> +
>>  #define BPF_JMP_IMM(OP, DST, IMM, OFF)				\
>>  	((struct bpf_insn) {					\
>>  		.code  = BPF_JMP | BPF_OP(OP) | BPF_K,		\
>> diff --git a/runtest/syscalls b/runtest/syscalls
>> index 874ae4d4f..4e6310193 100644
>> --- a/runtest/syscalls
>> +++ b/runtest/syscalls
>> @@ -34,6 +34,7 @@ bind03 bind03
>>  
>>  bpf_map01 bpf_map01
>>  bpf_prog01 bpf_prog01
>> +bpf_prog02 bpf_prog02
>>  
>>  brk01 brk01
>>  
>> diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
>> index 7eb5f7c92..1704f9841 100644
>> --- a/testcases/kernel/syscalls/bpf/.gitignore
>> +++ b/testcases/kernel/syscalls/bpf/.gitignore
>> @@ -1,2 +1,3 @@
>>  bpf_map01
>>  bpf_prog01
>> +bpf_prog02
>> diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
>> new file mode 100644
>> index 000000000..dc8b92f00
>> --- /dev/null
>> +++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
>> @@ -0,0 +1,182 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
>> + *
>> + * Check if eBPF can do arithmetic with 64bits. This targets a specific
>> + * regression which only effects unprivileged users who are subject to extra
>> + * pointer arithmetic checks during verification.
>> + *
>> + * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
>> + * https://new.blog.cloudflare.com/ebpf-cant-count/
>> + *
>> + * This test is very similar in structure to bpf_prog01 which is better
>> + * annotated.
>> + */
>> +
>> +#include <limits.h>
>> +#include <string.h>
>> +#include <stdio.h>
>> +
>> +#include "config.h"
>> +#include "tst_test.h"
>> +#include "tst_capability.h"
>> +#include "lapi/socket.h"
>> +#include "lapi/bpf.h"
>> +
>> +#define A64INT (((uint64_t)1) << 60)
>> +
>> +const char MSG[] = "Ahoj!";
>> +static char *msg;
>> +
>> +static char *log;
>> +static uint32_t *key;
>> +static uint64_t *val;
>> +static union bpf_attr *attr;
>> +
>> +static int load_prog(int fd)
>> +{
>> +	struct bpf_insn *prog;
>> +	struct bpf_insn insn[] = {
>> +		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1 */
>> +
>> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 1: r1 = &fd */
>> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 3: r2 = fp */
>> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 4: r2 = r2 - 8 */
>> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* 5: *r2 = 0 */
>> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
>> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
>> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 8: r3 = r0 */
>> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 9: r4 = 2^61 */
>> +		BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
>> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
>> +
>> +		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 13: r1 = &fd */
>> +		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 15: r2 = fp */
>> +		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 16: r2 = r2 - 8 */
>> +		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1),    /* 17: *r2 = 1 */
>> +		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
>> +		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),  /* 19: if(!r0) goto 25 */
>> +		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 20: r3 = r0 */
>> +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 = 2^61 */
>> +		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
>> +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
>> +
>> +		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 = 0 */
>> +		BPF_EXIT_INSN(),		        /* 26: return r0 */
>> +	};
>> +
>> +	/* Leaks memory when -i is specified */
>> +	prog = tst_alloc(sizeof(insn));
>
> Why not just declare the prog static? Then we could do:
>
> 	if (!prog)
> 		prog = tst_alloc(sizeof(insn));

OK!

>
>> +	memcpy(prog, insn, sizeof(insn));
>> +
>> +	memset(attr, 0, sizeof(*attr));
>> +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>> +	attr->insns = ptr_to_u64(prog);
>> +	attr->insn_cnt = ARRAY_SIZE(insn);
>> +	attr->license = ptr_to_u64("GPL");
>> +	attr->log_buf = ptr_to_u64(log);
>> +	attr->log_size = BUFSIZ;
>> +	attr->log_level = 1;
>> +
>> +	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
>> +	if (TST_RET == -1) {
>> +		if (log[0] != 0) {
>> +			tst_res(TINFO, "Verification log:");
>> +			fputs(log, stderr);
>
> I guess that we can do tst_res(TINFO, "Verification log:\n%s", log);
> instead.

Nope, the log is often too long for tst_res and is truncated.

>
>> +			tst_brk(TBROK | TTERRNO, "Failed verification");
>> +		} else {
>> +			tst_brk(TBROK | TTERRNO, "Failed to load program");
>> +		}
>> +	}
>> +
>> +	return TST_RET;
>> +}
>> +
>> +static void setup(void)
>> +{
>> +	memcpy(msg, MSG, sizeof(MSG));
>> +}
>> +
>> +static void run(void)
>> +{
>> +	int map_fd, prog_fd;
>> +	int sk[2];
>> +
>> +	memset(attr, 0, sizeof(*attr));
>> +	attr->map_type = BPF_MAP_TYPE_ARRAY;
>> +	attr->key_size = 4;
>> +	attr->value_size = 8;
>> +	attr->max_entries = 2;
>> +
>> +	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
>> +	if (TST_RET == -1) {
>> +		if (TST_ERR == EPERM) {
>> +			tst_brk(TCONF | TTERRNO,
>> +				"bpf() requires CAP_SYS_ADMIN on this system");
>> +		} else {
>> +			tst_brk(TBROK | TTERRNO, "Failed to create array map");
>> +		}
>> +	}
>> +	map_fd = TST_RET;
>> +
>> +	prog_fd = load_prog(map_fd);
>> +
>> +	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
>> +	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
>> +			&prog_fd, sizeof(prog_fd));
>> +
>> +	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
>> +
>> +	memset(attr, 0, sizeof(*attr));
>> +	attr->map_fd = map_fd;
>> +	attr->key = ptr_to_u64(key);
>> +	attr->value = ptr_to_u64(val);
>> +	*key = 0;
>> +
>> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "array map lookup");
>> +	} else if (*val != A64INT + 1) {
>> +		tst_res(TFAIL,
>> +			"val = %lu, but should be val = %lu + 1",
>> +			*val, A64INT);
>> +        } else {
>> +	        tst_res(TPASS, "val = %lu + 1", A64INT);
>> +	}
>
> Wrong indentation, also I do not fancy this if else maze. I guess that
> we can can safely do goto exit; if map lookup fails that would point
> right before the SAFE_CLOSE() block.

OK!

>
>> +	*key = 1;
>> +
>> +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> +	if (TST_RET == -1) {
>> +		tst_res(TFAIL | TTERRNO, "array map lookup");
>> +	} else if (*val != A64INT - 1) {
>> +		tst_res(TFAIL,
>> +			"val = %lu, but should be val = %lu - 1",
>> +			*val, A64INT);
>> +        } else {
>> +	        tst_res(TPASS, "val = %lu - 1", A64INT);
>> +	}
>
>
> Here as well.

OK!

>
>> +	SAFE_CLOSE(prog_fd);
>> +	SAFE_CLOSE(map_fd);
>> +	SAFE_CLOSE(sk[0]);
>> +	SAFE_CLOSE(sk[1]);
>> +}
>> +
>> +static struct tst_test test = {
>> +	.setup = setup,
>> +	.test_all = run,
>> +	.min_kver = "3.18",
>> +	.caps = (struct tst_cap []) {
>> +		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
>> +		{}
>> +	},
>> +	.bufs = (struct tst_buffers []) {
>> +		{&key, .size = sizeof(*key)},
>> +		{&val, .size = sizeof(*val)},
>> +		{&log, .size = BUFSIZ},
>> +		{&attr, .size = sizeof(*attr)},
>> +		{&msg, .size = sizeof(MSG)},
>> +		{},
>> +	}
>> +};
>> -- 
>> 2.22.1
>>
Richard Palethorpe Sept. 17, 2019, 7:27 a.m. UTC | #5
Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> > +		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 =
>> > 2^61 */
>>   ^
>>   I think 2^61 is different to 1<<60
>
> Agreed, 1<<60 is 2^60
>
>> > +		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23:
>> > r4 -= r6 */
>> > +		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24:
>> > *r3 = r4 */
>> > +
>> > +		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 =
>> > 0 */
>> > +		BPF_EXIT_INSN(),		        /* 26:
>> > return r0 */
>> > +	};
>> > +
>> > +	/* Leaks memory when -i is specified */
>> > +	prog = tst_alloc(sizeof(insn));
>> > +	memcpy(prog, insn, sizeof(insn));
>> > +
>> > +	memset(attr, 0, sizeof(*attr));
>> > +	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
>> > +	attr->insns = ptr_to_u64(prog);
>> > +	attr->insn_cnt = ARRAY_SIZE(insn);
>> > +	attr->license = ptr_to_u64("GPL");
>> > +	attr->log_buf = ptr_to_u64(log);
>> > +	attr->log_size = BUFSIZ;
>> > +	attr->log_level = 1;
>> > +
>> > +	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
>> > +	if (TST_RET == -1) {
>> > +		if (log[0] != 0) {
>> > +			tst_res(TINFO, "Verification log:");
>> > +			fputs(log, stderr);
>> > +			tst_brk(TBROK | TTERRNO, "Failed
>> > verification");
>> > +		} else {
>> > +			tst_brk(TBROK | TTERRNO, "Failed to load
>> > program");
>> > +		}
>> > +	}
>> > +
>> > +	return TST_RET;
>> > +}
>> > +
>> > +static void setup(void)
>> > +{
>> > +	memcpy(msg, MSG, sizeof(MSG));
>> > +}
>> > +
>> > +static void run(void)
>> > +{
>> > +	int map_fd, prog_fd;
>> > +	int sk[2];
>> > +
>> > +	memset(attr, 0, sizeof(*attr));
>> > +	attr->map_type = BPF_MAP_TYPE_ARRAY;
>> > +	attr->key_size = 4;
>> > +	attr->value_size = 8;
>> > +	attr->max_entries = 2;
>> > +
>> > +	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
>> > +	if (TST_RET == -1) {
>> > +		if (TST_ERR == EPERM) {
>> > +			tst_brk(TCONF | TTERRNO,
>> > +				"bpf() requires CAP_SYS_ADMIN on
>> > this system");
>> > +		} else {
>> > +			tst_brk(TBROK | TTERRNO, "Failed to create
>> > array map");
>> > +		}
>> > +	}
>> > +	map_fd = TST_RET;
>> > +
>> > +	prog_fd = load_prog(map_fd);
>> > +
>> > +	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
>> > +	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
>> > +			&prog_fd, sizeof(prog_fd));
>> > +
>> > +	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
>> > +
>> > +	memset(attr, 0, sizeof(*attr));
>> > +	attr->map_fd = map_fd;
>> > +	attr->key = ptr_to_u64(key);
>> > +	attr->value = ptr_to_u64(val);
>> > +	*key = 0;
>> > +
>> > +	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
>> > +	if (TST_RET == -1) {
>> > +		tst_res(TFAIL | TTERRNO, "array map lookup");
>> > +	} else if (*val != A64INT + 1) {
>> > +		tst_res(TFAIL,
>> > +			"val = %lu, but should be val = %lu + 1",
>>                                 ^
>> Not sure if it is really needed but I would use %llu here. If so, other
>> places as well.
>
> Technically we should be using PRIu64 which expands to %lu on 64 bit and
> to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
> 64 bit on Linux %llu should work as well.

Ack.
Cyril Hrubis Sept. 27, 2019, 11:08 a.m. UTC | #6
Hi!
> > Technically we should be using PRIu64 which expands to %lu on 64 bit and
> > to %llu on 32 bit. But given that sizeof(long long) == sizeof(long) on
> > 64 bit on Linux %llu should work as well.
> 
> Ack.

In the end I've pushed this with the PRIu64 since the compiler was
throwing warnings for %llu on 64 bit.

Patch
diff mbox series

diff --git a/include/lapi/bpf.h b/include/lapi/bpf.h
index 122eb5469..03073b45e 100644
--- a/include/lapi/bpf.h
+++ b/include/lapi/bpf.h
@@ -18,7 +18,9 @@ 
 /* Start copy from linux/bpf_(common).h */
 #define BPF_CLASS(code) ((code) & 0x07)
 #define		BPF_LD		0x00
+#define		BPF_LDX		0x01
 #define		BPF_ST		0x02
+#define		BPF_STX		0x03
 #define		BPF_JMP		0x05
 
 #define BPF_SIZE(code)  ((code) & 0x18)
@@ -30,6 +32,7 @@ 
 
 #define BPF_OP(code)    ((code) & 0xf0)
 #define		BPF_ADD		0x00
+#define		BPF_SUB		0x10
 
 #define		BPF_JEQ		0x10
 
@@ -432,6 +435,14 @@  enum bpf_func_id {
 
 /* Start copy from tools/include/filter.h */
 
+#define BPF_ALU64_REG(OP, DST, SRC)				\
+	((struct bpf_insn) {					\
+		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_X,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = 0,					\
+		.imm   = 0 })
+
 #define BPF_ALU64_IMM(OP, DST, IMM)				\
 	((struct bpf_insn) {					\
 		.code  = BPF_ALU64 | BPF_OP(OP) | BPF_K,	\
@@ -477,6 +488,22 @@  enum bpf_func_id {
 		.off   = OFF,					\
 		.imm   = IMM })
 
+#define BPF_LDX_MEM(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_LDX | BPF_SIZE(SIZE) | BPF_MEM,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = 0 })
+
+#define BPF_STX_MEM(SIZE, DST, SRC, OFF)			\
+	((struct bpf_insn) {					\
+		.code  = BPF_STX | BPF_SIZE(SIZE) | BPF_MEM,	\
+		.dst_reg = DST,					\
+		.src_reg = SRC,					\
+		.off   = OFF,					\
+		.imm   = 0 })
+
 #define BPF_JMP_IMM(OP, DST, IMM, OFF)				\
 	((struct bpf_insn) {					\
 		.code  = BPF_JMP | BPF_OP(OP) | BPF_K,		\
diff --git a/runtest/syscalls b/runtest/syscalls
index 874ae4d4f..4e6310193 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -34,6 +34,7 @@  bind03 bind03
 
 bpf_map01 bpf_map01
 bpf_prog01 bpf_prog01
+bpf_prog02 bpf_prog02
 
 brk01 brk01
 
diff --git a/testcases/kernel/syscalls/bpf/.gitignore b/testcases/kernel/syscalls/bpf/.gitignore
index 7eb5f7c92..1704f9841 100644
--- a/testcases/kernel/syscalls/bpf/.gitignore
+++ b/testcases/kernel/syscalls/bpf/.gitignore
@@ -1,2 +1,3 @@ 
 bpf_map01
 bpf_prog01
+bpf_prog02
diff --git a/testcases/kernel/syscalls/bpf/bpf_prog02.c b/testcases/kernel/syscalls/bpf/bpf_prog02.c
new file mode 100644
index 000000000..dc8b92f00
--- /dev/null
+++ b/testcases/kernel/syscalls/bpf/bpf_prog02.c
@@ -0,0 +1,182 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2019 Richard Palethorpe <rpalethorpe@suse.com>
+ *
+ * Check if eBPF can do arithmetic with 64bits. This targets a specific
+ * regression which only effects unprivileged users who are subject to extra
+ * pointer arithmetic checks during verification.
+ *
+ * Fixed by commit 3612af783cf52c74a031a2f11b82247b2599d3cd.
+ * https://new.blog.cloudflare.com/ebpf-cant-count/
+ *
+ * This test is very similar in structure to bpf_prog01 which is better
+ * annotated.
+ */
+
+#include <limits.h>
+#include <string.h>
+#include <stdio.h>
+
+#include "config.h"
+#include "tst_test.h"
+#include "tst_capability.h"
+#include "lapi/socket.h"
+#include "lapi/bpf.h"
+
+#define A64INT (((uint64_t)1) << 60)
+
+const char MSG[] = "Ahoj!";
+static char *msg;
+
+static char *log;
+static uint32_t *key;
+static uint64_t *val;
+static union bpf_attr *attr;
+
+static int load_prog(int fd)
+{
+	struct bpf_insn *prog;
+	struct bpf_insn insn[] = {
+		BPF_MOV64_IMM(BPF_REG_6, 1),            /* 0: r6 = 1 */
+
+		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 1: r1 = &fd */
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 3: r2 = fp */
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 4: r2 = r2 - 8 */
+		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),    /* 5: *r2 = 0 */
+		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 6: map_lookup_elem */
+		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 17), /* 7: if(!r0) goto 25 */
+		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 8: r3 = r0 */
+		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 9: r4 = 2^61 */
+		BPF_ALU64_REG(BPF_ADD, BPF_REG_4, BPF_REG_6), /* 11: r4 += r6 */
+		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 12: *r3 = r4 */
+
+		BPF_LD_MAP_FD(BPF_REG_1, fd),	        /* 13: r1 = &fd */
+		BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),   /* 15: r2 = fp */
+		BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),  /* 16: r2 = r2 - 8 */
+		BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 1),    /* 17: *r2 = 1 */
+		BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),/* 18: map_lookup_elem */
+		BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 5),  /* 19: if(!r0) goto 25 */
+		BPF_MOV64_REG(BPF_REG_3, BPF_REG_0),    /* 20: r3 = r0 */
+		BPF_LD_IMM64(BPF_REG_4, A64INT),        /* 21: r4 = 2^61 */
+		BPF_ALU64_REG(BPF_SUB, BPF_REG_4, BPF_REG_6), /* 23: r4 -= r6 */
+		BPF_STX_MEM(BPF_DW, BPF_REG_3, BPF_REG_4, 0), /* 24: *r3 = r4 */
+
+		BPF_MOV64_IMM(BPF_REG_0, 0),            /* 25: r0 = 0 */
+		BPF_EXIT_INSN(),		        /* 26: return r0 */
+	};
+
+	/* Leaks memory when -i is specified */
+	prog = tst_alloc(sizeof(insn));
+	memcpy(prog, insn, sizeof(insn));
+
+	memset(attr, 0, sizeof(*attr));
+	attr->prog_type = BPF_PROG_TYPE_SOCKET_FILTER;
+	attr->insns = ptr_to_u64(prog);
+	attr->insn_cnt = ARRAY_SIZE(insn);
+	attr->license = ptr_to_u64("GPL");
+	attr->log_buf = ptr_to_u64(log);
+	attr->log_size = BUFSIZ;
+	attr->log_level = 1;
+
+	TEST(bpf(BPF_PROG_LOAD, attr, sizeof(*attr)));
+	if (TST_RET == -1) {
+		if (log[0] != 0) {
+			tst_res(TINFO, "Verification log:");
+			fputs(log, stderr);
+			tst_brk(TBROK | TTERRNO, "Failed verification");
+		} else {
+			tst_brk(TBROK | TTERRNO, "Failed to load program");
+		}
+	}
+
+	return TST_RET;
+}
+
+static void setup(void)
+{
+	memcpy(msg, MSG, sizeof(MSG));
+}
+
+static void run(void)
+{
+	int map_fd, prog_fd;
+	int sk[2];
+
+	memset(attr, 0, sizeof(*attr));
+	attr->map_type = BPF_MAP_TYPE_ARRAY;
+	attr->key_size = 4;
+	attr->value_size = 8;
+	attr->max_entries = 2;
+
+	TEST(bpf(BPF_MAP_CREATE, attr, sizeof(*attr)));
+	if (TST_RET == -1) {
+		if (TST_ERR == EPERM) {
+			tst_brk(TCONF | TTERRNO,
+				"bpf() requires CAP_SYS_ADMIN on this system");
+		} else {
+			tst_brk(TBROK | TTERRNO, "Failed to create array map");
+		}
+	}
+	map_fd = TST_RET;
+
+	prog_fd = load_prog(map_fd);
+
+	SAFE_SOCKETPAIR(AF_UNIX, SOCK_DGRAM, 0, sk);
+	SAFE_SETSOCKOPT(sk[1], SOL_SOCKET, SO_ATTACH_BPF,
+			&prog_fd, sizeof(prog_fd));
+
+	SAFE_WRITE(1, sk[0], msg, sizeof(MSG));
+
+	memset(attr, 0, sizeof(*attr));
+	attr->map_fd = map_fd;
+	attr->key = ptr_to_u64(key);
+	attr->value = ptr_to_u64(val);
+	*key = 0;
+
+	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
+	if (TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, "array map lookup");
+	} else if (*val != A64INT + 1) {
+		tst_res(TFAIL,
+			"val = %lu, but should be val = %lu + 1",
+			*val, A64INT);
+        } else {
+	        tst_res(TPASS, "val = %lu + 1", A64INT);
+	}
+
+	*key = 1;
+
+	TEST(bpf(BPF_MAP_LOOKUP_ELEM, attr, sizeof(*attr)));
+	if (TST_RET == -1) {
+		tst_res(TFAIL | TTERRNO, "array map lookup");
+	} else if (*val != A64INT - 1) {
+		tst_res(TFAIL,
+			"val = %lu, but should be val = %lu - 1",
+			*val, A64INT);
+        } else {
+	        tst_res(TPASS, "val = %lu - 1", A64INT);
+	}
+
+	SAFE_CLOSE(prog_fd);
+	SAFE_CLOSE(map_fd);
+	SAFE_CLOSE(sk[0]);
+	SAFE_CLOSE(sk[1]);
+}
+
+static struct tst_test test = {
+	.setup = setup,
+	.test_all = run,
+	.min_kver = "3.18",
+	.caps = (struct tst_cap []) {
+		TST_CAP(TST_CAP_DROP, CAP_SYS_ADMIN),
+		{}
+	},
+	.bufs = (struct tst_buffers []) {
+		{&key, .size = sizeof(*key)},
+		{&val, .size = sizeof(*val)},
+		{&log, .size = BUFSIZ},
+		{&attr, .size = sizeof(*attr)},
+		{&msg, .size = sizeof(MSG)},
+		{},
+	}
+};