linux-kernel - Re: [PATCH 2/4] arm64: mte: Add asynchronous mode support

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: <CAAeHK+xBrCX1Ly0RU-=ySEU8SsyyRkMdOYrN52ONc4DeRJA5eg@mail.gmail.com>
Date:   Thu, 7 Jan 2021 20:18:13 +0100
From:   Andrey Konovalov <andreyknvl@...gle.com>
To:     Vincenzo Frascino <vincenzo.frascino@....com>
Cc:     Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        kasan-dev <kasan-dev@...glegroups.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Dmitry Vyukov <dvyukov@...gle.com>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Alexander Potapenko <glider@...gle.com>,
        Marco Elver <elver@...gle.com>,
        Evgenii Stepanov <eugenis@...gle.com>,
        Branislav Rankov <Branislav.Rankov@....com>
Subject: Re: [PATCH 2/4] arm64: mte: Add asynchronous mode support

On Thu, Jan 7, 2021 at 6:25 PM Vincenzo Frascino
<vincenzo.frascino@....com> wrote:
>
> Hi Andrey,
>
> On 1/7/21 4:29 PM, Andrey Konovalov wrote:
> > On Wed, Jan 6, 2021 at 12:56 PM Vincenzo Frascino
> > <vincenzo.frascino@....com> wrote:
> >>
> >> MTE provides an asynchronous mode for detecting tag exceptions. In
> >> particular instead of triggering a fault the arm64 core updates a
> >> register which is checked by the kernel at the first entry after the tag
> >> exception has occurred.
> >>
> >> Add support for MTE asynchronous mode.
> >>
> >> The exception handling mechanism will be added with a future patch.
> >>
> >> Note: KASAN HW activates async mode via kasan.mode kernel parameter.
> >> The default mode is set to synchronous.
> >>
> >> Cc: Catalin Marinas <catalin.marinas@....com>
> >> Cc: Will Deacon <will.deacon@....com>
> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@....com>
> >> ---
> >>  arch/arm64/kernel/mte.c | 31 +++++++++++++++++++++++++++++--
> >>  1 file changed, 29 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
> >> index 24a273d47df1..5d992e16b420 100644
> >> --- a/arch/arm64/kernel/mte.c
> >> +++ b/arch/arm64/kernel/mte.c
> >> @@ -153,8 +153,35 @@ void mte_init_tags(u64 max_tag)
> >>
> >>  void mte_enable_kernel(enum kasan_arg_mode mode)
> >>  {
> >> -       /* Enable MTE Sync Mode for EL1. */
> >> -       sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> >> +       const char *m;
> >> +
> >> +       /* Preset parameter values based on the mode. */
> >> +       switch (mode) {
> >> +       case KASAN_ARG_MODE_OFF:
> >> +               return;
> >> +       case KASAN_ARG_MODE_LIGHT:
> >> +               /* Enable MTE Async Mode for EL1. */
> >> +               sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_ASYNC);
> >> +               m = "asynchronous";
> >> +               break;
> >> +       case KASAN_ARG_MODE_DEFAULT:
> >> +       case KASAN_ARG_MODE_PROD:
> >> +       case KASAN_ARG_MODE_FULL:
> >> +               /* Enable MTE Sync Mode for EL1. */
> >> +               sysreg_clear_set(sctlr_el1, SCTLR_ELx_TCF_MASK, SCTLR_ELx_TCF_SYNC);
> >> +               m = "synchronous";
> >> +               break;
> >> +       default:
> >> +               /*
> >> +                * kasan mode should be always set hence we should
> >> +                * not reach this condition.
> >> +                */
> >> +               WARN_ON_ONCE(1);
> >> +               return;
> >> +       }
> >> +
> >> +       pr_info_once("MTE: enabled in %s mode at EL1\n", m);
> >> +
> >>         isb();
> >>  }
> >>
> >> --
> >> 2.29.2
> >>
> >
> > Hi Vincenzo,
> >
> > It would be cleaner to pass a bool to mte_enable_kernel() and have it
> > indicate sync/async mode. This way you don't have to pull all these
> > KASAN constants into the arm64 code.
> >
>
> Boolean arguments are generally bad for legibility, hence I tend to avoid them.
> In this case exposing the constants does not seem a big issue especially because
> the only user of this code is "KASAN_HW_TAGS" and definitely improves its
> legibility hence I would prefer to keep it as is.

I don't like that this spills KASAN internals to the arm64 code. Let's
add another enum with two values and pass it as an argument then.
Something like:

enum mte_mode {
  ARM_MTE_SYNC,
  ARM_MTE_ASYNC
}