Re: [PATCH bpf-next 1/1] Fix for bpf_sysctl_set_new_value

[Martin KaFai Lau <martin.lau@linux.dev>]

On 5/4/24 3:23 AM, Raman Shukhau wrote:
> Noticed that call to bpf_sysctl_set_new_value doesn't change final value
> of the parameter, when called from cgroup/syscall bpf handler. No error
> thrown in this case, new value is simply ignored and original value, sent
> to sysctl, is set. Example (see test added to this change for BPF handler
> logic):
> 
> sysctl -w net.ipv4.ip_local_reserved_ports = 11111
> ... cgroup/syscal handler call bpf_sysctl_set_new_value	and set 22222
> sysctl net.ipv4.ip_local_reserved_ports
> ... returns 11111
> 
> On investigation I found 2 things that needs to be changed:
> * return value check
> * new_len provided by bpf back to sysctl. proc_sys_call_handler	expects
>    this value NOT to include \0 symbol, e.g. if user do

Thanks for the report and the patch.

This patch is changing a few things (1 fix, 1 improvement, 1 test).

Separate these individual changes into its own patch. Patch 1 fixes the return 
value. Patch 2 improves the '\0' and *pcount situation. Patch 3 adds the test.

btw, I am curious what is missed in the test_sysctl.c that didn't catch the 
return value case?

