1
0
Fork 0
Daniel Borkmann says:

====================
pull-request: bpf 2021-01-16

1) Fix a double bpf_prog_put() for BPF_PROG_{TYPE_EXT,TYPE_TRACING} types in
   link creation's error path causing a refcount underflow, from Jiri Olsa.

2) Fix BTF validation errors for the case where kernel modules don't declare
   any new types and end up with an empty BTF, from Andrii Nakryiko.

3) Fix BPF local storage helpers to first check their {task,inode} owners for
   being NULL before access, from KP Singh.

4) Fix a memory leak in BPF setsockopt handling for the case where optlen is
   zero and thus temporary optval buffer should be freed, from Stanislav Fomichev.

5) Fix a syzbot memory allocation splat in BPF_PROG_TEST_RUN infra for
   raw_tracepoint caused by too big ctx_size_in, from Song Liu.

6) Fix LLVM code generation issues with verifier where PTR_TO_MEM{,_OR_NULL}
   registers were spilled to stack but not recognized, from Gilad Reti.

* https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf:
  MAINTAINERS: Update my email address
  selftests/bpf: Add verifier test for PTR_TO_MEM spill
  bpf: Support PTR_TO_MEM{,_OR_NULL} register spilling
  bpf: Reject too big ctx_size_in for raw_tp test run
  libbpf: Allow loading empty BTFs
  bpf: Allow empty module BTFs
  bpf: Don't leak memory in bpf getsockopt when optlen == 0
  bpf: Update local storage test to check handling of null ptrs
  bpf: Fix typo in bpf_inode_storage.c
  bpf: Local storage helpers should check nullness of owner ptr passed
  bpf: Prevent double bpf_prog_put call from bpf_tracing_prog_attach
====================

Link: https://lore.kernel.org/r/20210116002025.15706-1-daniel@iogearbox.net
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
master
Jakub Kicinski 2021-01-15 16:34:59 -08:00
commit e23a8d0021
14 changed files with 128 additions and 115 deletions

View File

@ -55,6 +55,8 @@ Bart Van Assche <bvanassche@acm.org> <bart.vanassche@wdc.com>
Ben Gardner <bgardner@wabtec.com>
Ben M Cahill <ben.m.cahill@intel.com>
Björn Steinbrink <B.Steinbrink@gmx.de>
Björn Töpel <bjorn@kernel.org> <bjorn.topel@gmail.com>
Björn Töpel <bjorn@kernel.org> <bjorn.topel@intel.com>
Boris Brezillon <bbrezillon@kernel.org> <b.brezillon.dev@gmail.com>
Boris Brezillon <bbrezillon@kernel.org> <b.brezillon@overkiz.com>
Boris Brezillon <bbrezillon@kernel.org> <boris.brezillon@bootlin.com>

View File

@ -3334,7 +3334,7 @@ F: arch/riscv/net/
X: arch/riscv/net/bpf_jit_comp64.c
BPF JIT for RISC-V (64-bit)
M: Björn Töpel <bjorn.topel@gmail.com>
M: Björn Töpel <bjorn@kernel.org>
L: netdev@vger.kernel.org
L: bpf@vger.kernel.org
S: Maintained
@ -19415,7 +19415,7 @@ F: drivers/net/ethernet/*/*/*xdp*
K: (?:\b|_)xdp(?:\b|_)
XDP SOCKETS (AF_XDP)
M: Björn Töpel <bjorn.topel@intel.com>
M: Björn Töpel <bjorn@kernel.org>
M: Magnus Karlsson <magnus.karlsson@intel.com>
R: Jonathan Lemon <jonathan.lemon@gmail.com>
L: netdev@vger.kernel.org

View File

