bpf: allow narrow loads of bpf_sysctl fields with offset > 0

PATCH Applied Ilya Leoshkevich 4 participants 8 messages Oct 28, 2019 – Oct 30, 2019

Subsystems (3)

Kselftest BPF BPF selftests

lore

Share & tools

lore

Ilya Leoshkevich

author

2019-10-28 12:29 UTC #0 lore raw

PATCH 1/1 bpf: allow narrow loads of bpf_sysctl fields with offset > 0

"ctx:file_pos sysctl:read read ok narrow" works on s390 by accident: it reads the wrong byte, which happens to have the expected value of 0. Improve the test by seeking to the 4th byte and expecting 4 instead of 0.

This makes the latent problem apparent: the test attempts to read the first byte of bpf_sysctl.file_pos, assuming this is the least-significant byte, which is not the case on big-endian machines: a non-zero offset is needed.

The point of the test is to verify narrow loads, so we cannot cheat our way out by simply using BPF_W. The existence of the test means that such loads have to be supported, most likely because llvm can generate them. Fix the test by adding a big-endian variant, which uses an offset to access the least-significant byte of bpf_sysctl.file_pos.

This reveals the final problem: verifier rejects accesses to bpf_sysctl fields with offset > 0. Such accesses are already allowed for a wide range of structs: __sk_buff, bpf_sock_addr and sk_msg_md to name a few. Extend this support to bpf_sysctl by using bpf_ctx_range instead of offsetof when matching field offsets.

Fixes 7b146cebe30c ("bpf: Sysctl hook")

Fixes e1550bfe0de4 ("bpf: Add file_pos field to bpf_sysctl ctx")

Fixes 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx")

Signed-off-by Ilya Leoshkevich <[email protected]>

---
 kernel/bpf/cgroup.c                       | 4 ++--
 tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

kernel/bpf/cgroup.c +2 -2

index ddd8addcdb5c..a3eaf08e7dd3 100644
@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
 		return false;
 
 	switch (off) {
-	case offsetof(struct bpf_sysctl, write):
+	case bpf_ctx_range(struct bpf_sysctl, write):
 		if (type != BPF_READ)
 			return false;
 		bpf_ctx_record_field_size(info, size_default);
 		return bpf_ctx_narrow_access_ok(off, size, size_default);
-	case offsetof(struct bpf_sysctl, file_pos):
+	case bpf_ctx_range(struct bpf_sysctl, file_pos):
 		if (type == BPF_READ) {
 			bpf_ctx_record_field_size(info, size_default);
 			return bpf_ctx_narrow_access_ok(off, size, size_default);

tools/testing/selftests/bpf/test_sysctl.c +7 -1

index a320e3844b17..7c6e5b173f33 100644
@@ -161,9 +161,14 @@ static struct sysctl_test tests[] = {
 		.descr = "ctx:file_pos sysctl:read read ok narrow",
 		.insns = {
 			/* If (file_pos == X) */
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
 				    offsetof(struct bpf_sysctl, file_pos)),
-			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 0, 2),
+#else
+			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
+				    offsetof(struct bpf_sysctl, file_pos) + 3),
+#endif
+			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 4, 2),
 
 			/* return ALLOW; */
 			BPF_MOV64_IMM(BPF_REG_0, 1),

