* [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around @ 2020-05-27 18:56 Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 1/5] selftests/bpf: fix a typo in test_maps Anton Protopopov 2020-05-28 14:39 ` [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Daniel Borkmann 0 siblings, 2 replies; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:56 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest This series fixes a bug in the map_lookup_and_delete_elem() function which should check for the FMODE_CAN_READ bit, because it returns data to user space. The rest of commits fix some typos and comment in selftests and extend the test_map_wronly test to cover the new check for the BPF_MAP_TYPE_STACK and BPF_MAP_TYPE_QUEUE map types. Anton Protopopov (5): selftests/bpf: fix a typo in test_maps selftests/bpf: cleanup some file descriptors in test_maps selftests/bpf: cleanup comments in test_maps bpf: fix map permissions check selftests/bpf: add tests for write-only stacks/queues kernel/bpf/syscall.c | 3 +- tools/testing/selftests/bpf/test_maps.c | 52 ++++++++++++++++++++++--- 2 files changed, 49 insertions(+), 6 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH bpf 1/5] selftests/bpf: fix a typo in test_maps 2020-05-27 18:56 [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Anton Protopopov @ 2020-05-27 18:56 ` Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 2/5] selftests/bpf: cleanup some file descriptors " Anton Protopopov 2020-05-28 14:39 ` [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Daniel Borkmann 1 sibling, 1 reply; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:56 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest Trivial fix to a typo in the test_map_wronly test: "read" -> "write" Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/testing/selftests/bpf/test_maps.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index c6766b2cff85..f717acc0c68d 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -1410,7 +1410,7 @@ static void test_map_wronly(void) fd = bpf_create_map(BPF_MAP_TYPE_HASH, sizeof(key), sizeof(value), MAP_SIZE, map_flags | BPF_F_WRONLY); if (fd < 0) { - printf("Failed to create map for read only test '%s'!\n", + printf("Failed to create map for write only test '%s'!\n", strerror(errno)); exit(1); } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf 2/5] selftests/bpf: cleanup some file descriptors in test_maps 2020-05-27 18:56 ` [PATCH bpf 1/5] selftests/bpf: fix a typo in test_maps Anton Protopopov @ 2020-05-27 18:56 ` Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 3/5] selftests/bpf: cleanup comments " Anton Protopopov 0 siblings, 1 reply; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:56 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest The test_map_rdonly and test_map_wronly tests should close file descriptors which they open. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/testing/selftests/bpf/test_maps.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index f717acc0c68d..46cf2c232964 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -1401,6 +1401,8 @@ static void test_map_rdonly(void) /* Check that key=2 is not found. */ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == ENOENT); assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == ENOENT); + + close(fd); } static void test_map_wronly(void) @@ -1423,6 +1425,8 @@ static void test_map_wronly(void) /* Check that key=2 is not found. */ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == EPERM); assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == EPERM); + + close(fd); } static void prepare_reuseport_grp(int type, int map_fd, size_t map_elem_size, -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf 3/5] selftests/bpf: cleanup comments in test_maps 2020-05-27 18:56 ` [PATCH bpf 2/5] selftests/bpf: cleanup some file descriptors " Anton Protopopov @ 2020-05-27 18:56 ` Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 4/5] bpf: fix map permissions check Anton Protopopov 0 siblings, 1 reply; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:56 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest Make comments inside the test_map_rdonly and test_map_wronly tests consistent with logic. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/testing/selftests/bpf/test_maps.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index 46cf2c232964..08d63948514a 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -1394,11 +1394,11 @@ static void test_map_rdonly(void) key = 1; value = 1234; - /* Insert key=1 element. */ + /* Try to insert key=1 element. */ assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == -1 && errno == EPERM); - /* Check that key=2 is not found. */ + /* Check that key=1 is not found. */ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == ENOENT); assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == ENOENT); @@ -1422,7 +1422,7 @@ static void test_map_wronly(void) /* Insert key=1 element. */ assert(bpf_map_update_elem(fd, &key, &value, BPF_ANY) == 0); - /* Check that key=2 is not found. */ + /* Check that reading elements and keys from the map is not allowed. */ assert(bpf_map_lookup_elem(fd, &key, &value) == -1 && errno == EPERM); assert(bpf_map_get_next_key(fd, &key, &value) == -1 && errno == EPERM); -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf 4/5] bpf: fix map permissions check 2020-05-27 18:56 ` [PATCH bpf 3/5] selftests/bpf: cleanup comments " Anton Protopopov @ 2020-05-27 18:56 ` Anton Protopopov 2020-05-27 18:57 ` [PATCH bpf 5/5] selftests/bpf: add tests for write-only stacks/queues Anton Protopopov 0 siblings, 1 reply; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:56 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest The map_lookup_and_delete_elem() function should check for both FMODE_CAN_WRITE and FMODE_CAN_READ permissions because it returns a map element to user space. Fixes: bd513cd08f10 ("bpf: add MAP_LOOKUP_AND_DELETE_ELEM syscall") Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- kernel/bpf/syscall.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 4e6dee19a668..5e52765161f9 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -1468,7 +1468,8 @@ static int map_lookup_and_delete_elem(union bpf_attr *attr) map = __bpf_map_get(f); if (IS_ERR(map)) return PTR_ERR(map); - if (!(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) { + if (!(map_get_sys_perms(map, f) & FMODE_CAN_READ) || + !(map_get_sys_perms(map, f) & FMODE_CAN_WRITE)) { err = -EPERM; goto err_put; } -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH bpf 5/5] selftests/bpf: add tests for write-only stacks/queues 2020-05-27 18:56 ` [PATCH bpf 4/5] bpf: fix map permissions check Anton Protopopov @ 2020-05-27 18:57 ` Anton Protopopov 0 siblings, 0 replies; 7+ messages in thread From: Anton Protopopov @ 2020-05-27 18:57 UTC (permalink / raw) To: Alexei Starovoitov, Daniel Borkmann, Shuah Khan Cc: Anton Protopopov, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest For write-only stacks and queues bpf_map_update_elem should be allowed, but bpf_map_lookup_elem and bpf_map_lookup_and_delete_elem should fail with EPERM. Signed-off-by: Anton Protopopov <a.s.protopopov@gmail.com> --- tools/testing/selftests/bpf/test_maps.c | 40 ++++++++++++++++++++++++- 1 file changed, 39 insertions(+), 1 deletion(-) diff --git a/tools/testing/selftests/bpf/test_maps.c b/tools/testing/selftests/bpf/test_maps.c index 08d63948514a..6a12a0e01e07 100644 --- a/tools/testing/selftests/bpf/test_maps.c +++ b/tools/testing/selftests/bpf/test_maps.c @@ -1405,7 +1405,7 @@ static void test_map_rdonly(void) close(fd); } -static void test_map_wronly(void) +static void test_map_wronly_hash(void) { int fd, key = 0, value = 0; @@ -1429,6 +1429,44 @@ static void test_map_wronly(void) close(fd); } +static void test_map_wronly_stack_or_queue(enum bpf_map_type map_type) +{ + int fd, value = 0; + + assert(map_type == BPF_MAP_TYPE_QUEUE || + map_type == BPF_MAP_TYPE_STACK); + fd = bpf_create_map(map_type, 0, sizeof(value), MAP_SIZE, + map_flags | BPF_F_WRONLY); + /* Stack/Queue maps do not support BPF_F_NO_PREALLOC */ + if (map_flags & BPF_F_NO_PREALLOC) { + assert(fd < 0 && errno == EINVAL); + return; + } + if (fd < 0) { + printf("Failed to create map '%s'!\n", strerror(errno)); + exit(1); + } + + value = 1234; + assert(bpf_map_update_elem(fd, NULL, &value, BPF_ANY) == 0); + + /* Peek element should fail */ + assert(bpf_map_lookup_elem(fd, NULL, &value) == -1 && errno == EPERM); + + /* Pop element should fail */ + assert(bpf_map_lookup_and_delete_elem(fd, NULL, &value) == -1 && + errno == EPERM); + + close(fd); +} + +static void test_map_wronly(void) +{ + test_map_wronly_hash(); + test_map_wronly_stack_or_queue(BPF_MAP_TYPE_STACK); + test_map_wronly_stack_or_queue(BPF_MAP_TYPE_QUEUE); +} + static void prepare_reuseport_grp(int type, int map_fd, size_t map_elem_size, __s64 *fds64, __u64 *sk_cookies, unsigned int n) -- 2.20.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around 2020-05-27 18:56 [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 1/5] selftests/bpf: fix a typo in test_maps Anton Protopopov @ 2020-05-28 14:39 ` Daniel Borkmann 1 sibling, 0 replies; 7+ messages in thread From: Daniel Borkmann @ 2020-05-28 14:39 UTC (permalink / raw) To: Anton Protopopov Cc: Alexei Starovoitov, Shuah Khan, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev, bpf, linux-kernel, linux-kselftest On Wed, May 27, 2020 at 06:56:55PM +0000, Anton Protopopov wrote: > This series fixes a bug in the map_lookup_and_delete_elem() function which > should check for the FMODE_CAN_READ bit, because it returns data to user space. > The rest of commits fix some typos and comment in selftests and extend the > test_map_wronly test to cover the new check for the BPF_MAP_TYPE_STACK and > BPF_MAP_TYPE_QUEUE map types. > > Anton Protopopov (5): > selftests/bpf: fix a typo in test_maps > selftests/bpf: cleanup some file descriptors in test_maps > selftests/bpf: cleanup comments in test_maps > bpf: fix map permissions check > selftests/bpf: add tests for write-only stacks/queues > > kernel/bpf/syscall.c | 3 +- > tools/testing/selftests/bpf/test_maps.c | 52 ++++++++++++++++++++++--- > 2 files changed, 49 insertions(+), 6 deletions(-) Looks good to me and is also consistent with what we do for the lookup + delete batch interface, applied thanks! Fyi, I've taken it to bpf-next given 5.7 is right around the corner. We can take the permissions fix to stable once in Linus' tree. ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-05-28 14:39 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-05-27 18:56 [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 1/5] selftests/bpf: fix a typo in test_maps Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 2/5] selftests/bpf: cleanup some file descriptors " Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 3/5] selftests/bpf: cleanup comments " Anton Protopopov 2020-05-27 18:56 ` [PATCH bpf 4/5] bpf: fix map permissions check Anton Protopopov 2020-05-27 18:57 ` [PATCH bpf 5/5] selftests/bpf: add tests for write-only stacks/queues Anton Protopopov 2020-05-28 14:39 ` [PATCH bpf 0/5] bpf: fix map permissions check and cleanup code around Daniel Borkmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.