* [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).