linux-kernel - Re: [PATCH v4 4/5] arm64: mte: Enable async tag check fault

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: <7db77f90-d595-c094-2f71-8ef1b05f1c7f@arm.com>
Date:   Tue, 19 Jan 2021 14:45:52 +0000
From:   Vincenzo Frascino <vincenzo.frascino@....com>
To:     Catalin Marinas <catalin.marinas@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        kasan-dev@...glegroups.com, Will Deacon <will@...nel.org>,
        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>,
        Andrey Konovalov <andreyknvl@...gle.com>
Subject: Re: [PATCH v4 4/5] arm64: mte: Enable async tag check fault



On 1/19/21 2:34 PM, Catalin Marinas wrote:
> On Mon, Jan 18, 2021 at 06:30:32PM +0000, Vincenzo Frascino wrote:
>>  static void update_sctlr_el1_tcf0(u64 tcf0)
>>  {
>>  	/* ISB required for the kernel uaccess routines */
>> @@ -235,6 +273,15 @@ void mte_thread_switch(struct task_struct *next)
>>  	/* avoid expensive SCTLR_EL1 accesses if no change */
>>  	if (current->thread.sctlr_tcf0 != next->thread.sctlr_tcf0)
>>  		update_sctlr_el1_tcf0(next->thread.sctlr_tcf0);
>> +
>> +	/*
>> +	 * Check if an async tag exception occurred at EL1.
>> +	 *
>> +	 * Note: On the context switch path we rely on the dsb() present
>> +	 * in __switch_to() to guarantee that the indirect writes to TFSR_EL1
>> +	 * are synchronized before this point.
>> +	 */
>> +	mte_check_tfsr_el1();
>>  }
> 
> We need an isb() before mte_check_tfsr_el1() here as well, we only have
> a dsb() in __switch_to(). We do have an isb() in update_sctlr_el1_tcf0()
> but only if the check passed. Now, it's worth benchmarking how expensive
> update_sctlr_el1_tcf0() is (i.e. an SCTLR_EL1 access + isb with
> something like hackbench) and we could probably remove the check
> altogether. In the meantime, you can add an isb() on the "else" path of
> the above check.
> 

Good catch, I saw the isb() in update_sctlr_el1_tcf0() and for some reasons that
it is not escaping me I thought it was sufficient, but clearly it is not.

I am happy to benchmark what you are suggesting and provide some data after this
series is merged (if it works for you) so that we can decide. In the meantime as
you suggested I will fix the "else" for v5.

-- 
Regards,
Vincenzo