| Message ID | 20211012155542.827631-1-atenart@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC,net-next] net: sysctl data could be in .bss | expand |
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
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
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;
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(-)