Re: [PATCH v2] sysctl: move the "cad_pid" entry from pid_table[] to kern_reboot_table[]

[Oleg Nesterov <oleg@redhat.com>]

OK, sashiko correctly warns about pre-existing problem

	https://sashiko.dev/#/patchset/al4C572uhLdBvyzH%40redhat.com

but this is exactly what the patches from Cen should fix.

Oleg.

On 07/20, Oleg Nesterov wrote:
>
> cad_pid is global, and kill_cad_pid() is only used in the root namespace.
>
> However, due to pid_table_root_permissions(), a non-root user can unshare
> pid/user namespaces and modify it from the child namespace. This makes no
> sense and is simply wrong.
>
> Move it to kern_reboot_table[] where it logically belongs; this ensures
> that only GLOBAL_ROOT_UID can read/modify this sysctl.
>
> Note that this patch doesn't preserve "#ifdef CONFIG_PROC_SYSCTL" around
> the "cad_pid"; CONFIG_PROC_SYSCTL selects CONFIG_SYSCTL, so it is always
> set when kern_reboot_table[] is compiled.
>
> Cc: stable@vger.kernel.org
> Fixes: e054bcbe7e7a ("sysctl: move cad_pid into kernel/pid.c")
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> Acked-by: Alexey Gladkov <legion@kernel.org>
> Reviewed-by: Bradley Morgan <include@grrlz.net>
> Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  kernel/pid.c    | 31 -------------------------------
>  kernel/reboot.c | 29 +++++++++++++++++++++++++++++
>  2 files changed, 29 insertions(+), 31 deletions(-)
>
> diff --git a/kernel/pid.c b/kernel/pid.c
> index f55189a3d07d..1c27e63fa0ad 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -764,29 +764,6 @@ static struct ctl_table_root pid_table_root = {
>  	.set_ownership	= pid_table_root_set_ownership,
>  };
>
> -static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffer,
> -		size_t *lenp, loff_t *ppos)
> -{
> -	struct pid *new_pid;
> -	pid_t tmp_pid;
> -	int r;
> -	struct ctl_table tmp_table = *table;
> -
> -	tmp_pid = pid_vnr(cad_pid);
> -	tmp_table.data = &tmp_pid;
> -
> -	r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
> -	if (r || !write)
> -		return r;
> -
> -	new_pid = find_get_pid(tmp_pid);
> -	if (!new_pid)
> -		return -ESRCH;
> -
> -	put_pid(xchg(&cad_pid, new_pid));
> -	return 0;
> -}
> -
>  static const struct ctl_table pid_table[] = {
>  	{
>  		.procname	= "pid_max",
> @@ -797,14 +774,6 @@ static const struct ctl_table pid_table[] = {
>  		.extra1		= &pid_max_min,
>  		.extra2		= &pid_max_max,
>  	},
> -#ifdef CONFIG_PROC_SYSCTL
> -	{
> -		.procname	= "cad_pid",
> -		.maxlen		= sizeof(int),
> -		.mode		= 0600,
> -		.proc_handler	= proc_do_cad_pid,
> -	},
> -#endif
>  };
>  #endif
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index 695c33e75efd..f070c5c1103a 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -1366,6 +1366,29 @@ static struct attribute *reboot_attrs[] = {
>  };
>
>  #ifdef CONFIG_SYSCTL
> +static int proc_do_cad_pid(const struct ctl_table *table, int write, void *buffer,
> +			   size_t *lenp, loff_t *ppos)
> +{
> +	struct ctl_table tmp_table = *table;
> +	struct pid *new_pid;
> +	pid_t tmp_pid;
> +	int r;
> +
> +	tmp_pid = pid_vnr(cad_pid);
> +	tmp_table.data = &tmp_pid;
> +
> +	r = proc_dointvec(&tmp_table, write, buffer, lenp, ppos);
> +	if (r || !write)
> +		return r;
> +
> +	new_pid = find_get_pid(tmp_pid);
> +	if (!new_pid)
> +		return -ESRCH;
> +
> +	put_pid(xchg(&cad_pid, new_pid));
> +	return 0;
> +}
> +
>  static const struct ctl_table kern_reboot_table[] = {
>  	{
>  		.procname       = "poweroff_cmd",
> @@ -1381,6 +1404,12 @@ static const struct ctl_table kern_reboot_table[] = {
>  		.mode           = 0644,
>  		.proc_handler   = proc_dointvec,
>  	},
> +	{
> +		.procname	= "cad_pid",
> +		.maxlen		= sizeof(int),
> +		.mode		= 0600,
> +		.proc_handler	= proc_do_cad_pid,
> +	},
>  };
>
>  static void __init kernel_reboot_sysctls_init(void)
> --
> 2.52.0
>