linux-kernel - Re: bugs in page colouring code

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]

Date:	Thu, 14 Jun 2012 10:27:55 -0400
From:	Rik van Riel <riel@...hat.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
CC:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, sjhill@...s.com, ralf@...ux-mips.org,
	Borislav Petkov <borislav.petkov@....com>,
	"H. Peter Anvin" <hpa@...ux.intel.com>,
	Rob Herring <rob.herring@...xeda.com>,
	Nicolas Pitre <nico@...aro.org>
Subject: Re: bugs in page colouring code

On 06/14/2012 09:20 AM, Russell King - ARM Linux wrote:
> On Wed, Jun 13, 2012 at 03:29:36PM -0400, Rik van Riel wrote:
>> COLOUR_ALIGN_DOWN can use the pgoff % shm_align_mask either positively
>>     or negatively, depending on the address initially found by
>>     get_unmapped_area
>>
>> static inline unsigned long COLOUR_ALIGN_DOWN(unsigned long addr,
>>                                                unsigned long pgoff)
>> {
>>          unsigned long base = addr&  ~shm_align_mask;
>>          unsigned long off = (pgoff<<  PAGE_SHIFT)&  shm_align_mask;
>>
>>          if (base + off<= addr)
>>                  return base + off;
>>
>>          return base - off;
>> }
>
> Yes, that is bollocks code, introduced by this commit:
>
> commit 7dbaa466780a754154531b44c2086f6618cee3a8
> Author: Rob Herring<rob.herring@...xeda.com>
> Date:   Tue Nov 22 04:01:07 2011 +0100

It's not just ARM that has this bug. It appears to
be cut'n'pasted from other architectures (MIPS? SPARC?).

>> The fix would be to return an address that is a whole shm_align_mask
>> lower: (((base - shm_align_mask)&  ~shm_align_mask) + off
>
> Yes, agreed.

I will make sure the arch-independent colouring
code does that.

> This brings up the question: should a MAP_PRIVATE mapping see updates
> to the backing file made via a shared mapping and/or writing the file
> directly?  After all, a r/w MAP_PRIVATE mapping which has been CoW'd
> won't see the updates.
>
> So I'd argue that a file mapped MAP_SHARED must be mapped according to
> the colour rules, but a MAP_PRIVATE is free not to be so.

OK, fair enough.

>> Secondly, MAP_FIXED never checks for page colouring alignment.
>> I assume the cache aliasing on AMD Bulldozer is merely a performance
>> issue, and we can simply ignore page colouring for MAP_FIXED?
>> That will be easy to get right in an architecture-independent
>> implementation.
>
> There's a whole bunch of issues with MAP_FIXED, specifically address
> space overflow has been discussed previously, and resulted in this patch:
>
> [PATCH 0/6] get rid of extra check for TASK_SIZE in get_unmapped_area

Turns out, get_unmapped_area_prot (the function
that calls arch_get_unmapped_area) checks for
these overflows, so we should be fine.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/