linux-kernel - Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations

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: <956a8e10-e03f-a21c-99d9-8a75c2616e0a@virtuozzo.com>
Date:   Tue, 21 Mar 2017 12:25:06 +0300
From:   Andrey Ryabinin <aryabinin@...tuozzo.com>
To:     Mark Rutland <mark.rutland@....com>,
        Dmitry Vyukov <dvyukov@...gle.com>
CC:     <peterz@...radead.org>, <mingo@...hat.com>, <will.deacon@....com>,
        <akpm@...ux-foundation.org>, <kasan-dev@...glegroups.com>,
        <linux-mm@...ck.org>, <x86@...nel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] asm-generic, x86: wrap atomic operations

On 03/20/2017 08:17 PM, Mark Rutland wrote:
> Hi,
> 
> On Tue, Mar 14, 2017 at 08:24:13PM +0100, Dmitry Vyukov wrote:
>>  /**
>> - * atomic_read - read atomic variable
>> + * arch_atomic_read - read atomic variable
>>   * @v: pointer of type atomic_t
>>   *
>>   * Atomically reads the value of @v.
>>   */
>> -static __always_inline int atomic_read(const atomic_t *v)
>> +static __always_inline int arch_atomic_read(const atomic_t *v)
>>  {
>> -	return READ_ONCE((v)->counter);
>> +	/*
>> +	 * We use READ_ONCE_NOCHECK() because atomic_read() contains KASAN
>> +	 * instrumentation. Double instrumentation is unnecessary.
>> +	 */
>> +	return READ_ONCE_NOCHECK((v)->counter);
>>  }
> 
> Just to check, we do this to avoid duplicate reports, right?
> 
> If so, double instrumentation isn't solely "unnecessary"; it has a
> functional difference, and we should explicitly describe that in the
> comment.
> 
> ... or are duplicate reports supressed somehow?
> 

They are not suppressed yet. But I think we should just switch kasan to single shot mode,
i.e. report only the first error. Single bug quite often has multiple invalid memory accesses
causing storm in dmesg. Also write OOB might corrupt metadata so the next report will print
bogus alloc/free stacktraces.
In most cases we need to look only at the first report, so reporting anything after the first
is just counterproductive.