[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAGXu5jKcg7wwrh_8vxVRnvwsXKofpvda=ZmoaUk8epPHkwMdWg@mail.gmail.com>
Date: Tue, 1 Dec 2015 11:15:47 -0800
From: Kees Cook <keescook@...omium.org>
To: Laura Abbott <labbott@...hat.com>
Cc: Russell King <linux@....linux.org.uk>, Ard Biesheuvel <ard.biesheuvel@...aro.org>,
Will Deacon <will.deacon@....com>, Nicolas Pitre <nico@...aro.org>, Arnd Bergmann <arnd@...db.de>,
Catalin Marinas <catalin.marinas@....com>,
"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, LKML <linux-kernel@...r.kernel.org>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: [PATCH] ARM: mm: flip priority of CONFIG_DEBUG_RODATA
On Mon, Nov 30, 2015 at 5:20 PM, Laura Abbott <labbott@...hat.com> wrote:
> On 11/30/2015 05:08 PM, Kees Cook wrote:
>>
>> On Mon, Nov 30, 2015 at 5:03 PM, Laura Abbott <labbott@...hat.com> wrote:
>>>
>>> On 11/30/2015 03:38 PM, Kees Cook wrote:
>>>>
>>>>
>>>> Given the choice between making things NX or making things RO, we want
>>>> RO first. As such, redefine CONFIG_DEBUG_RODATA to actually do the bulk
>>>
>>>
>>>
>>> Can you give a citation for why? The thread that inspired it might be
>>> a good link.
>>
>>
>> This was inspired by my examining the existing architecture's
>> implementations of CONFIG_DEBUG_RODATA after Ingo suggested it be made
>> a common feature not a build-time config (or at least renamed):
>> http://www.openwall.com/lists/kernel-hardening/2015/11/30/13
>>
>
> Thanks. I read the thread and I think it would be good to put a link
> in the commit message to make it clearer why this is going in.
Okay, I will add this (and a summary, as Nicolas suggested).
>>>> index 41218867a9a6..b617084e9520 100644
>>>> --- a/arch/arm/mm/Kconfig
>>>> +++ b/arch/arm/mm/Kconfig
>>>> @@ -1039,24 +1039,26 @@ config ARCH_SUPPORTS_BIG_ENDIAN
>>>> This option specifies the architecture can support big endian
>>>> operation.
>>>>
>>>> -config ARM_KERNMEM_PERMS
>>>> - bool "Restrict kernel memory permissions"
>>>> +config DEBUG_RODATA
>>>> + bool "Make kernel text and rodata read-only"
>>>> depends on MMU
>>>> + default y if CPU_V7
>>>> help
>>>> - If this is set, kernel memory other than kernel text (and
>>>> rodata)
>>>> - will be made non-executable. The tradeoff is that each region
>>>> is
>>>> - padded to section-size (1MiB) boundaries (because their
>>>> permissions
>>>> - are different and splitting the 1M pages into 4K ones causes
>>>> TLB
>>>> - performance problems), wasting memory.
>>>> + If this is set, kernel memory (text, rodata, etc) will be made
>>>> + read-only, and non-text kernel memory will be made
>>>> non-executable.
>>>> + The tradeoff is that each region is padded to section-size
>>>> (1MiB)
>>>> + boundaries (because their permissions are different and
>>>> splitting
>>>> + the 1M pages into 4K ones causes TLB performance problems),
>>>> which
>>>> + can waste memory.
>>>>
>>>> -config DEBUG_RODATA
>>>> - bool "Make kernel text and rodata read-only"
>>>> - depends on ARM_KERNMEM_PERMS
>>>> +config DEBUG_ALIGN_RODATA
>>>> + bool "Make rodata strictly non-executable"
>>>> + depends on DEBUG_RODATA
>>>> default y
>>>> help
>>>> - If this is set, kernel text and rodata will be made read-only.
>>>> This
>>>> - is to help catch accidental or malicious attempts to change
>>>> the
>>>> - kernel's executable code. Additionally splits rodata from
>>>> kernel
>>>> - text so it can be made explicitly non-executable. This creates
>>>> - another section-size padded region, so it can waste more
>>>> memory
>>>> - space while gaining the read-only protections.
>>>> + If this is set, rodata will be made explicitly non-executable.
>>>> This
>>>> + provides protection on the rare chance that attackers might
>>>> find
>>>> and
>>>> + use ROP gadgets that exist in the rodata section. This adds an
>>>> + additional section-aligned split of rodata from kernel text so
>>>> it
>>>> + can be made explicitly non-executable. This padding may waste
>>>> memory
>>>> + space to gain this additional protection.
>>>
>>>
>>>
>>> I get that you want to make this match arm64 but it's really not
>>> intuitive that
>>> something with ALIGN_RODATA in the name is actually for setting NX. The
>>> purpose
>>> of ALIGN_RODATA was also slightly different on arm64 since the RO/NX will
>>> still
>>> be there, the difference is if the sections are present versus broken
>>> down into
>>> pages.
>>
>>
>> Well, it seems to have the same effect: without the alignment, a
>> portion of rodata may remain executable on arm64. Unless I
>> misunderstand?
>>
>
> No, on arm64 everything should always be NX, the difference is part of the
> NX
> sections may be mapped as pages instead of sections so you take the TLB hit.
> It's a trade off of memory vs TLB pressure instead of just security vs TLB.
Ah! I understand now. I will clarify the commit and Kconfig text on
ARM. I'd still prefer to keep the name, since it's still doing
section-alignment of rodata, it's just that without ARM's
ALIGN_RODATA, you get an executable rodata section.
-Kees
--
Kees Cook
Chrome OS & Brillo Security
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
