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

[PATCH 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>
Reviewed-by: Jonathon Reinhart <jonathon.reinhart@gmail.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.

Thanks,
Antoine

[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);
 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: [PATCH net-next] net: sysctl data could be in .bss

[Jakub Kicinski]

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@kernel.org>
> Reviewed-by: Jonathon Reinhart <jonathon.reinhart@gmail.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;

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

[Jonathon Reinhart]

On Fri, Oct 22, 2021 at 4:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> 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@kernel.org>
> > Reviewed-by: Jonathon Reinhart <jonathon.reinhart@gmail.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?

The core_kernel_data() function was introduced in a2d063ac216c1, and
the commit message says:

"It may or may not return true for RO data... This utility function is
used to determine if data is safe from ever being freed. Thus it
should return true for all RW global data that is not in a module or
has been allocated, or false otherwise."

The intent of the function seems to be more in line with the
higher-level "is this global kernel data" semantics you suggested. The
purpose seems to be to differentiate between "part of the loaded
kernel image" vs. a dynamic allocation (which would include a loaded
module image). And given that it *might* return true for RO data
(depending on the arch linker script, presumably), I think it would be
safe to include .bss -- clearly, with that caveat in place, it isn't
promising strict section semantics.

There are only two existing in-tree consumers:

1. __register_ftrace_function() [kernel/trace/ftrace.c] -- Sets
FTRACE_OPS_FL_DYNAMIC if core_kernel_data(ops) returns false, which
denotes "dynamically allocated ftrace_ops which need special care". It
would be unlikely (if not impossible) for the "ops" object in question
to be all-zero and end up in the .bss, but if it were, then the
current behavior would be wrong. IOW, it would be more correct to
include .bss.

2. ensure_safe_net_sysctl() [net/sysctl_net.c] (The subject of this
thread) -- Trying to distinguish "global kernel data" (static/global
variables) from kmalloc-allocated objects. More correct to include
.bss.

Both of these callers only seem to delineate between static and
dynamic object allocations. Put another way, if core_kernel_bss(), all
existing callers should be updated to check core_kernel_data() ||
core_kernel_bss().

Since Steven introduced it, and until I added
ensure_safe_net_sysctl(), he / tracing was the only consumer.

Thinking critically from the C language perspective, I can't come up
with any case where one would actually expect core_kernel_data() to
return true for 'int global = 1' and false for 'int global = 0'.

In conclusion, I agree with your alternative proposal Jakub, and I
think this patch is the right way forward:

diff --git a/kernel/extable.c b/kernel/extable.c
index b0ea5eb0c3b4..8b6f1d0bdaf6 100644
--- a/kernel/extable.c
+++ b/kernel/extable.c
@@ -97,6 +97,9 @@ int core_kernel_data(unsigned long addr)
        if (addr >= (unsigned long)_sdata &&
            addr < (unsigned long)_edata)
                return 1;
+       if (addr >= (unsigned long)__bss_start &&
+           addr < (unsigned long)__bss_stop)
+               return 1;
        return 0;
 }

Jonathon Reinhart

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

[Steven Rostedt]

On Mon, 8 Nov 2021 00:24:33 -0500
Jonathon Reinhart <jonathon.reinhart@gmail.com> wrote:

> On Fri, Oct 22, 2021 at 4:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > Widening the CC list a little.

Thanks!

[..]

> The core_kernel_data() function was introduced in a2d063ac216c1, and
> the commit message says:
> 
> "It may or may not return true for RO data... This utility function is
> used to determine if data is safe from ever being freed. Thus it
> should return true for all RW global data that is not in a module or
> has been allocated, or false otherwise."
> 
> The intent of the function seems to be more in line with the
> higher-level "is this global kernel data" semantics you suggested. The
> purpose seems to be to differentiate between "part of the loaded
> kernel image" vs. a dynamic allocation (which would include a loaded
> module image). And given that it *might* return true for RO data
> (depending on the arch linker script, presumably), I think it would be
> safe to include .bss -- clearly, with that caveat in place, it isn't
> promising strict section semantics.
> 
> There are only two existing in-tree consumers:
> 
> 1. __register_ftrace_function() [kernel/trace/ftrace.c] -- Sets
> FTRACE_OPS_FL_DYNAMIC if core_kernel_data(ops) returns false, which
> denotes "dynamically allocated ftrace_ops which need special care". It
> would be unlikely (if not impossible) for the "ops" object in question
> to be all-zero and end up in the .bss, but if it were, then the
> current behavior would be wrong. IOW, it would be more correct to
> include .bss.
> 
> 2. ensure_safe_net_sysctl() [net/sysctl_net.c] (The subject of this
> thread) -- Trying to distinguish "global kernel data" (static/global
> variables) from kmalloc-allocated objects. More correct to include
> .bss.
> 
> Both of these callers only seem to delineate between static and
> dynamic object allocations. Put another way, if core_kernel_bss(), all
> existing callers should be updated to check core_kernel_data() ||
> core_kernel_bss().
> 
> Since Steven introduced it, and until I added
> ensure_safe_net_sysctl(), he / tracing was the only consumer.