@@ -176,6 +181,7 @@ static struct sysctl_test tests[] = {
 		.attach_type = BPF_CGROUP_SYSCTL,
 		.sysctl = "kernel/ostype",
 		.open_flags = O_RDONLY,
+		.seek = 4,
 		.result = SUCCESS,
 	},
 	{

Signature · 1 line

2.23.0

Andrey Ignatov

2019-10-29 00:00 UTC #1 lore raw

Ilya Leoshkevich <[email protected]> [Mon, 2019-10-28 05:29 -0700]:

Quoted 5 lines

"ctx:file_pos sysctl:read read ok narrow" works on s390 by accident: it reads the wrong byte, which happens to have the expected value of 0. Improve the test by seeking to the 4th byte and expecting 4 instead of 0.

Quoted 8 lines

This makes the latent problem apparent: the test attempts to read the first byte of bpf_sysctl.file_pos, assuming this is the least-significant byte, which is not the case on big-endian machines: a non-zero offset is needed.
The point of the test is to verify narrow loads, so we cannot cheat our way out by simply using BPF_W. The existence of the test means that such loads have to be supported, most likely because llvm can generate them.

Right, llvm sometimes generates narrow load even if C proram uses u32 and this is the reason to support them.

Quoted 23 lines

Fix the test by adding a big-endian variant, which uses an offset to access the least-significant byte of bpf_sysctl.file_pos.
This reveals the final problem: verifier rejects accesses to bpf_sysctl fields with offset > 0. Such accesses are already allowed for a wide range of structs: __sk_buff, bpf_sock_addr and sk_msg_md to name a few. Extend this support to bpf_sysctl by using bpf_ctx_range instead of offsetof when matching field offsets.
Fixes 7b146cebe30c ("bpf: Sysctl hook")
Fixes e1550bfe0de4 ("bpf: Add file_pos field to bpf_sysctl ctx")
Fixes 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx")
Signed-off-by Ilya Leoshkevich <[email protected]>
---
 kernel/bpf/cgroup.c                       | 4 ++--
 tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)
kernel/bpf/cgroup.c +0 -0
index ddd8addcdb5c..a3eaf08e7dd3 100644

kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type, · line 1318

Quoted 9 lines

kernel/bpf/cgroup.c +1 -1

@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
 		return false;
 
 	switch (off) {
-	case offsetof(struct bpf_sysctl, write):
+	case bpf_ctx_range(struct bpf_sysctl, write):
 		if (type != BPF_READ)
 			return false;
 		bpf_ctx_record_field_size(info, size_default);
 		return bpf_ctx_narrow_access_ok(off, size, size_default);

LGTM, but could you please add a test case for narrow load from write? From what I see all existing test cases use BPF_W.

Quoted 36 lines

kernel/bpf/cgroup.c +1 -1

-	case offsetof(struct bpf_sysctl, file_pos):
+	case bpf_ctx_range(struct bpf_sysctl, file_pos):
 		if (type == BPF_READ) {
 			bpf_ctx_record_field_size(info, size_default);
 			return bpf_ctx_narrow_access_ok(off, size, size_default);

tools/testing/selftests/bpf/test_sysctl.c +7 -1

index a320e3844b17..7c6e5b173f33 100644
@@ -161,9 +161,14 @@ static struct sysctl_test tests[] = {
 		.descr = "ctx:file_pos sysctl:read read ok narrow",
 		.insns = {
 			/* If (file_pos == X) */
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
 				    offsetof(struct bpf_sysctl, file_pos)),
-			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 0, 2),
+#else
+			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
+				    offsetof(struct bpf_sysctl, file_pos) + 3),
+#endif
+			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 4, 2),
 
 			/* return ALLOW; */
 			BPF_MOV64_IMM(BPF_REG_0, 1),

@@ -176,6 +181,7 @@ static struct sysctl_test tests[] = {
 		.attach_type = BPF_CGROUP_SYSCTL,
 		.sysctl = "kernel/ostype",
 		.open_flags = O_RDONLY,
+		.seek = 4,
 		.result = SUCCESS,
 	},
 	{

Signature · 1 line

2.23.0

Signature · 1 line

Andrey Ignatov

Andrii Nakryiko

2019-10-29 04:37 UTC #2 lore raw

In reply to Ilya Leoshkevich

Quoted 37 lines · Ilya Leoshkevich

"ctx:file_pos sysctl:read read ok narrow" works on s390 by accident: it reads the wrong byte, which happens to have the expected value of 0. Improve the test by seeking to the 4th byte and expecting 4 instead of 0.
This makes the latent problem apparent: the test attempts to read the first byte of bpf_sysctl.file_pos, assuming this is the least-significant byte, which is not the case on big-endian machines: a non-zero offset is needed.
The point of the test is to verify narrow loads, so we cannot cheat our way out by simply using BPF_W. The existence of the test means that such loads have to be supported, most likely because llvm can generate them. Fix the test by adding a big-endian variant, which uses an offset to access the least-significant byte of bpf_sysctl.file_pos.
This reveals the final problem: verifier rejects accesses to bpf_sysctl fields with offset > 0. Such accesses are already allowed for a wide range of structs: __sk_buff, bpf_sock_addr and sk_msg_md to name a few. Extend this support to bpf_sysctl by using bpf_ctx_range instead of offsetof when matching field offsets.
Fixes 7b146cebe30c ("bpf: Sysctl hook")
Fixes e1550bfe0de4 ("bpf: Add file_pos field to bpf_sysctl ctx")
Fixes 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx")
Signed-off-by Ilya Leoshkevich <[email protected]>
---
 kernel/bpf/cgroup.c                       | 4 ++--
 tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)