@ -176,14 +176,14 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
* bpf_local_storage_update expects the owner to have a
* valid storage pointer.
*/
if (!inode_storage_ptr(inode))
if (!inode || !inode_storage_ptr(inode))
return (unsigned long)NULL;
sdata = inode_storage_lookup(inode, map, true);
if (sdata)
return (unsigned long)sdata->data;
/* This helper must only called from where the inode is gurranteed
/* This helper must only called from where the inode is guaranteed
* to have a refcount and cannot be freed.
*/
if (flags & BPF_LOCAL_STORAGE_GET_F_CREATE) {
@ -200,7 +200,10 @@ BPF_CALL_4(bpf_inode_storage_get, struct bpf_map *, map, struct inode *, inode,
BPF_CALL_2(bpf_inode_storage_delete,
struct bpf_map *, map, struct inode *, inode)
{
/* This helper must only called from where the inode is gurranteed
if (!inode)
return -EINVAL;
/* This helper must only called from where the inode is guaranteed
* to have a refcount and cannot be freed.
*/
return inode_storage_delete(inode, map);

View File

@ -218,7 +218,7 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
* bpf_local_storage_update expects the owner to have a
* valid storage pointer.
*/
if (!task_storage_ptr(task))
if (!task || !task_storage_ptr(task))
return (unsigned long)NULL;
sdata = task_storage_lookup(task, map, true);
@ -243,6 +243,9 @@ BPF_CALL_4(bpf_task_storage_get, struct bpf_map *, map, struct task_struct *,
BPF_CALL_2(bpf_task_storage_delete, struct bpf_map *, map, struct task_struct *,
task)
{
if (!task)
return -EINVAL;
/* This helper must only be called from places where the lifetime of the task
* is guaranteed. Either by being refcounted or by being protected
* by an RCU read-side critical section.

View File

@ -4172,7 +4172,7 @@ static int btf_parse_hdr(struct btf_verifier_env *env)
return -ENOTSUPP;
}
if (btf_data_size == hdr->hdr_len) {
if (!btf->base_btf && btf_data_size == hdr->hdr_len) {
btf_verifier_log(env, "No data");
return -EINVAL;
}

View File

@ -1391,12 +1391,13 @@ int __cgroup_bpf_run_filter_setsockopt(struct sock *sk, int *level,
if (ctx.optlen != 0) {
*optlen = ctx.optlen;
*kernel_optval = ctx.optval;
/* export and don't free sockopt buf */
return 0;
}
}
out:
if (ret)
sockopt_free_buf(&ctx);
sockopt_free_buf(&ctx);
return ret;
}

View File

@ -2712,7 +2712,6 @@ out_unlock:
out_put_prog:
if (tgt_prog_fd && tgt_prog)
bpf_prog_put(tgt_prog);
bpf_prog_put(prog);
return err;
}
@ -2825,7 +2824,10 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
tp_name = prog->aux->attach_func_name;
break;
}
return bpf_tracing_prog_attach(prog, 0, 0);
err = bpf_tracing_prog_attach(prog, 0, 0);
if (err >= 0)
return err;
goto out_put_prog;
case BPF_PROG_TYPE_RAW_TRACEPOINT:
case BPF_PROG_TYPE_RAW_TRACEPOINT_WRITABLE:
if (strncpy_from_user(buf,

View File

@ -2217,6 +2217,8 @@ static bool is_spillable_regtype(enum bpf_reg_type type)
case PTR_TO_RDWR_BUF:
case PTR_TO_RDWR_BUF_OR_NULL:
case PTR_TO_PERCPU_BTF_ID:
case PTR_TO_MEM:
case PTR_TO_MEM_OR_NULL:
return true;
default:
return false;

View File

@ -272,7 +272,8 @@ int bpf_prog_test_run_raw_tp(struct bpf_prog *prog,
kattr->test.repeat)
return -EINVAL;
if (ctx_size_in < prog->aux->max_ctx_offset)
if (ctx_size_in < prog->aux->max_ctx_offset ||
ctx_size_in > MAX_BPF_FUNC_ARGS * sizeof(u64))
return -EINVAL;
if ((kattr->test.flags & BPF_F_TEST_RUN_ON_CPU) == 0 && cpu != 0)

View File

@ -240,11 +240,6 @@ static int btf_parse_hdr(struct btf *btf)
}
meta_left = btf->raw_size - sizeof(*hdr);
if (!meta_left) {
pr_debug("BTF has no data\n");
return -EINVAL;
}
if (meta_left < hdr->str_off + hdr->str_len) {
pr_debug("Invalid BTF total size:%u\n", btf->raw_size);
return -EINVAL;

View File

@ -34,61 +34,6 @@ struct storage {
struct bpf_spin_lock lock;
};
/* Copies an rm binary to a temp file. dest is a mkstemp template */
static int copy_rm(char *dest)
{
int fd_in, fd_out = -1, ret = 0;
struct stat stat;
char *buf = NULL;
fd_in = open("/bin/rm", O_RDONLY);
if (fd_in < 0)
return -errno;
fd_out = mkstemp(dest);
if (fd_out < 0) {
ret = -errno;
goto out;
}
ret = fstat(fd_in, &stat);
if (ret == -1) {
ret = -errno;
goto out;
}
buf = malloc(stat.st_blksize);
if (!buf) {
ret = -errno;
goto out;
}
while (ret = read(fd_in, buf, stat.st_blksize), ret > 0) {
ret = write(fd_out, buf, ret);
if (ret < 0) {
ret = -errno;
goto out;
}
}
if (ret < 0) {
ret = -errno;
goto out;
}
/* Set executable permission on the copied file */
ret = chmod(dest, 0100);
if (ret == -1)
ret = -errno;
out:
free(buf);
close(fd_in);
close(fd_out);
return ret;
}
/* Fork and exec the provided rm binary and return the exit code of the
* forked process and its pid.
*/
@ -168,9 +113,11 @@ static bool check_syscall_operations(int map_fd, int obj_fd)
void test_test_local_storage(void)
{
char tmp_exec_path[PATH_MAX] = "/tmp/copy_of_rmXXXXXX";
char tmp_dir_path[64] = "/tmp/local_storageXXXXXX";
int err, serv_sk = -1, task_fd = -1, rm_fd = -1;
struct local_storage *skel = NULL;
char tmp_exec_path[64];
char cmd[256];
skel = local_storage__open_and_load();
if (CHECK(!skel, "skel_load", "lsm skeleton failed\n"))
@ -189,18 +136,24 @@ void test_test_local_storage(void)
task_fd))
goto close_prog;
err = copy_rm(tmp_exec_path);
if (CHECK(err < 0, "copy_rm", "err %d errno %d\n", err, errno))
if (CHECK(!mkdtemp(tmp_dir_path), "mkdtemp",
"unable to create tmpdir: %d\n", errno))
goto close_prog;
snprintf(tmp_exec_path, sizeof(tmp_exec_path), "%s/copy_of_rm",
tmp_dir_path);
snprintf(cmd, sizeof(cmd), "cp /bin/rm %s", tmp_exec_path);
if (CHECK_FAIL(system(cmd)))
goto close_prog_rmdir;
rm_fd = open(tmp_exec_path, O_RDONLY);
if (CHECK(rm_fd < 0, "open", "failed to open %s err:%d, errno:%d",
tmp_exec_path, rm_fd, errno))
goto close_prog;
goto close_prog_rmdir;
if (!check_syscall_operations(bpf_map__fd(skel->maps.inode_storage_map),
rm_fd))
goto close_prog;
goto close_prog_rmdir;
/* Sets skel->bss->monitored_pid to the pid of the forked child
* forks a child process that executes tmp_exec_path and tries to
@ -209,33 +162,36 @@ void test_test_local_storage(void)
*/
err = run_self_unlink(&skel->bss->monitored_pid, tmp_exec_path);
if (CHECK(err != EPERM, "run_self_unlink", "err %d want EPERM\n", err))
goto close_prog_unlink;
goto close_prog_rmdir;
/* Set the process being monitored to be the current process */
skel->bss->monitored_pid = getpid();
/* Remove the temporary created executable */
err = unlink(tmp_exec_path);
if (CHECK(err != 0, "unlink", "unable to unlink %s: %d", tmp_exec_path,
errno))
goto close_prog_unlink;
/* Move copy_of_rm to a new location so that it triggers the
* inode_rename LSM hook with a new_dentry that has a NULL inode ptr.
*/
snprintf(cmd, sizeof(cmd), "mv %s/copy_of_rm %s/check_null_ptr",
tmp_dir_path, tmp_dir_path);
if (CHECK_FAIL(system(cmd)))
goto close_prog_rmdir;
CHECK(skel->data->inode_storage_result != 0, "inode_storage_result",
"inode_local_storage not set\n");
serv_sk = start_server(AF_INET6, SOCK_STREAM, NULL, 0, 0);
if (CHECK(serv_sk < 0, "start_server", "failed to start server\n"))
goto close_prog;
goto close_prog_rmdir;
CHECK(skel->data->sk_storage_result != 0, "sk_storage_result",
"sk_local_storage not set\n");
if (!check_syscall_operations(bpf_map__fd(skel->maps.sk_storage_map),
serv_sk))
goto close_prog;
goto close_prog_rmdir;
close_prog_unlink:
unlink(tmp_exec_path);
close_prog_rmdir:
snprintf(cmd, sizeof(cmd), "rm -rf %s", tmp_dir_path);
system(cmd);
close_prog:
close(serv_sk);
close(rm_fd);

View File

@ -50,7 +50,6 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim)
__u32 pid = bpf_get_current_pid_tgid() >> 32;
struct local_storage *storage;
bool is_self_unlink;
int err;
if (pid != monitored_pid)
return 0;
@ -66,8 +65,27 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim)
return -EPERM;
}
storage = bpf_inode_storage_get(&inode_storage_map, victim->d_inode, 0,
BPF_LOCAL_STORAGE_GET_F_CREATE);
return 0;
}
SEC("lsm/inode_rename")
int BPF_PROG(inode_rename, struct inode *old_dir, struct dentry *old_dentry,
struct inode *new_dir, struct dentry *new_dentry,
unsigned int flags)
{
__u32 pid = bpf_get_current_pid_tgid() >> 32;
struct local_storage *storage;
int err;
/* new_dentry->d_inode can be NULL when the inode is renamed to a file
* that did not exist before. The helper should be able to handle this
* NULL pointer.
*/
bpf_inode_storage_get(&inode_storage_map, new_dentry->d_inode, 0,
BPF_LOCAL_STORAGE_GET_F_CREATE);
storage = bpf_inode_storage_get(&inode_storage_map, old_dentry->d_inode,
0, 0);
if (!storage)
return 0;
@ -76,7 +94,7 @@ int BPF_PROG(unlink_hook, struct inode *dir, struct dentry *victim)
inode_storage_result = -1;
bpf_spin_unlock(&storage->lock);
err = bpf_inode_storage_delete(&inode_storage_map, victim->d_inode);
err = bpf_inode_storage_delete(&inode_storage_map, old_dentry->d_inode);
if (!err)
inode_storage_result = err;
@ -133,37 +151,18 @@ int BPF_PROG(socket_post_create, struct socket *sock, int family, int type,
return 0;
}
SEC("lsm/file_open")
int BPF_PROG(file_open, struct file *file)
{
__u32 pid = bpf_get_current_pid_tgid() >> 32;
struct local_storage *storage;
if (pid != monitored_pid)
return 0;
if (!file->f_inode)
return 0;
storage = bpf_inode_storage_get(&inode_storage_map, file->f_inode, 0,
BPF_LOCAL_STORAGE_GET_F_CREATE);
if (!storage)
return 0;
bpf_spin_lock(&storage->lock);
storage->value = DUMMY_STORAGE_VALUE;
bpf_spin_unlock(&storage->lock);
return 0;
}
/* This uses the local storage to remember the inode of the binary that a
* process was originally executing.
*/
SEC("lsm/bprm_committed_creds")
void BPF_PROG(exec, struct linux_binprm *bprm)
{
__u32 pid = bpf_get_current_pid_tgid() >> 32;
struct local_storage *storage;
if (pid != monitored_pid)
return;
storage = bpf_task_storage_get(&task_storage_map,
bpf_get_current_task_btf(), 0,
BPF_LOCAL_STORAGE_GET_F_CREATE);
@ -172,4 +171,13 @@ void BPF_PROG(exec, struct linux_binprm *bprm)
storage->exec_inode = bprm->file->f_inode;
bpf_spin_unlock(&storage->lock);
}
storage = bpf_inode_storage_get(&inode_storage_map, bprm->file->f_inode,
0, BPF_LOCAL_STORAGE_GET_F_CREATE);
if (!storage)
return;
bpf_spin_lock(&storage->lock);
storage->value = DUMMY_STORAGE_VALUE;
bpf_spin_unlock(&storage->lock);
}

