netdev - Re: [PATCH net-next] net: sysctl data could be in .bss

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 22 Oct 2021 13:01:46 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     davem@...emloft.net, jonathon.reinhart@...il.com,
        netdev@...r.kernel.org, tglx@...utronix.de, peterz@...radead.org,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH net-next] net: sysctl data could be in .bss

Widening the CC list a little.

On Wed, 20 Oct 2021 10:38:54 +0200 Antoine Tenart wrote:
> A check is made when registering non-init netns sysctl files to ensure
> their data pointer does not point to a global data section. This works
> well for modules as the check is made against the whole module address
> space (is_module_address). But when built-in, the check is made against
> the .data section. However global variables initialized to 0 can be in
> .bss (-fzero-initialized-in-bss).
> 
> Add an extra check to make sure the sysctl data does not point to the
> .bss section either.
> 
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> Reviewed-by: Jonathon Reinhart <jonathon.reinhart@...il.com>
> ---
> Hello,
> 
> This was previously sent as an RFC[1] waiting for a problematic sysctl
> to be fixed. The fix was accepted and is now in the nf tree[2].
> 
> This is not sent as a fix to avoid possible new warnings in stable
> kernels. (The actual fixes of sysctl files should go).
> 
> I think this can go through the net-next tree as kernel/extable.c
> doesn't seem to be under any subsystem and a conflict is unlikely to
> happen.

> [1] https://lore.kernel.org/all/20211012155542.827631-1-atenart@kernel.org/T/
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf.git/commit/?id=174c376278949c44aad89c514a6b5db6cee8db59
> 
>  include/linux/kernel.h | 1 +
>  kernel/extable.c       | 8 ++++++++
>  net/sysctl_net.c       | 2 +-
>  3 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index 2776423a587e..beb61d0ab220 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -231,6 +231,7 @@ extern char *next_arg(char *args, char **param, char **val);
>  extern int core_kernel_text(unsigned long addr);
>  extern int init_kernel_text(unsigned long addr);
>  extern int core_kernel_data(unsigned long addr);
> +extern int core_kernel_bss(unsigned long addr);

Is the intention of these helpers to have strict section name semantics
or higher level "is this global kernel data" semantics? If it's the
latter we could make core_kernel_data() check bss instead, chances are
all callers will either want that or not care. Steven?

>  extern int __kernel_text_address(unsigned long addr);
>  extern int kernel_text_address(unsigned long addr);
>  extern int func_ptr_is_kernel_text(void *ptr);
> diff --git a/kernel/extable.c b/kernel/extable.c
> index b0ea5eb0c3b4..477a4b6c8f63 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -100,6 +100,14 @@ int core_kernel_data(unsigned long addr)
>  	return 0;
>  }
>  
> +int core_kernel_bss(unsigned long addr)
> +{
> +	if (addr >= (unsigned long)__bss_start &&
> +	    addr < (unsigned long)__bss_stop)
> +		return 1;
> +	return 0;
> +}
> +
>  int __kernel_text_address(unsigned long addr)
>  {
>  	if (kernel_text_address(addr))
> diff --git a/net/sysctl_net.c b/net/sysctl_net.c
> index f6cb0d4d114c..d883cf65029f 100644
> --- a/net/sysctl_net.c
> +++ b/net/sysctl_net.c
> @@ -144,7 +144,7 @@ static void ensure_safe_net_sysctl(struct net *net, const char *path,
>  		addr = (unsigned long)ent->data;
>  		if (is_module_address(addr))
>  			where = "module";
> -		else if (core_kernel_data(addr))
> +		else if (core_kernel_data(addr) || core_kernel_bss(addr))
>  			where = "kernel";
>  		else
>  			continue;