kernel/bpf/cgroup.c +0 -0
index ddd8addcdb5c..a3eaf08e7dd3 100644

kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type, · line 1314

Quoted 5 lines

kernel/bpf/cgroup.c +1 -1

@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
                return false;
 
        switch (off) {
-       case offsetof(struct bpf_sysctl, write):
+       case bpf_ctx_range(struct bpf_sysctl, write):

this will actually allow reads pas t write field (e.g., offset = 2, size = 4).

kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,

Quoted 6 lines

kernel/bpf/cgroup.c +1 -1

                if (type != BPF_READ)
                        return false;
                bpf_ctx_record_field_size(info, size_default);
                return bpf_ctx_narrow_access_ok(off, size, size_default);
-       case offsetof(struct bpf_sysctl, file_pos):
+       case bpf_ctx_range(struct bpf_sysctl, file_pos)

this will allow read past context struct altogether. When we allow ranges, we will have to adjust allowed read size.

Quoted 34 lines

kernel/bpf/cgroup.c +0 -0

                if (type == BPF_READ) {
                        bpf_ctx_record_field_size(info, size_default);
                        return bpf_ctx_narrow_access_ok(off, size, size_default);

tools/testing/selftests/bpf/test_sysctl.c +7 -2

index a320e3844b17..7c6e5b173f33 100644
@@ -161,9 +161,14 @@ static struct sysctl_test tests[] = {
                .descr = "ctx:file_pos sysctl:read read ok narrow",
                .insns = {
                        /* If (file_pos == X) */
+#if __BYTE_ORDER == __LITTLE_ENDIAN
                        BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
                                    offsetof(struct bpf_sysctl, file_pos)),
-                       BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 0, 2),
+#else
+                       BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
+                                   offsetof(struct bpf_sysctl, file_pos) + 3),
+#endif
+                       BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 4, 2),
 
                        /* return ALLOW; */
                        BPF_MOV64_IMM(BPF_REG_0, 1),

