| 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
| ||
|
Date: Thu, 3 Mar 2016 10:28:45 -0700 From: Khalid Aziz <khalid.aziz@...cle.com> To: Julian Calaby <julian.calaby@...il.com> Cc: "David S. Miller" <davem@...emloft.net>, corbet@....net, Andrew Morton <akpm@...ux-foundation.org>, dingel@...ux.vnet.ibm.com, zhenzhang.zhang@...wei.com, bob.picco@...cle.com, kirill.shutemov@...ux.intel.com, aneesh.kumar@...ux.vnet.ibm.com, aarcange@...hat.com, Arnd Bergmann <arnd@...db.de>, sparclinux <sparclinux@...r.kernel.org>, rob.gardner@...cle.com, mhocko@...e.cz, chris.hyser@...cle.com, richard@....at, vbabka@...e.cz, koct9i@...il.com, oleg@...hat.com, gthelen@...gle.com, jack@...e.cz, xiexiuqi@...wei.com, Vineet.Gupta1@...opsys.com, Andy Lutomirski <luto@...nel.org>, ebiederm@...ssion.com, bsegall@...gle.com, Geert Uytterhoeven <geert@...ux-m68k.org>, dave@...olabs.net, Alexey Dobriyan <adobriyan@...il.com>, linux-doc@...r.kernel.org, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, linux-mm@...ck.org, linux-arch@...r.kernel.org, linux-api@...r.kernel.org Subject: Re: [PATCH v2] sparc64: Add support for Application Data Integrity (ADI) On 03/02/2016 05:48 PM, Julian Calaby wrote: > Hi Khalid, > > On Thu, Mar 3, 2016 at 11:25 AM, Khalid Aziz <khalid.aziz@...cle.com> wrote: >> Thanks, Julian! I really appreciate your feedback. > > No problem! > >> My comments below. >> >> On 03/02/2016 04:08 PM, Julian Calaby wrote: >>> >>> Hi Khalid, >>> >>> On Thu, Mar 3, 2016 at 7:39 AM, Khalid Aziz <khalid.aziz@...cle.com> >>> wrote: >>>> >>>> >>>> Enable Application Data Integrity (ADI) support in the sparc >>>> kernel for applications to use ADI in userspace. ADI is a new >>>> feature supported on sparc M7 and newer processors. ADI is supported >>>> for data fetches only and not instruction fetches. This patch adds >>>> prctl commands to enable and disable ADI (TSTATE.mcde), return ADI >>>> parameters to userspace, enable/disable MCD (Memory Corruption >>>> Detection) on selected memory ranges and enable TTE.mcd in PTEs. It >>>> also adds handlers for all traps related to MCD. ADI is not enabled >>>> by default for any task and a task must explicitly enable ADI >>>> (TSTATE.mcde), turn MCD on on a memory range and set version tag >>>> for ADI to be effective for the task. This patch adds support for >>>> ADI for hugepages only. Addresses passed into system calls must be >>>> non-ADI tagged addresses. >>> >>> >>> I can't comment on the actual functionality here, but I do see a few >>> minor style issues in your patch. >>> >>> My big concern is that you're defining a lot of new code that is ADI >>> specific but isn't inside a CONFIG_SPARC_ADI ifdef. (That said, >>> handling ADI specific traps if ADI isn't enabled looks like a good >>> idea to me, however most of the other stuff is just dead code if >>> CONFIG_SPARC_ADI isn't enabled.) >> >> >> Some of the code will be executed when CONFIG_SPARC_ADI is not enabled, for >> instance init_adi() which will parse machine description to determine if >> platform supports ADI. On the other hand, it might still make sense to >> enclose this code in #ifdef. More on that below. >> >> >>> >>>> Signed-off-by: Khalid Aziz <khalid.aziz@...cle.com> >>>> --- >>>> NOTES: ADI is a new feature added to M7 processor to allow hardware >>>> to catch rogue accesses to memory. An app can enable ADI on >>>> its data pages, set version tags on them and use versioned >>>> addresses (bits 63-60 of the address contain a version tag) >>>> to access the data pages. If a rogue app attempts to access >>>> ADI enabled data pages, its access is blocked and processor >>>> generates an exception. Enabling this functionality for all >>>> data pages of an app requires adding infrastructure to save >>>> version tags for any data pages that get swapped out and >>>> restoring those tags when pages are swapped back in. In this >>>> first implementation I am enabling ADI for hugepages only >>>> since these pages are locked in memory and hence avoid the >>>> issue of saving and restoring tags. Once this core functionality >>>> is stable, ADI for other memory pages can be enabled more >>>> easily. >>>> >>>> v2: >>>> - Fixed a build error >>>> >>>> Documentation/prctl/sparc_adi.txt | 62 ++++++++++ >>>> Documentation/sparc/adi.txt | 206 >>>> +++++++++++++++++++++++++++++++ >>>> arch/sparc/Kconfig | 12 ++ >>>> arch/sparc/include/asm/hugetlb.h | 14 +++ >>>> arch/sparc/include/asm/hypervisor.h | 2 + >>>> arch/sparc/include/asm/mmu_64.h | 1 + >>>> arch/sparc/include/asm/pgtable_64.h | 15 +++ >>>> arch/sparc/include/asm/processor_64.h | 19 +++ >>>> arch/sparc/include/asm/ttable.h | 10 ++ >>>> arch/sparc/include/uapi/asm/asi.h | 3 + >>>> arch/sparc/include/uapi/asm/pstate.h | 10 ++ >>>> arch/sparc/kernel/entry.h | 3 + >>>> arch/sparc/kernel/head_64.S | 1 + >>>> arch/sparc/kernel/mdesc.c | 81 +++++++++++++ >>>> arch/sparc/kernel/process_64.c | 222 >>>> ++++++++++++++++++++++++++++++++++ >>>> arch/sparc/kernel/sun4v_mcd.S | 16 +++ >>>> arch/sparc/kernel/traps_64.c | 96 ++++++++++++++- >>>> arch/sparc/kernel/ttable_64.S | 6 +- >>>> include/linux/mm.h | 2 + >>>> include/uapi/asm-generic/siginfo.h | 5 +- >>>> include/uapi/linux/prctl.h | 16 +++ >>>> kernel/sys.c | 30 +++++ >>>> 22 files changed, 826 insertions(+), 6 deletions(-) >>>> create mode 100644 Documentation/prctl/sparc_adi.txt >>>> create mode 100644 Documentation/sparc/adi.txt >>>> create mode 100644 arch/sparc/kernel/sun4v_mcd.S >>> >>> >>> I must admit that I'm slightly impressed that the documentation is >>> over a quarter of the lines added. =) >>> >>>> diff --git a/arch/sparc/Kconfig b/arch/sparc/Kconfig >>>> index 56442d2..0aac0ae 100644 >>>> --- a/arch/sparc/Kconfig >>>> +++ b/arch/sparc/Kconfig >>>> @@ -80,6 +80,7 @@ config SPARC64 >>>> select NO_BOOTMEM >>>> select HAVE_ARCH_AUDITSYSCALL >>>> select ARCH_SUPPORTS_ATOMIC_RMW >>>> + select SPARC_ADI >>> >>> >>> This doesn't look right. >>> >>>> config ARCH_DEFCONFIG >>>> string >>>> @@ -314,6 +315,17 @@ if SPARC64 >>>> source "kernel/power/Kconfig" >>>> endif >>>> >>>> +config SPARC_ADI >>>> + bool "Application Data Integrity support" >>>> + def_bool y if SPARC64 >>> >>> >>> def_bool is for config options without names (i.e. "this is a boolean >>> value and it's default is...") >>> >>> So if you want people to be able to disable this option, then you >>> should remove the select above and just have: >>> >>> bool "Application Data Integrity support" >>> default y if SPARC64 >>> >>> If you don't want people disabling it, then there's no point in having >>> a separate Kconfig symbol. >>> >> >> Ah, I see. I do not want people disabling it. I will make changes. > > Why don't you want people disabling it? I must acknowledge that it's > not a lot of code, but I can see people wanting to build "minimal" > kernels for processors without ADI or to run some specific thing that > doesn't use ADI. Providing the kernel responds appropriately if > there's an unexpected ADI fault I don't see why the code would be > needed if it'll never be used. > Hi Julian, My goal in making CONFIG_SPARC_ADI auto-selected was to not add yet another config option that end user has to understand and figure out what to do with, and make the kernel self-configuring where ADI simply becomes available if platform supports it. Kernel auto-detecting platform features is especially useful for distro kernels. I do see your point in being able to build a minimal kernel when building a custom kernel. Both options of making CONFIG_SPARC_ADI auto-selected or not, have pros and cons. I don't have a strong feeling about it one way or the other and can go either way. Thanks, Khalid
Powered by blists - more mailing lists