linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: nixiaoming <nixiaoming@huawei.com>
Cc: Jan Kara <jack@suse.cz>, Amir Goldstein <amir73il@gmail.com>,
	"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH] fix memory leak in ramoops_init
Date: Fri, 28 Sep 2018 15:18:19 -0700	[thread overview]
Message-ID: <CAGXu5jKfZbRC2t0b6RnFmS2buhP-bvTO0aFSkR8knPXZFnDCsA@mail.gmail.com> (raw)
In-Reply-To: <20180917091531.21356-1-nixiaoming@huawei.com>

In the future, please use scripts/get_maintainer.pl to find your To/Cc list. :)

On Mon, Sep 17, 2018 at 2:15 AM, nixiaoming <nixiaoming@huawei.com> wrote:
> 1, memory leak in ramoops_register_dummy.
>    dummy_data = kzalloc(sizeof(*dummy_data), GFP_KERNEL);
>    but no free when platform_device_register_data return fail

Yup, that's a problem.

> 2, if kzalloc(sizeof(*dummy_data), GFP_KERNEL) return NULL,
>     but platform_driver_register(&ramoops_driver) return 0
>    kfree(NULL) in ramoops_exit
> so, add return val for ramoops_register_dummy, and check it in ramoops_init

The kfree(NULL) isn't a problem, but a NULL platform_data implies
device tree data is expected, so it'll just confuse things. However,
since the dummy platform data is optional, there's no need to plumb
the return value back up. Either it gets set up correctly, or there is
a pr_info() about why it has been ignored.

> 3, memory leak in ramoops_init.
>    miss platform_device_unregister(dummy) and kfree(dummy_data)
>    when platform_driver_register(&ramoops_driver) return fail

Agreed.

> Signed-off-by: nixiaoming <nixiaoming@huawei.com>
> ---
>  fs/pstore/ram.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index bd9812e..331b600 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -604,17 +604,17 @@ static struct platform_driver ramoops_driver = {
>         },
>  };
>
> -static void ramoops_register_dummy(void)
> +static int ramoops_register_dummy(void)

Another bug is that this should actually be __init (it's only called
by an __init).

>  {
>         if (!mem_size)
> -               return;
> +               return -EINVAL;
>
>         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;
> +               return -ENOMEM;
>         }
>
>         dummy_data->mem_size = mem_size;
> @@ -636,13 +636,25 @@ static void ramoops_register_dummy(void)
>         if (IS_ERR(dummy)) {
>                 pr_info("could not create platform device: %ld\n",
>                         PTR_ERR(dummy));
> +               kfree(dummy_data);
> +               return PTR_ERR(dummy);
>         }
> +       return 0;
>  }
>
>  static int __init ramoops_init(void)
>  {
> -       ramoops_register_dummy();
> -       return platform_driver_register(&ramoops_driver);
> +       int ret = ramoops_register_dummy();
> +
> +       if (ret != 0)
> +               return ret;
> +
> +       ret = platform_driver_register(&ramoops_driver);
> +       if (ret != 0) {
> +               platform_device_unregister(dummy);
> +               kfree(dummy_data);
> +       }
> +       return ret;
>  }
>  postcore_initcall(ramoops_init);
>
> --
> 1.9.0
>

I'll send an updated patch with a similar fix...

-Kees

-- 
Kees Cook
Pixel Security

      parent reply	other threads:[~2018-09-29  4:44 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-09-17  9:15 [PATCH] fix memory leak in ramoops_init nixiaoming
2018-09-28 21:26 ` Andrew Morton
2018-09-28 21:54   ` Kees Cook
2018-09-28 22:18 ` Kees Cook [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAGXu5jKfZbRC2t0b6RnFmS2buhP-bvTO0aFSkR8knPXZFnDCsA@mail.gmail.com \
    --to=keescook@chromium.org \
    --cc=akpm@linux-foundation.org \
    --cc=amir73il@gmail.com \
    --cc=jack@suse.cz \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nixiaoming@huawei.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).