View File

@ -50,7 +50,7 @@
#define MAX_INSNS BPF_MAXINSNS
#define MAX_TEST_INSNS 1000000
#define MAX_FIXUPS 8
#define MAX_NR_MAPS 20
#define MAX_NR_MAPS 21
#define MAX_TEST_RUNS 8
#define POINTER_VALUE 0xcafe4all
#define TEST_DATA_LEN 64
@ -87,6 +87,7 @@ struct bpf_test {
int fixup_sk_storage_map[MAX_FIXUPS];
int fixup_map_event_output[MAX_FIXUPS];
int fixup_map_reuseport_array[MAX_FIXUPS];
int fixup_map_ringbuf[MAX_FIXUPS];
const char *errstr;
const char *errstr_unpriv;
uint32_t insn_processed;
@ -640,6 +641,7 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
int *fixup_sk_storage_map = test->fixup_sk_storage_map;
int *fixup_map_event_output = test->fixup_map_event_output;
int *fixup_map_reuseport_array = test->fixup_map_reuseport_array;
int *fixup_map_ringbuf = test->fixup_map_ringbuf;
if (test->fill_helper) {
test->fill_insns = calloc(MAX_TEST_INSNS, sizeof(struct bpf_insn));
@ -817,6 +819,14 @@ static void do_test_fixup(struct bpf_test *test, enum bpf_prog_type prog_type,
fixup_map_reuseport_array++;
} while (*fixup_map_reuseport_array);
}
if (*fixup_map_ringbuf) {
map_fds[20] = create_map(BPF_MAP_TYPE_RINGBUF, 0,
0, 4096);
do {
prog[*fixup_map_ringbuf].imm = map_fds[20];
fixup_map_ringbuf++;
} while (*fixup_map_ringbuf);
}
}
struct libcap {

View File

@ -28,6 +28,36 @@
.result = ACCEPT,
.result_unpriv = ACCEPT,
},
{
"check valid spill/fill, ptr to mem",
.insns = {
/* reserve 8 byte ringbuf memory */
BPF_ST_MEM(BPF_DW, BPF_REG_10, -8, 0),
BPF_LD_MAP_FD(BPF_REG_1, 0),
BPF_MOV64_IMM(BPF_REG_2, 8),
BPF_MOV64_IMM(BPF_REG_3, 0),
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_reserve),
/* store a pointer to the reserved memory in R6 */
BPF_MOV64_REG(BPF_REG_6, BPF_REG_0),
/* check whether the reservation was successful */
BPF_JMP_IMM(BPF_JEQ, BPF_REG_0, 0, 6),
/* spill R6(mem) into the stack */
BPF_STX_MEM(BPF_DW, BPF_REG_10, BPF_REG_6, -8),
/* fill it back in R7 */
BPF_LDX_MEM(BPF_DW, BPF_REG_7, BPF_REG_10, -8),
/* should be able to access *(R7) = 0 */
BPF_ST_MEM(BPF_DW, BPF_REG_7, 0, 0),
/* submit the reserved ringbuf memory */
BPF_MOV64_REG(BPF_REG_1, BPF_REG_7),
BPF_MOV64_IMM(BPF_REG_2, 0),
BPF_RAW_INSN(BPF_JMP | BPF_CALL, 0, 0, 0, BPF_FUNC_ringbuf_submit),
BPF_MOV64_IMM(BPF_REG_0, 0),
BPF_EXIT_INSN(),
},
.fixup_map_ringbuf = { 1 },
.result = ACCEPT,
.result_unpriv = ACCEPT,
},
{
"check corrupted spill/fill",
.insns = {