@@ -176,6 +181,7 @@ static struct sysctl_test tests[] = {
                .attach_type = BPF_CGROUP_SYSCTL,
                .sysctl = "kernel/ostype",
                .open_flags = O_RDONLY,
+               .seek = 4,
                .result = SUCCESS,
        },
        {
--

2.23.0

Ilya Leoshkevich

author

2019-10-29 14:20 UTC #3 lore raw

Quoted 9 lines

Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <[email protected]>:
Quoted 6 lines · Ilya Leoshkevich
kernel/bpf/cgroup.c +0 -0
@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
               return false;
 

Quoted 5 lines

kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type, · line 1314
Quoted 3 lines
kernel/bpf/cgroup.c +1 -1
       switch (off) {
-       case offsetof(struct bpf_sysctl, write):
+       case bpf_ctx_range(struct bpf_sysctl, write):
this will actually allow reads pas t write field (e.g., offset = 2, size = 4).

Wouldn't

	if (off < 0 || off + size > sizeof(struct bpf_sysctl) || off % size)
		return false;

prevent all OOB read-write attempts? Especially the off % size part - I think it has the effect of preventing OOB accesses for fields. In particular, it would filter offset = 2, size = 4 case.

I have also checked the other usages of bpf_ctx_range, for example, bpf_skb_is_valid_access, and they don't seem to be doing anything special.

Quoted 10 lines

kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
Quoted 7 lines
kernel/bpf/cgroup.c +1 -1
               if (type != BPF_READ)
                       return false;
               bpf_ctx_record_field_size(info, size_default);
               return bpf_ctx_narrow_access_ok(off, size, size_default);
-       case offsetof(struct bpf_sysctl, file_pos):
+       case bpf_ctx_range(struct bpf_sysctl, file_pos)
this will allow read past context struct altogether. When we allow ranges, we will have to adjust allowed read size.

Same here.

Andrey Ignatov

2019-10-29 15:17 UTC #4 lore raw

Ilya Leoshkevich <[email protected]> [Tue, 2019-10-29 07:20 -0700]:

Quoted 15 lines

Quoted 9 lines
Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <[email protected]>:
Quoted 6 lines · Ilya Leoshkevich
kernel/bpf/cgroup.c +0 -0
@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
               return false;
 
Quoted 5 lines
kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type, · line 1314
Quoted 3 lines
kernel/bpf/cgroup.c +1 -1
       switch (off) {
-       case offsetof(struct bpf_sysctl, write):
+       case bpf_ctx_range(struct bpf_sysctl, write):
this will actually allow reads pas t write field (e.g., offset = 2, size = 4).

Quoted 8 lines

Wouldn't
	if (off < 0 || off + size > sizeof(struct bpf_sysctl) || off % size)
		return false;
prevent all OOB read-write attempts? Especially the off % size part - I think it has the effect of preventing OOB accesses for fields. In particular, it would filter offset = 2, size = 4 case.

Yes, it would. This code makes sure that narrow accesses are aligned so that offset = 2 would allow only size = 2 or size = 1.

Quoted 3 lines

I have also checked the other usages of bpf_ctx_range, for example, bpf_skb_is_valid_access, and they don't seem to be doing anything special.

Yes, sysctl hook follows logic similar to that of other program types.

Quoted 11 lines

Quoted 9 lines
kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
Quoted 6 lines
kernel/bpf/cgroup.c +1 -1
               if (type != BPF_READ)
                       return false;
               bpf_ctx_record_field_size(info, size_default);
               return bpf_ctx_narrow_access_ok(off, size, size_default);
-       case offsetof(struct bpf_sysctl, file_pos):
+       case bpf_ctx_range(struct bpf_sysctl, file_pos)
this will allow read past context struct altogether. When we allow ranges, we will have to adjust allowed read size.
Same here.

Signature · 1 line

Andrey Ignatov

Andrii Nakryiko

Acked-by

2019-10-29 17:40 UTC #5 lore raw

Quoted 22 lines · Andrey Ignatov

Ilya Leoshkevich <[email protected]> [Tue, 2019-10-29 07:20 -0700]:
Quoted 20 lines
Quoted 9 lines
Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <[email protected]>:
Quoted 6 lines · Ilya Leoshkevich
kernel/bpf/cgroup.c +0 -0
@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
               return false;
 
Quoted 5 lines
kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type, · line 1314
Quoted 3 lines
kernel/bpf/cgroup.c +1 -1
       switch (off) {
-       case offsetof(struct bpf_sysctl, write):
+       case bpf_ctx_range(struct bpf_sysctl, write):
this will actually allow reads pas t write field (e.g., offset = 2, size = 4).
Wouldn't
      if (off < 0 || off + size > sizeof(struct bpf_sysctl) || off % size)
              return false;

Quoted 6 lines

Quoted 3 lines
prevent all OOB read-write attempts? Especially the off % size part - I think it has the effect of preventing OOB accesses for fields. In particular, it would filter offset = 2, size = 4 case.
Yes, it would. This code makes sure that narrow accesses are aligned so that offset = 2 would allow only size = 2 or size = 1.

Yes, you both are right, I missed the "off % size" check above. Thanks. Looks good to me as well.

Acked-by Andrii Nakryiko <[email protected]>

Quoted 21 lines

Quoted 4 lines
I have also checked the other usages of bpf_ctx_range, for example, bpf_skb_is_valid_access, and they don't seem to be doing anything special.
Yes, sysctl hook follows logic similar to that of other program types.
Quoted 11 lines
Quoted 9 lines
kernel/bpf/cgroup.c static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
Quoted 6 lines
kernel/bpf/cgroup.c +1 -1
               if (type != BPF_READ)
                       return false;
               bpf_ctx_record_field_size(info, size_default);
               return bpf_ctx_narrow_access_ok(off, size, size_default);
-       case offsetof(struct bpf_sysctl, file_pos):
+       case bpf_ctx_range(struct bpf_sysctl, file_pos)
this will allow read past context struct altogether. When we allow ranges, we will have to adjust allowed read size.
Same here.
--
Andrey Ignatov

Alexei Starovoitov

Applied

2019-10-30 19:54 UTC #6 lore raw

Applied to bpf tree. Thanks

Andrey Ignatov

Acked-by on v1 1/1

2019-10-29 15:19 UTC #7 lore raw

In reply to Ilya Leoshkevich

Ilya Leoshkevich <[email protected]> [Mon, 2019-10-28 05:29 -0700]:

Quoted 22 lines

"ctx:file_pos sysctl:read read ok narrow" works on s390 by accident: it reads the wrong byte, which happens to have the expected value of 0. Improve the test by seeking to the 4th byte and expecting 4 instead of 0.
This makes the latent problem apparent: the test attempts to read the first byte of bpf_sysctl.file_pos, assuming this is the least-significant byte, which is not the case on big-endian machines: a non-zero offset is needed.
The point of the test is to verify narrow loads, so we cannot cheat our way out by simply using BPF_W. The existence of the test means that such loads have to be supported, most likely because llvm can generate them. Fix the test by adding a big-endian variant, which uses an offset to access the least-significant byte of bpf_sysctl.file_pos.
This reveals the final problem: verifier rejects accesses to bpf_sysctl fields with offset > 0. Such accesses are already allowed for a wide range of structs: __sk_buff, bpf_sock_addr and sk_msg_md to name a few. Extend this support to bpf_sysctl by using bpf_ctx_range instead of offsetof when matching field offsets.

Quoted 4 lines

Fixes 7b146cebe30c ("bpf: Sysctl hook")
Fixes e1550bfe0de4 ("bpf: Add file_pos field to bpf_sysctl ctx")
Fixes 9a1027e52535 ("selftests/bpf: Test file_pos field in bpf_sysctl ctx")
Signed-off-by Ilya Leoshkevich <[email protected]>

Thanks for following up with the test case and for the bugfix itself!

Acked-by Andrey Ignatov <[email protected]>

Quoted 55 lines

---
 kernel/bpf/cgroup.c                       | 4 ++--
 tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

kernel/bpf/cgroup.c +2 -2

index ddd8addcdb5c..a3eaf08e7dd3 100644
@@ -1311,12 +1311,12 @@ static bool sysctl_is_valid_access(int off, int size, enum bpf_access_type type,
 		return false;
 
 	switch (off) {
-	case offsetof(struct bpf_sysctl, write):
+	case bpf_ctx_range(struct bpf_sysctl, write):
 		if (type != BPF_READ)
 			return false;
 		bpf_ctx_record_field_size(info, size_default);
 		return bpf_ctx_narrow_access_ok(off, size, size_default);
-	case offsetof(struct bpf_sysctl, file_pos):
+	case bpf_ctx_range(struct bpf_sysctl, file_pos):
 		if (type == BPF_READ) {
 			bpf_ctx_record_field_size(info, size_default);
 			return bpf_ctx_narrow_access_ok(off, size, size_default);

tools/testing/selftests/bpf/test_sysctl.c +7 -1

index a320e3844b17..7c6e5b173f33 100644
@@ -161,9 +161,14 @@ static struct sysctl_test tests[] = {
 		.descr = "ctx:file_pos sysctl:read read ok narrow",
 		.insns = {
 			/* If (file_pos == X) */
+#if __BYTE_ORDER == __LITTLE_ENDIAN
 			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
 				    offsetof(struct bpf_sysctl, file_pos)),
-			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 0, 2),
+#else
+			BPF_LDX_MEM(BPF_B, BPF_REG_7, BPF_REG_1,
+				    offsetof(struct bpf_sysctl, file_pos) + 3),
+#endif
+			BPF_JMP_IMM(BPF_JNE, BPF_REG_7, 4, 2),
 
 			/* return ALLOW; */
 			BPF_MOV64_IMM(BPF_REG_0, 1),

@@ -176,6 +181,7 @@ static struct sysctl_test tests[] = {
 		.attach_type = BPF_CGROUP_SYSCTL,
 		.sysctl = "kernel/ostype",
 		.open_flags = O_RDONLY,
+		.seek = 4,
 		.result = SUCCESS,
 	},
 	{

Signature · 1 line

2.23.0

Signature · 1 line

Andrey Ignatov