linux-kernel - Re: [PATCH 09/13] platform/chrome: cros_ec_proto: use devm

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]

Date:   Mon, 6 Jun 2022 09:04:09 -0700
From:   Guenter Roeck <groeck@...gle.com>
To:     Tzung-Bi Shih <tzungbi@...nel.org>
Cc:     Benson Leung <bleung@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        chrome-platform@...ts.linux.dev,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 09/13] platform/chrome: cros_ec_proto: use devm_krealloc()

On Mon, Jun 6, 2022 at 7:12 AM Tzung-Bi Shih <tzungbi@...nel.org> wrote:
>
> Use devm_krealloc() to re-allocate `din` and `dout`.
>
> Also remove the unneeded devm_kfree() in error handling path as they are
> device managed memory.
>

Problem with that is that the callers don't always handle error
returns from calls to cros_ec_query_all(), that the call isn't
typically from the probe function, and that the release function would
not be called after partial allocation failures until the driver is
unloaded. This would result in memory leaks, making the memory
situation even worse. I am not sure if using devm_ functions to
allocate the memory really makes sense here.

Guenter

> Signed-off-by: Tzung-Bi Shih <tzungbi@...nel.org>
> ---
>  drivers/platform/chrome/cros_ec_proto.c      | 25 ++++++--------------
>  drivers/platform/chrome/cros_ec_proto_test.c |  3 +--
>  2 files changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
> index abb30a685567..5f4414f05d66 100644
> --- a/drivers/platform/chrome/cros_ec_proto.c
> +++ b/drivers/platform/chrome/cros_ec_proto.c
> @@ -479,21 +479,13 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>                 }
>         }
>
> -       devm_kfree(dev, ec_dev->din);
> -       devm_kfree(dev, ec_dev->dout);
> -
> -       ec_dev->din = devm_kzalloc(dev, ec_dev->din_size, GFP_KERNEL);
> -       if (!ec_dev->din) {
> -               ret = -ENOMEM;
> -               goto exit;
> -       }
> +       ec_dev->din = devm_krealloc(dev, ec_dev->din, ec_dev->din_size, GFP_KERNEL | __GFP_ZERO);
> +       if (!ec_dev->din)
> +               return -ENOMEM;
>
> -       ec_dev->dout = devm_kzalloc(dev, ec_dev->dout_size, GFP_KERNEL);
> -       if (!ec_dev->dout) {
> -               devm_kfree(dev, ec_dev->din);
> -               ret = -ENOMEM;
> -               goto exit;
> -       }
> +       ec_dev->dout = devm_krealloc(dev, ec_dev->dout, ec_dev->dout_size, GFP_KERNEL | __GFP_ZERO);
> +       if (!ec_dev->dout)
> +               return -ENOMEM;
>
>         /* Probe if MKBP event is supported */
>         ret = cros_ec_get_host_command_version_mask(ec_dev,
> @@ -542,10 +534,7 @@ int cros_ec_query_all(struct cros_ec_device *ec_dev)
>                                 "failed to retrieve wake mask: %d\n", ret);
>         }
>
> -       ret = 0;
> -
> -exit:
> -       return ret;
> +       return 0;
>  }
>  EXPORT_SYMBOL(cros_ec_query_all);
>
> diff --git a/drivers/platform/chrome/cros_ec_proto_test.c b/drivers/platform/chrome/cros_ec_proto_test.c
> index 79150bf511fb..22f9322787f4 100644
> --- a/drivers/platform/chrome/cros_ec_proto_test.c
> +++ b/drivers/platform/chrome/cros_ec_proto_test.c
> @@ -180,8 +180,7 @@ static void cros_ec_proto_test_query_all_pretest(struct kunit *test)
>
>         /*
>          * cros_ec_query_all() will free din and dout and allocate them again to fit the usage by
> -        * calling devm_kfree() and devm_kzalloc().  Set them to NULL as they aren't managed by
> -        * ec_dev->dev.
> +        * calling devm_krealloc().  Set them to NULL as they aren't managed by ec_dev->dev.
>          */
>         ec_dev->din = NULL;
>         ec_dev->dout = NULL;
> --
> 2.36.1.255.ge46751e96f-goog
>