> 
> 	```
>    open("/proc/sys/net/ipv4/ip_local_reserved_ports", ...)
>    write(fd, "11111", sizeof("22222"))
>    ```
> 
>    or `echo -n "11111" > /proc/sys/net/ipv4/ip_local_reserved_ports`
> 
>    or `sysctl -w	net.ipv4.ip_local_reserved_ports=11111
> 
>    proc_sys_call_handler receives count equal to `5`. To make it consistent
>    with bpf_sysctl_set_new_value, this change also adjust `new_len` with
>    `-1`, if `\0` passed as last character. Alternatively, using
>    `sizeof("11111") - 1` in BPF handler should work, but it might not be
>    obvious and spark confusion. Note: if incorrect count is used, sysctl
>    returns EINVAL to the user.
> 
> Signed-off-by: Raman Shukhau <ramasha@fb.com>
> ---
>   kernel/bpf/cgroup.c                           |  7 ++-
>   .../bpf/progs/test_sysctl_overwrite.c         | 47 +++++++++++++++++++
>   tools/testing/selftests/bpf/test_sysctl.c     | 35 +++++++++++++-
>   3 files changed, 85 insertions(+), 4 deletions(-)
>   create mode 100644 tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> 
> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c
> index 8ba73042a239..23736aed1b53 100644
> --- a/kernel/bpf/cgroup.c
> +++ b/kernel/bpf/cgroup.c
> @@ -1739,10 +1739,13 @@ int __cgroup_bpf_run_filter_sysctl(struct ctl_table_header *head,
>   
>   	kfree(ctx.cur_val);
>   
> -	if (ret == 1 && ctx.new_updated) {
> +	if (ret == 0 && ctx.new_updated) {
>   		kfree(*buf);
>   		*buf = ctx.new_val;
> -		*pcount = ctx.new_len;
> +		if (!(*buf)[ctx.new_len])
> +			*pcount = ctx.new_len - 1;

 From looking at how new_updated is set, my understanding is new_len cannot be 0 
here. just want to double check.


> +		else
> +			*pcount = ctx.new_len;
>   	} else {
>   		kfree(ctx.new_val);
>   	}
> diff --git a/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> new file mode 100644
> index 000000000000..e44b429fcfc1
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/test_sysctl_overwrite.c
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2019 Facebook
> +
> +#include <string.h>
> +
> +#include <linux/bpf.h>
> +
> +#include <bpf/bpf_helpers.h>
> +
> +#include "bpf_compiler.h"
> +
> +static const char sysctl_value[] = "31337";
> +static const char sysctl_name[] = "net/ipv4/ip_local_reserved_ports";
> +static __always_inline int is_expected_name(struct bpf_sysctl *ctx)
> +{
> +	unsigned char i;
> +	char name[sizeof(sysctl_name)];
> +	int ret;
> +
> +	memset(name, 0, sizeof(name));
> +	ret = bpf_sysctl_get_name(ctx, name, sizeof(name), 0);
> +	if (ret < 0 || ret != sizeof(sysctl_name) - 1)
> +		return 0;
> +
> +	__pragma_loop_unroll_full
> +	for (i = 0; i < sizeof(sysctl_name); ++i)
> +		if (name[i] != sysctl_name[i])

bpf_strncmp() should be useful here.

> +			return 0;
> +
> +	return 1;
> +}
> +
> +SEC("cgroup/sysctl")
> +int test_value_overwrite(struct bpf_sysctl *ctx)
> +{
> +	if (!ctx->write)
> +		return 1;
> +
> +	if (!is_expected_name(ctx))
> +		return 0;
> +
> +	if (bpf_sysctl_set_new_value(ctx, sysctl_value, sizeof(sysctl_value)) == 0)
> +		return 1;
> +	return 0;
> +}
> +
> +char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/bpf/test_sysctl.c b/tools/testing/selftests/bpf/test_sysctl.c
> index bcdbd27f22f0..dfa479861d3a 100644
> --- a/tools/testing/selftests/bpf/test_sysctl.c
> +++ b/tools/testing/selftests/bpf/test_sysctl.c
> @@ -35,6 +35,7 @@ struct sysctl_test {
>   	int seek;
>   	const char *newval;
>   	const char *oldval;
> +	const char *updval;
>   	enum {
>   		LOAD_REJECT,
>   		ATTACH_REJECT,
> @@ -1395,6 +1396,16 @@ static struct sysctl_test tests[] = {
>   		.open_flags = O_RDONLY,
>   		.result = SUCCESS,
>   	},
> +	{
> +		"C prog: override write to ip_local_reserved_ports",
> +		.prog_file = "./test_sysctl_overwrite.bpf.o",

test_sysctl.c is not run in bpf CI. It is not very useful to extend this test 
further. Lets take this chance to create a new progs/cgrp_sysctl.c test that 
will be exercised by ./test_progs in bpf CI. Then it can use the newer skel 
open_and_load also.

Not asking to to migrate the existing tests in test_sysctl.c to the new 
progs/cgrp_sysctl.c in this patch set. The new cgrp_sysctl.c can only have the 
tests that exercise the changes in this patch set. However, it will be useful if 
progs/cgrp_sysctl.c can be bootstrapped in a way that the future test_sysctl.c 
migration will be easier. I also wouldn't worry too much on the existing raw 
insns tests in test_sysctl.c for now. They will need to be moved to either C or 
bpf asm in the future.

pw-bot: cr

> +		.attach_type = BPF_CGROUP_SYSCTL,
> +		.sysctl = "net/ipv4/ip_local_reserved_ports",
> +		.open_flags = O_RDWR,
> +		.newval = "11111",
> +		.updval = "31337",
> +		.result = SUCCESS,
> +	},
>   };
>   
>   static size_t probe_prog_length(const struct bpf_insn *fp)
> @@ -1520,13 +1531,33 @@ static int access_sysctl(const char *sysctl_path,
>   			log_err("Read value %s != %s", buf, test->oldval);
>   			goto err;
>   		}
> -	} else if (test->open_flags == O_WRONLY) {
> +	} else if (test->open_flags == O_WRONLY || test->open_flags == O_RDWR) {
>   		if (!test->newval) {
>   			log_err("New value for sysctl is not set");
>   			goto err;
>   		}
> -		if (write(fd, test->newval, strlen(test->newval)) == -1)
> +		if (write(fd, test->newval, strlen(test->newval)) == -1) {
> +			log_err("Unable to write sysctl value");
>   			goto err;
> +		}
> +		if (test->open_flags == O_RDWR) {
> +			char buf[128];
> +
> +			if (!test->updval) {
> +				log_err("Expected value for sysctl is not set");
> +				goto err;
> +			}
> +
> +			lseek(fd, 0, SEEK_SET);
> +			if (read(fd, buf, sizeof(buf)) == -1) {
> +				log_err("Unable to read updated value");
> +				goto err;
> +			}
> +			if (strncmp(buf, test->updval, strlen(test->updval))) {
> +				log_err("Overwritten value %s != %s", buf, test->updval);
> +				goto err;
> +			}
> +		}
>   	} else {
>   		log_err("Unexpected sysctl access: neither read nor write");
>   		goto err;