[RFC net-next] net: sysctl data could be in .bss

[RFC net-next] net: sysctl data could be in .bss

[Antoine Tenart]

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@kernel.org>
---

Hello,

This is sent as an RFC as I'd like a fix[1] to be merged before to
avoid introducing a new warning. But this can be reviewed in the
meantime.

I'm not sending this 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.

Thanks!
Antoine

[1] https://lore.kernel.org/all/20211012145437.754391-1-atenart@kernel.org/T/

 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);
 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;
-- 
2.31.1

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

[Jonathon Reinhart]

On Tue, Oct 12, 2021 at 11:55 AM Antoine Tenart <atenart@kernel.org> 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@kernel.org>

Reviewed-by: Jonathon Reinhart <jonathon.reinhart@gmail.com>

> ---
>
> Hello,
>
> This is sent as an RFC as I'd like a fix[1] to be merged before to
> avoid introducing a new warning. But this can be reviewed in the
> meantime.
>
> I'm not sending this 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.
>
> Thanks!
> Antoine
>
> [1] https://lore.kernel.org/all/20211012145437.754391-1-atenart@kernel.org/T/
>
>  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);
>  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;
> --
> 2.31.1
>

This looks good to me. Thanks for the improvement, Antoine!

I would ask about .rodata, that would imply the use of 'const'
variables, which would be causing compiler warnings or errors. And
writes to those variables would already be crashing. So it doesn't
seem to be necessary.

(sorry for the duplicate mail; I accidentally sent HTML from mobile
the first time.)

Jonathon

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

[Antoine Tenart]

Quoting Antoine Tenart (2021-10-12 17:55:42)
> 
> This is sent as an RFC as I'd like a fix[1] to be merged before to
> avoid introducing a new warning. But this can be reviewed in the
> meantime.
> 
> I'm not sending this as a fix to avoid possible new warnings in stable
> kernels. (The actual fixes of sysctl files should go).
> 
> [1] https://lore.kernel.org/all/20211012145437.754391-1-atenart@kernel.org/T/

The related fix[1] was merged in the nf tree. It's not in net-next yet
and I'm not sure it'll have a chance to get there before the merge
window, but I don't think this matter much (there is no real dependency
between the two, except for avoiding to introduce a runtime warning).
I'll submit this formally.

Antoine