linux-kernel - Re: [PATCH] kcov: don't lose track of remote references during softirqs

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]

Message-ID: <CA+fCnZdfB206Bjw=MAkZ9qbKUtf-KeGrrqJnOJ1ZrgH6fGXRhA@mail.gmail.com>
Date: Fri, 14 Jun 2024 01:02:20 +0200
From: Andrey Konovalov <andreyknvl@...il.com>
To: Aleksandr Nogikh <nogikh@...gle.com>
Cc: dvyukov@...gle.com, arnd@...db.de, akpm@...ux-foundation.org, 
	elver@...gle.com, glider@...gle.com, syzkaller@...glegroups.com, 
	kasan-dev@...glegroups.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kcov: don't lose track of remote references during softirqs

On Tue, Jun 11, 2024 at 3:32 PM Aleksandr Nogikh <nogikh@...gle.com> wrote:
>
> In kcov_remote_start()/kcov_remote_stop(), we swap the previous KCOV
> metadata of the current task into a per-CPU variable. However, the
> kcov_mode_enabled(mode) check is not sufficient in the case of remote
> KCOV coverage: current->kcov_mode always remains KCOV_MODE_DISABLED
> for remote KCOV objects.
>
> If the original task that has invoked the KCOV_REMOTE_ENABLE ioctl
> happens to get interrupted and kcov_remote_start() is called, it
> ultimately leads to kcov_remote_stop() NOT restoring the original
> KCOV reference. So when the task exits, all registered remote KCOV
> handles remain active forever.
>
> Fix it by introducing a special kcov_mode that is assigned to the
> task that owns a KCOV remote object. It makes kcov_mode_enabled()
> return true and yet does not trigger coverage collection in
> __sanitizer_cov_trace_pc() and write_comp_data().
>
> Signed-off-by: Aleksandr Nogikh <nogikh@...gle.com>
> Fixes: 5ff3b30ab57d ("kcov: collect coverage from interrupts")
> ---
>  include/linux/kcov.h | 2 ++
>  kernel/kcov.c        | 1 +
>  2 files changed, 3 insertions(+)
>
> diff --git a/include/linux/kcov.h b/include/linux/kcov.h
> index b851ba415e03..3b479a3d235a 100644
> --- a/include/linux/kcov.h
> +++ b/include/linux/kcov.h
> @@ -21,6 +21,8 @@ enum kcov_mode {
>         KCOV_MODE_TRACE_PC = 2,
>         /* Collecting comparison operands mode. */
>         KCOV_MODE_TRACE_CMP = 3,
> +       /* The process owns a KCOV remote reference. */
> +       KCOV_MODE_REMOTE = 4,
>  };
>
>  #define KCOV_IN_CTXSW  (1 << 30)
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index c3124f6d5536..5371d3f7b5c3 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -632,6 +632,7 @@ static int kcov_ioctl_locked(struct kcov *kcov, unsigned int cmd,
>                         return -EINVAL;
>                 kcov->mode = mode;
>                 t->kcov = kcov;
> +               WRITE_ONCE(t->kcov_mode, KCOV_MODE_REMOTE);

Looking at this again, I don't think we need this WRITE_ONCE here, as
we have interrupts disabled. But if we do, perhaps it makes sense to
add a comment explaining why.

>                 kcov->t = t;
>                 kcov->remote = true;
>                 kcov->remote_size = remote_arg->area_size;
> --
> 2.45.2.505.gda0bf45e8d-goog
>