Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210914165623.18972-1-james.morse@arm.com>
In-Reply-To: <20210914165623.18972-1-james.morse@arm.com>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Tue, 14 Sep 2021 19:08:33 +0200
Message-ID: <CAJZ5v0juNZA8waQGyxTRQR05gQZa+qi0WksHUcfJSrzyFuMO7Q@mail.gmail.com>
Subject: Re: [PATCH] cpufreq: schedutil: Destroy mutex before kobject_put()
 frees the memory
To:     James Morse <james.morse@arm.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux PM <linux-pm@vger.kernel.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Kevin Hao <haokexin@gmail.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Ingo Molnar <mingo@redhat.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Viresh Kumar <viresh.kumar@linaro.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Tue, Sep 14, 2021 at 6:56 PM James Morse <james.morse@arm.com> wrote:
>
> Since commit e5c6b312ce3c ("cpufreq: schedutil: Use kobject release()
> method to free sugov_tunables") kobject_put() has kfree()d the
> attr_set before gov_attr_set_put() returns.
>
> kobject_put() isn't the last user of attr_set in gov_attr_set_put(),
> the subsequent mutex_destroy() triggers a use-after-free:
> | BUG: KASAN: use-after-free in mutex_is_locked+0x20/0x60
> | Read of size 8 at addr ffff000800ca4250 by task cpuhp/2/20
> |
> | CPU: 2 PID: 20 Comm: cpuhp/2 Not tainted 5.15.0-rc1 #12369
> | Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development
> | Platform, BIOS EDK II Jul 30 2018
> | Call trace:
> |  dump_backtrace+0x0/0x380
> |  show_stack+0x1c/0x30
> |  dump_stack_lvl+0x8c/0xb8
> |  print_address_description.constprop.0+0x74/0x2b8
> |  kasan_report+0x1f4/0x210
> |  kasan_check_range+0xfc/0x1a4
> |  __kasan_check_read+0x38/0x60
> |  mutex_is_locked+0x20/0x60
> |  mutex_destroy+0x80/0x100
> |  gov_attr_set_put+0xfc/0x150
> |  sugov_exit+0x78/0x190
> |  cpufreq_offline.isra.0+0x2c0/0x660
> |  cpuhp_cpufreq_offline+0x14/0x24
> |  cpuhp_invoke_callback+0x430/0x6d0
> |  cpuhp_thread_fun+0x1b0/0x624
> |  smpboot_thread_fn+0x5e0/0xa6c
> |  kthread+0x3a0/0x450
> |  ret_from_fork+0x10/0x20
>
> Swap the order of the calls.
>
> Fixes: e5c6b312ce3c ("cpufreq: schedutil: Use kobject release() method to free sugov_tunables")
> Cc: 4.7+ <stable@vger.kernel.org> # 4.7+
> CC: Kevin Hao <haokexin@gmail.com>
> CC: Viresh Kumar <viresh.kumar@linaro.org>
> CC: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  drivers/cpufreq/cpufreq_governor_attr_set.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpufreq/cpufreq_governor_attr_set.c b/drivers/cpufreq/cpufreq_governor_attr_set.c
> index 66b05a326910..a6f365b9cc1a 100644
> --- a/drivers/cpufreq/cpufreq_governor_attr_set.c
> +++ b/drivers/cpufreq/cpufreq_governor_attr_set.c
> @@ -74,8 +74,8 @@ unsigned int gov_attr_set_put(struct gov_attr_set *attr_set, struct list_head *l
>         if (count)
>                 return count;
>
> -       kobject_put(&attr_set->kobj);
>         mutex_destroy(&attr_set->update_lock);
> +       kobject_put(&attr_set->kobj);

Doh

I should have caught this one, thanks!

>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(gov_attr_set_put);
> --

Applied as 5.15-rc material, thanks!