linux-kernel - Re: Problem with atomic accesses in pstore on some ARM CPUs

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 16 Aug 2016 17:26:40 -0700
From:	Guenter Roeck <groeck@...gle.com>
To:	Kees Cook <keescook@...omium.org>
Cc:	Colin Cross <ccross@...roid.com>,
	Mark Rutland <mark.rutland@....com>,
	Will Deacon <will.deacon@....com>,
	Tony Luck <tony.luck@...el.com>,
	Jeffy Chen <jeffy.chen@...k-chips.com>,
	Douglas Anderson <dianders@...omium.org>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Robin Murphy <robin.murphy@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Tony Lindgren <tony@...mide.com>,
	Rob Herring <robh@...nel.org>,
	Anton Vorontsov <anton@...msg.org>
Subject: Re: Problem with atomic accesses in pstore on some ARM CPUs

On Tue, Aug 16, 2016 at 1:50 PM, Kees Cook <keescook@...omium.org> wrote:

[ ... ]

>>> persistent_ram uses atomic ops in uncached memory to store the start
>>> and end positions in the ringbuffer so that the state of the
>>> ringbuffer will be valid if the kernel crashes at any time.  This was
>>> inherited from Android's ram_console implementation, and worked
>>> through armv7.  It has been causing more and more problems recently,
>>> see for example 027bc8b08242c59e19356b4b2c189f2d849ab660 (pstore-ram:
>>> Allow optional mapping with pgprot_noncached) and
>>> 7ae9cb81933515dc7db1aa3c47ef7653717e3090 (pstore-ram: Fix hangs by
>>> using write-combine mappings).
>>>
>>> Maybe it should be replaced with a spinlock in normal ram protecting
>>> writes to the uncached region.
>>
>> The necessary functions already exist, and are used for memory mapped
>> with ioremap() / ioremap_wc(). They were introduced with commit
>> 0405a5cec3 ("pstore/ram: avoid atomic accesses for ioremapped
>> regions"), and the description in that patch sounds quite similar to
>> the current problem. Given that, would it be acceptable to remove
>> buffer_start_add_atomic() and buffer_size_add_atomic(), and always use
>> buffer_start_add_locked() and buffer_size_add_locked() instead ? Those
>> functions still use atomic_set() and atomic_read(), which works fine
>> in my tests. The only difference is that a spinlock in main memory is
>> used instead of atomic_cmpxchg().
>
> I don't see much of a down side to this. ramoops isn't expected to be
> high-bandwidth so trading for a single global lock doesn't really
> bother me.
>

Sounds good. I'll submit a patch to address the problem as suggested above.

Guenter