Re: [PATCH bpf] bpf: allow narrow loads of bpf_sysctl fields with offset > 0

[Andrey Ignatov <rdna@fb.com>]

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