I agree with your analysis.

The intent is that allocated ftrace_ops (things that function tracer uses
to know what callbacks are called from function entry), must go through a
very slow synchronization (rcu_synchronize_tasks). But this is not needed
if the ftrace_ops is part of the core kernel (.data or .bss) as that will
never be freed, and thus does not need to worry about it disappearing while
they are still in use.

> 
> Thinking critically from the C language perspective, I can't come up
> with any case where one would actually expect core_kernel_data() to
> return true for 'int global = 1' and false for 'int global = 0'.
> 
> In conclusion, I agree with your alternative proposal Jakub, and I
> think this patch is the right way forward:
> 
> diff --git a/kernel/extable.c b/kernel/extable.c
> index b0ea5eb0c3b4..8b6f1d0bdaf6 100644
> --- a/kernel/extable.c
> +++ b/kernel/extable.c
> @@ -97,6 +97,9 @@ int core_kernel_data(unsigned long addr)
>         if (addr >= (unsigned long)_sdata &&
>             addr < (unsigned long)_edata)
>                 return 1;
> +       if (addr >= (unsigned long)__bss_start &&
> +           addr < (unsigned long)__bss_stop)
> +               return 1;
>         return 0;
>  }

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

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

[Antoine Tenart]

Thanks Jonathon for the analysis and Steven for the review! I'll send
the patch formally then.

Antoine

Quoting Steven Rostedt (2021-11-08 17:11:14)
> On Mon, 8 Nov 2021 00:24:33 -0500
> Jonathon Reinhart <jonathon.reinhart@gmail.com> wrote:
> 
> > On Fri, Oct 22, 2021 at 4:01 PM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > Widening the CC list a little.
> 
> Thanks!
> 
> [..]
> 
> > The core_kernel_data() function was introduced in a2d063ac216c1, and
> > the commit message says:
> > 
> > "It may or may not return true for RO data... This utility function is
> > used to determine if data is safe from ever being freed. Thus it
> > should return true for all RW global data that is not in a module or
> > has been allocated, or false otherwise."
> > 
> > The intent of the function seems to be more in line with the
> > higher-level "is this global kernel data" semantics you suggested. The
> > purpose seems to be to differentiate between "part of the loaded
> > kernel image" vs. a dynamic allocation (which would include a loaded
> > module image). And given that it *might* return true for RO data
> > (depending on the arch linker script, presumably), I think it would be
> > safe to include .bss -- clearly, with that caveat in place, it isn't
> > promising strict section semantics.
> > 
> > There are only two existing in-tree consumers:
> > 
> > 1. __register_ftrace_function() [kernel/trace/ftrace.c] -- Sets
> > FTRACE_OPS_FL_DYNAMIC if core_kernel_data(ops) returns false, which
> > denotes "dynamically allocated ftrace_ops which need special care". It
> > would be unlikely (if not impossible) for the "ops" object in question
> > to be all-zero and end up in the .bss, but if it were, then the
> > current behavior would be wrong. IOW, it would be more correct to
> > include .bss.
> > 
> > 2. ensure_safe_net_sysctl() [net/sysctl_net.c] (The subject of this
> > thread) -- Trying to distinguish "global kernel data" (static/global
> > variables) from kmalloc-allocated objects. More correct to include
> > .bss.
> > 
> > Both of these callers only seem to delineate between static and
> > dynamic object allocations. Put another way, if core_kernel_bss(), all
> > existing callers should be updated to check core_kernel_data() ||
> > core_kernel_bss().
> > 
> > Since Steven introduced it, and until I added
> > ensure_safe_net_sysctl(), he / tracing was the only consumer.
> 
> I agree with your analysis.
> 
> The intent is that allocated ftrace_ops (things that function tracer uses
> to know what callbacks are called from function entry), must go through a
> very slow synchronization (rcu_synchronize_tasks). But this is not needed
> if the ftrace_ops is part of the core kernel (.data or .bss) as that will
> never be freed, and thus does not need to worry about it disappearing while
> they are still in use.
> 
> > 
> > Thinking critically from the C language perspective, I can't come up
> > with any case where one would actually expect core_kernel_data() to
> > return true for 'int global = 1' and false for 'int global = 0'.
> > 
> > In conclusion, I agree with your alternative proposal Jakub, and I
> > think this patch is the right way forward:
> > 
> > diff --git a/kernel/extable.c b/kernel/extable.c
> > index b0ea5eb0c3b4..8b6f1d0bdaf6 100644
> > --- a/kernel/extable.c
> > +++ b/kernel/extable.c
> > @@ -97,6 +97,9 @@ int core_kernel_data(unsigned long addr)
> >         if (addr >= (unsigned long)_sdata &&
> >             addr < (unsigned long)_edata)
> >                 return 1;
> > +       if (addr >= (unsigned long)__bss_start &&
> > +           addr < (unsigned long)__bss_stop)
> > +               return 1;
> >         return 0;
> >  }
> 
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> -- Steve
>