linux-kernel - Re: [PATCH] drm/msm/dpu: Fix memory leak in msm_mdss_parse_data_bus_icc

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

Message-ID: <CAD=FV=XXoue_vkuM7TmxnECfT3-p9d-OH=H_dyeoOk_Kp4kK2w@mail.gmail.com>
Date:   Tue, 6 Dec 2022 07:40:33 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Miaoqian Lin <linmq006@...il.com>
Cc:     Rob Clark <robdclark@...il.com>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Sean Paul <sean@...rly.run>, David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Stephen Boyd <swboyd@...omium.org>,
        linux-arm-msm@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/msm/dpu: Fix memory leak in msm_mdss_parse_data_bus_icc_path

Hi,

On Mon, Dec 5, 2022 at 11:55 PM Miaoqian Lin <linmq006@...il.com> wrote:
>
> of_icc_get() alloc resources for path1, we should release it when not
> need anymore. Early return when IS_ERR_OR_NULL(path0) may leak path1.
> Add icc_put(path1) in the error path to fix this.
>
> Fixes: b9364eed9232 ("drm/msm/dpu: Move min BW request and full BW disable back to mdss")
> Signed-off-by: Miaoqian Lin <linmq006@...il.com>
> ---
>  drivers/gpu/drm/msm/msm_mdss.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_mdss.c b/drivers/gpu/drm/msm/msm_mdss.c
> index e13c5c12b775..a38fa9a9a3d6 100644
> --- a/drivers/gpu/drm/msm/msm_mdss.c
> +++ b/drivers/gpu/drm/msm/msm_mdss.c
> @@ -49,8 +49,10 @@ static int msm_mdss_parse_data_bus_icc_path(struct device *dev,
>         struct icc_path *path0 = of_icc_get(dev, "mdp0-mem");
>         struct icc_path *path1 = of_icc_get(dev, "mdp1-mem");
>
> -       if (IS_ERR_OR_NULL(path0))
> +       if (IS_ERR_OR_NULL(path0)) {
> +               icc_put(path1);
>                 return PTR_ERR_OR_ZERO(path0);
> +       }
>
>         msm_mdss->path[0] = path0;
>         msm_mdss->num_paths = 1;

Hmmm. I guess the original author of the code (which wasn't me--I just
restored the code that was deleted by a previous change) was assuming
that if mdp0-mem had a problem that mdp1-mem would also have a
problem. That would mean that you wouldn't need to call icc_put() on
it.

...and, in fact, your patch doesn't handle that case, does it? If
_both_ of the two are error or NULL then you'll be calling icc_put()
on something invalid. I guess icc_put() handles those cases without
crashing but it will give a WARN_ON() splat if it happens to be an
error...

Really, there's a better solution anyway. Instead, you should do:

path0 = of_icc_get(dev, "mdp0-mem");
if (IS_ERR_OR_NULL(path0))
  return PTR_ERR_OR_ZERO(path0);

msm_mdss->path[0] = path0;
msm_mdss->num_paths = 1;

path1 = of_icc_get(dev, "mdp1-mem");
if (!IS_ERR_OR_NULL(path1)) {
 ...
}

In other words just defer getting path1 until after you've checked
path0 for an error.

-Doug