netdev - Re: [RFC 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]

Message-ID: <CAPFHKzfg2+cvbbbDL0Qt1LRiYKO6N+7DeNshLZXnbb6+Fo7QPQ@mail.gmail.com>
Date:   Wed, 13 Oct 2021 08:03:39 -0400
From:   Jonathon Reinhart <jonathon.reinhart@...il.com>
To:     Antoine Tenart <atenart@...nel.org>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Netdev List <netdev@...r.kernel.org>
Subject: Re: [RFC net-next] net: sysctl data could be in .bss

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

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