linux-kernel - Re: [PATCH] eventfs: Hold eventfs_mutex when calling callback functions

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: <CA+G9fYu5VdDPB25croT8OEFRcfxMSHRduL0EK8ct7EVWN3t5aQ@mail.gmail.com>
Date:   Tue, 31 Oct 2023 03:11:31 +0530
From:   Naresh Kamboju <naresh.kamboju@...aro.org>
To:     Steven Rostedt <rostedt@...dmis.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linux Trace Kernel <linux-trace-kernel@...r.kernel.org>,
        Masami Hiramatsu <mhiramat@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "Arnd Bergmann <arnd@...db.de>, naresh.kamboju@...aro.org, Beau Belgrave
        <beaub@...ux.microsoft.com>, Ajay Kaher <akaher@...are.com>, Andrew
        Morton" <akpm@...ux-foundation.org>
Subject: Re: [PATCH] eventfs: Hold eventfs_mutex when calling callback functions

On Mon, 30 Oct 2023 at 21:10, Steven Rostedt <rostedt@...dmis.org> wrote:
>
> From: "Steven Rostedt (Google)" <rostedt@...dmis.org>
>
> The callback function that is used to create inodes and dentries is not
> protected by anything and the data that is passed to it could become
> stale. After eventfs_remove_dir() is called by the tracing system, it is
> free to remove the events that are associated to that directory.
> Unfortunately, that means the callbacks must not be called after that.
>
>      CPU0                               CPU1
>      ----                               ----
>  eventfs_root_lookup() {
>                                  eventfs_remove_dir() {
>                                       mutex_lock(&event_mutex);
>                                       ei->is_freed = set;
>                                       mutex_unlock(&event_mutex);
>                                  }
>                                  kfree(event_call);
>
>     for (...) {
>       entry = &ei->entries[i];
>       r = entry->callback() {
>           call = data;          // call == event_call above
>           if (call->flags ...)
>
>  [ USE AFTER FREE BUG ]
>
> The safest way to protect this is to wrap the callback with:
>
>  mutex_lock(&eventfs_mutex);
>  if (!ei->is_freed)
>      r = entry->callback();
>  else
>      r = -1;
>  mutex_unlock(&eventfs_mutex);
>
> This will make sure that the callback will not be called after it is
> freed. But now it needs to be known that the callback is called while
> holding internal eventfs locks, and that it must not call back into the
> eventfs / tracefs system. There's no reason it should anyway, but document
> that as well.
>
> Link: https://lore.kernel.org/all/CA+G9fYu9GOEbD=rR5eMR-=HJ8H6rMsbzDC2ZY5=Y50WpWAE7_Q@mail.gmail.com/
>
> Reported-by: Linux Kernel Functional Testing <lkft@...aro.org>
> Reported-by: Naresh Kamboju <naresh.kamboju@...aro.org>
> Signed-off-by: Steven Rostedt (Google) <rostedt@...dmis.org>

Tested-by: Linux Kernel Functional Testing <lkft@...aro.org>
Tested-by: Naresh Kamboju <naresh.kamboju@...aro.org>

> ---
>  fs/tracefs/event_inode.c | 22 ++++++++++++++++++--
>  include/linux/tracefs.h  | 43 ++++++++++++++++++++++++++++++++++++++++


--
Linaro LKFT
https://lkft.linaro.org