bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
@ 2019-10-28 12:29 Ilya Leoshkevich
  2019-10-28 23:59 ` Andrey Ignatov
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Ilya Leoshkevich @ 2019-10-28 12:29 UTC (permalink / raw)
  To: Daniel Borkmann, Alexei Starovoitov
  Cc: bpf, Heiko Carstens, Vasily Gorbik, Andrey Ignatov, 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 <iii@linux.ibm.com>
---
 kernel/bpf/cgroup.c                       | 4 ++--
 tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
index ddd8addcdb5c..a3eaf08e7dd3 100644
--- a/kernel/bpf/cgroup.c
+++ b/kernel/bpf/cgroup.c
@@ -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);
diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
index a320e3844b17..7c6e5b173f33 100644
--- a/tools/testing/selftests/bpf/test_sysctl.c
+++ b/tools/testing/selftests/bpf/test_sysctl.c
@@ -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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-28 12:29 [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0 Ilya Leoshkevich
@ 2019-10-28 23:59 ` Andrey Ignatov
  2019-10-29  4:36 ` Andrii Nakryiko
  2019-10-29 15:19 ` Andrey Ignatov
  2 siblings, 0 replies; 8+ messages in thread
From: Andrey Ignatov @ 2019-10-28 23:59 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Heiko Carstens, Vasily Gorbik

Ilya Leoshkevich <iii@linux.ibm.com> [Mon, 2019-10-28 05:29 -0700]:
> "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.

Right, llvm sometimes generates narrow load even if C proram uses u32 and this
is the reason to support 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 <iii@linux.ibm.com>
> ---
>  kernel/bpf/cgroup.c                       | 4 ++--
>  tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ddd8addcdb5c..a3eaf08e7dd3 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -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.

> -	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);
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index a320e3844b17..7c6e5b173f33 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/test_sysctl.c
> @@ -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
> 

-- 
Andrey Ignatov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-28 12:29 [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0 Ilya Leoshkevich
  2019-10-28 23:59 ` Andrey Ignatov
@ 2019-10-29  4:36 ` Andrii Nakryiko
  2019-10-29 14:19   ` Ilya Leoshkevich
  2019-10-29 15:19 ` Andrey Ignatov
  2 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2019-10-29  4:36 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Heiko Carstens,
	Vasily Gorbik, Andrey Ignatov

On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> "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 <iii@linux.ibm.com>
> ---
>  kernel/bpf/cgroup.c                       | 4 ++--
>  tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ddd8addcdb5c..a3eaf08e7dd3 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -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).

>                 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.

>                 if (type == BPF_READ) {
>                         bpf_ctx_record_field_size(info, size_default);
>                         return bpf_ctx_narrow_access_ok(off, size, size_default);
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index a320e3844b17..7c6e5b173f33 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/test_sysctl.c
> @@ -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
>

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-29  4:36 ` Andrii Nakryiko
@ 2019-10-29 14:19   ` Ilya Leoshkevich
  2019-10-29 15:16     ` Andrey Ignatov
  0 siblings, 1 reply; 8+ messages in thread
From: Ilya Leoshkevich @ 2019-10-29 14:19 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Heiko Carstens,
	Vasily Gorbik, Andrey Ignatov

> Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> 
> On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>> 
>> --- a/kernel/bpf/cgroup.c
>> +++ b/kernel/bpf/cgroup.c
>> @@ -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).

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.

> 
>>                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.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-29 14:19   ` Ilya Leoshkevich
@ 2019-10-29 15:16     ` Andrey Ignatov
  2019-10-29 17:39       ` Andrii Nakryiko
  0 siblings, 1 reply; 8+ messages in thread
From: Andrey Ignatov @ 2019-10-29 15:16 UTC (permalink / raw)
  To: Ilya Leoshkevich, Andrii Nakryiko
  Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Heiko Carstens, Vasily Gorbik

Ilya Leoshkevich <iii@linux.ibm.com> [Tue, 2019-10-29 07:20 -0700]:
> > Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> > 
> > On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> >> 
> >> --- a/kernel/bpf/cgroup.c
> >> +++ b/kernel/bpf/cgroup.c
> >> @@ -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).
> 
> 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.

> 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.

> >>                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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-28 12:29 [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0 Ilya Leoshkevich
  2019-10-28 23:59 ` Andrey Ignatov
  2019-10-29  4:36 ` Andrii Nakryiko
@ 2019-10-29 15:19 ` Andrey Ignatov
  2 siblings, 0 replies; 8+ messages in thread
From: Andrey Ignatov @ 2019-10-29 15:19 UTC (permalink / raw)
  To: Ilya Leoshkevich
  Cc: Daniel Borkmann, Alexei Starovoitov, bpf, Heiko Carstens, Vasily Gorbik

Ilya Leoshkevich <iii@linux.ibm.com> [Mon, 2019-10-28 05:29 -0700]:
> "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 <iii@linux.ibm.com>

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

Acked-by: Andrey Ignatov <rdna@fb.com>

> ---
>  kernel/bpf/cgroup.c                       | 4 ++--
>  tools/testing/selftests/bpf/test_sysctl.c | 8 +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index ddd8addcdb5c..a3eaf08e7dd3 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -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);
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index a320e3844b17..7c6e5b173f33 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/test_sysctl.c
> @@ -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
> 

-- 
Andrey Ignatov

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-29 15:16     ` Andrey Ignatov
@ 2019-10-29 17:39       ` Andrii Nakryiko
  2019-10-30 19:54         ` Alexei Starovoitov
  0 siblings, 1 reply; 8+ messages in thread
From: Andrii Nakryiko @ 2019-10-29 17:39 UTC (permalink / raw)
  To: Andrey Ignatov
  Cc: Ilya Leoshkevich, Daniel Borkmann, Alexei Starovoitov, bpf,
	Heiko Carstens, Vasily Gorbik

On Tue, Oct 29, 2019 at 8:16 AM Andrey Ignatov <rdna@fb.com> wrote:
>
> Ilya Leoshkevich <iii@linux.ibm.com> [Tue, 2019-10-29 07:20 -0700]:
> > > Am 29.10.2019 um 05:36 schrieb Andrii Nakryiko <andrii.nakryiko@gmail.com>:
> > >
> > > On Mon, Oct 28, 2019 at 1:09 PM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
> > >>
> > >> --- a/kernel/bpf/cgroup.c
> > >> +++ b/kernel/bpf/cgroup.c
> > >> @@ -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).
> >
> > 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.

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

Acked-by: Andrii Nakryiko <andriin@fb.com>

>
> > 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.
>
> > >>                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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0
  2019-10-29 17:39       ` Andrii Nakryiko
@ 2019-10-30 19:54         ` Alexei Starovoitov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexei Starovoitov @ 2019-10-30 19:54 UTC (permalink / raw)
  To: Andrii Nakryiko
  Cc: Andrey Ignatov, Ilya Leoshkevich, Daniel Borkmann,
	Alexei Starovoitov, bpf, Heiko Carstens, Vasily Gorbik

Applied to bpf tree. Thanks

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-10-30 19:54 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 12:29 [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0 Ilya Leoshkevich
2019-10-28 23:59 ` Andrey Ignatov
2019-10-29  4:36 ` Andrii Nakryiko
2019-10-29 14:19   ` Ilya Leoshkevich
2019-10-29 15:16     ` Andrey Ignatov
2019-10-29 17:39       ` Andrii Nakryiko
2019-10-30 19:54         ` Alexei Starovoitov
2019-10-29 15:19 ` Andrey Ignatov

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