* [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 a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).