linux-kernel - Re: [PATCH] pstore/ram: Replace dummy_data heap memory with stack memory

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>] [day] [month] [year] [list]

Message-ID: <CAGXu5jKs5Hzfaho0Bg7rJCLa7XJPzBOnBXMUNHPj2jjgOBD=8Q@mail.gmail.com>
Date:   Tue, 22 Jan 2019 14:53:01 +1300
From:   Kees Cook <keescook@...omium.org>
To:     Yue Hu <huyue2@...ong.com>
Cc:     Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pstore/ram: Replace dummy_data heap memory with stack memory

On Tue, Jan 22, 2019 at 2:37 PM Yue Hu <huyue2@...ong.com> wrote:
>
> From e37cbd4d22eae55c034536817b22d429ba0ae27a Mon Sep 17 00:00:00 2001
> From: Yue Hu <huyue2@...ong.com>
> Date: Mon, 21 Jan 2019 18:20:41 +0800
> Subject: [PATCH] pstore/ram: Replace dummy_data heap memory with stack
> memory
>
> In ramoops_register_dummy() dummy_data is allocated via kzalloc()
> then it will always occupy the heap space after register platform
> device via platform_device_register_data(), but it will not be
> used any more. So let's free it for system usage, replace it with
> stack memory is better due to small size.

That's not true: dummy_data is registered with
platform_device_register_data() which stores the pointer. (And is used
in ramoops_probe().) When the probe runs, the stack frame will be
gone.

That said, it would be possible to move the stack allocation into
ramoops_init() instead, since the frame will still be available during
the call to platform_driver_register() which will call
ramoops_probe(). If you made that change, then we could drop the
ramoops_unregister_dummy() call, and rename ramoops_register_dummy()
into ramoops_populate_dummy() or something.

But even in this case, you'd need to remove the platform data after
the probe just to make sure the stack pointer didn't stick around.

-Kees

>
> Signed-off-by: Yue Hu <huyue2@...ong.com>
> ---
>  fs/pstore/ram.c | 34 +++++++++++++---------------------
>  1 file changed, 13 insertions(+), 21 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index 96f7d32..8db1f7f 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -110,7 +110,6 @@ struct ramoops_context {
>  };
>
>  static struct platform_device *dummy;
> -static struct ramoops_platform_data *dummy_data;
>
>  static int ramoops_pstore_open(struct pstore_info *psi)
>  {
> @@ -896,13 +895,12 @@ static inline void ramoops_unregister_dummy(void)
>  {
>         platform_device_unregister(dummy);
>         dummy = NULL;
> -
> -       kfree(dummy_data);
> -       dummy_data = NULL;
>  }
>
>  static void __init ramoops_register_dummy(void)
>  {
> +       struct ramoops_platform_data pdata;
> +
>         /*
>          * Prepare a dummy platform data structure to carry the module
>          * parameters. If mem_size isn't set, then there are no module
> @@ -913,30 +911,24 @@ static void __init ramoops_register_dummy(void)
>
>         pr_info("using module parameters\n");
>
> -       dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
> -       if (!dummy_data) {
> -               pr_info("could not allocate pdata\n");
> -               return;
> -       }
> -
> -       dummy_data->mem_size = mem_size;
> -       dummy_data->mem_address = mem_address;
> -       dummy_data->mem_type = mem_type;
> -       dummy_data->record_size = record_size;
> -       dummy_data->console_size = ramoops_console_size;
> -       dummy_data->ftrace_size = ramoops_ftrace_size;
> -       dummy_data->pmsg_size = ramoops_pmsg_size;
> -       dummy_data->dump_oops = dump_oops;
> -       dummy_data->flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
> +       pdata.mem_size = mem_size;
> +       pdata.mem_address = mem_address;
> +       pdata.mem_type = mem_type;
> +       pdata.record_size = record_size;
> +       pdata.console_size = ramoops_console_size;
> +       pdata.ftrace_size = ramoops_ftrace_size;
> +       pdata.pmsg_size = ramoops_pmsg_size;
> +       pdata.dump_oops = dump_oops;
> +       pdata.flags = RAMOOPS_FLAG_FTRACE_PER_CPU;
>
>         /*
>          * For backwards compatibility ramoops.ecc=1 means 16 bytes ECC
>          * (using 1 byte for ECC isn't much of use anyway).
>          */
> -       dummy_data->ecc_info.ecc_size = ramoops_ecc == 1 ? 16 :
> ramoops_ecc;
> +       pdata.ecc_info.ecc_size = ramoops_ecc == 1 ? 16 : ramoops_ecc;
>
>         dummy = platform_device_register_data(NULL, "ramoops", -1,
> -                       dummy_data, sizeof(struct
> ramoops_platform_data));
> +                       &pdata, sizeof(struct ramoops_platform_data));
>         if (IS_ERR(dummy)) {
>                 pr_info("could not create platform device: %ld\n",
>                         PTR_ERR(dummy));
> --
> 1.9.1
>
>
>
>


-- 
Kees Cook