| 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
| ||
|
Message-ID: <56D78471.1020707@oracle.com>
Date: Wed, 2 Mar 2016 17:25:21 -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)
Thanks, Julian! I really appreciate your feedback.
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.
>> + help
>> + Support for Application Data Integrity (ADI). ADI feature allows
>> + a process to tag memory blocks with version tags. Once ADI is
>> + enabled and version tag is set on a memory block, any access to
>> + it is allowed only if the correct version tag is presented by
>> + a process. This feature is meant to help catch rogue accesses
>> + to memory.
>> +
>
> You should probably mention that it's only available on newer
> processors and recommend that it's enabled on them.
Good point.
>
> This code won't break anything on older processors, right? I haven't
> looked very closely, but I don't see anything that specifically
> disables the code if it's run on, say, a UltraSparc I.
Right, this code does not break anything on older processors and has
been tested on older machines. init_adi() will detect the platform does
not support ADI when it parses machine description and will leave ADI
disabled in that case (adi_state.enabled=false).
>
>> config SCHED_SMT
>> bool "SMT (Hyperthreading) scheduler support"
>> depends on SPARC64 && SMP
>> diff --git a/arch/sparc/include/asm/processor_64.h b/arch/sparc/include/asm/processor_64.h
>> index 6924bde..9a71701 100644
>> --- a/arch/sparc/include/asm/processor_64.h
>> +++ b/arch/sparc/include/asm/processor_64.h
>> @@ -97,6 +97,25 @@ struct thread_struct {
>> struct task_struct;
>> unsigned long thread_saved_pc(struct task_struct *);
>>
>> +#ifdef CONFIG_SPARC_ADI
>> +extern struct adi_caps *get_adi_caps(void);
>> +extern long get_sparc_adicaps(unsigned long);
>> +extern long set_sparc_pstate_mcde(unsigned long);
>> +extern long enable_sparc_adi(unsigned long, unsigned long);
>> +extern long disable_sparc_adi(unsigned long, unsigned long);
>> +extern long get_sparc_adi_status(unsigned long);
>> +extern bool adi_capable(void);
>> +
>> +#define GET_SPARC_ADICAPS(a) get_sparc_adicaps(a)
>> +#define SET_SPARC_MCDE(a) set_sparc_pstate_mcde(a)
>> +#define ENABLE_SPARC_ADI(a, b) enable_sparc_adi(a, b)
>> +#define DISABLE_SPARC_ADI(a, b) disable_sparc_adi(a, b)
>> +#define GET_SPARC_ADI_STATUS(a) get_sparc_adi_status(a)
>> +#define ADI_CAPABLE() adi_capable()
>
> Get rid of the ADI_CAPABLE macro, the usual pattern here is to define
> a static inline function for the entire API when the symbol is
> disabled, i.e.
>
> #ifdef CONFIG_SPARC_ADI
> ...
> extern bool adi_capable(void);
> #else
> ...
> static inline bool adi_capable(void) {
> return false;
> }
> #endif
>
> That way you get type checking on the arguments even if the option is
> disabled and modern compilers are smart enough to optimise all the
> no-op code away. (Not that the type checking is needed here.)
>
> Also, in all but one place you use the ADI_CAPABLE() macro when the
> adi_capable() function is defined and available.
I defined ADI_CAPABLE() 0 for the case when CONFIG_SPARC_ADI is not set
to help compiler optimize sun4v_mem_corrupt_detect_precise(). Since
sun4v_mem_corrupt_detect_precise() is exception handler, optimizing it
can be good for performance but perhaps compiler is smart enough to do
that any way if adi_capable() is defined inline as you show above? I do
like that doing it this way retains type checking.
>
>> +#else
>> +#define ADI_CAPABLE() 0
>> +#endif
>> +
>> /* On Uniprocessor, even in RMO processes see TSO semantics */
>> #ifdef CONFIG_SMP
>> #define TSTATE_INITIAL_MM TSTATE_TSO
>> diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
>> index 6f80936..79f981c 100644
>> --- a/arch/sparc/kernel/mdesc.c
>> +++ b/arch/sparc/kernel/mdesc.c
>> @@ -1007,6 +1013,80 @@ static int mdesc_open(struct inode *inode, struct file *file)
>> return 0;
>> }
>>
>> +bool adi_capable(void)
>> +{
>> + return adi_state.enabled;
>> +}
>> +
>> +struct adi_caps *get_adi_caps(void)
>> +{
>> + return &adi_state.caps;
>> +}
>> +
>> +void __init
>> +init_adi(void)
>> +{
>> + struct mdesc_handle *hp = mdesc_grab();
>> + const char *prop;
>> + u64 pn, *val;
>> + int len;
>> +
>> + adi_state.enabled = false;
>> +
>> + if (!hp)
>> + return;
>> +
>> + pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "cpu");
>> + if (pn == MDESC_NODE_NULL)
>> + goto out;
>> +
>> + prop = mdesc_get_property(hp, pn, "hwcap-list", &len);
>> + if (!prop)
>> + goto out;
>> +
>> + /*
>> + * Look for "adp" keyword in hwcap-list which would indicate
>> + * ADI support
>> + */
>> + while (len) {
>> + int plen;
>> +
>> + if (!strcmp(prop, "adp")) {
>> + adi_state.enabled = true;
>> + break;
>> + }
>> +
>> + plen = strlen(prop) + 1;
>> + prop += plen;
>> + len -= plen;
>> + }
>> +
>> + if (!adi_state.enabled)
>> + goto out;
>> +
>> + pn = mdesc_node_by_name(hp, MDESC_NODE_NULL, "platform");
>> + if (pn == MDESC_NODE_NULL)
>> + goto out;
>> +
>> + val = (u64 *) mdesc_get_property(hp, pn, "adp-blksz", &len);
>> + if (!val)
>> + goto out;
>> + adi_state.caps.blksz = *val;
>> +
>> + val = (u64 *) mdesc_get_property(hp, pn, "adp-nbits", &len);
>> + if (!val)
>> + goto out;
>> + adi_state.caps.nbits = *val;
>> +
>> + val = (u64 *) mdesc_get_property(hp, pn, "ue-on-adp", &len);
>> + if (!val)
>> + goto out;
>> + adi_state.caps.ue_on_adi = *val;
>> +
>> +out:
>> + mdesc_release(hp);
>> +}
>> +
>
> Should all the ADI related functions above be within a #ifdef CONFIG_SPARC_ADI?
>
CONFIG_SPARC_ADI is selected for 64-bit kernels only since M7 is 64-bit
only. init_adi() will do the right thing whether CONFIG_SPARC_ADI is
enabled or not. It will parse machine description on 32-bit kernels,
detect ADI is not supported by the platform and leave
adi_state.enabled=false. I was considering adding something like
/proc/sys/vm/sparc_adi_available at later point which would get its data
from what init_adi() detects. On the other hand, since 32-bit processors
do not support ADI, why have even this much code run on them. I can
enclose this code as well inside #ifdef.
>> static ssize_t mdesc_read(struct file *file, char __user *buf,
>> size_t len, loff_t *offp)
>> {
>> diff --git a/arch/sparc/kernel/traps_64.c b/arch/sparc/kernel/traps_64.c
>> index d21cd62..29db583 100644
>> --- a/arch/sparc/kernel/traps_64.c
>> +++ b/arch/sparc/kernel/traps_64.c
>> @@ -2531,6 +2589,38 @@ void sun4v_do_mna(struct pt_regs *regs, unsigned long addr, unsigned long type_c
>> force_sig_info(SIGBUS, &info, current);
>> }
>>
>> +void sun4v_mem_corrupt_detect_precise(struct pt_regs *regs, unsigned long addr,
>> + unsigned long context)
>> +{
>> + siginfo_t info;
>> +
>> + if (!ADI_CAPABLE()) {
>> + bad_trap(regs, 0x1a);
>> + return;
>> + }
>> +
>> + if (notify_die(DIE_TRAP, "memory corruption precise exception", regs,
>> + 0, 0x8, SIGSEGV) == NOTIFY_STOP)
>> + return;
>> +
>> + if (regs->tstate & TSTATE_PRIV) {
>> + pr_emerg("sun4v_mem_corrupt_detect_precise: ADDR[%016lx] "
>> + "CTX[%lx], going.\n", addr, context);
>> + die_if_kernel("MCD precise", regs);
>> + }
>> +
>> + if (test_thread_flag(TIF_32BIT)) {
>> + regs->tpc &= 0xffffffff;
>> + regs->tnpc &= 0xffffffff;
>> + }
>> + info.si_signo = SIGSEGV;
>> + info.si_code = SEGV_ADIPERR;
>> + info.si_errno = 0;
>> + info.si_addr = (void __user *) addr;
>> + info.si_trapno = 0;
>> + force_sig_info(SIGSEGV, &info, current);
>> +}
>> +
>
> Should this be ifdef'd too?
I would prefer to leave exception handlers in place any way unless there
are strong objections.
>
>> void do_privop(struct pt_regs *regs)
>> {
>> enum ctx_state prev_state = exception_enter();
>> diff --git a/kernel/sys.c b/kernel/sys.c
>> index 6af9212..fa7b5d9 100644
>> --- a/kernel/sys.c
>> +++ b/kernel/sys.c
>> @@ -103,6 +103,21 @@
>> #ifndef SET_FP_MODE
>> # define SET_FP_MODE(a,b) (-EINVAL)
>> #endif
>> +#ifndef GET_SPARC_ADICAPS
>> +# define GET_SPARC_ADICAPS(a) (-EINVAL)
>> +#endif
>> +#ifndef SET_SPARC_MCDE
>> +# define SET_SPARC_MCDE(a) (-EINVAL)
>> +#endif
>> +#ifndef ENABLE_SPARC_ADI
>> +# define ENABLE_SPARC_ADI(a, b) (-EINVAL)
>> +#endif
>> +#ifndef DISABLE_SPARC_ADI
>> +# define DISABLE_SPARC_ADI(a, b) (-EINVAL)
>> +#endif
>> +#ifndef GET_SPARC_ADI_STATUS
>> +# define GET_SPARC_ADI_STATUS(a) (-EINVAL)
>> +#endif
>
> Ah, I was wondering why you were defining macros in processor_64.h.
>
>> /*
>> * this is where the system-wide overflow UID and GID are defined, for
>
> I've got a couple more comments, I'll send another email with them shortly.
>
> Thanks,
>
Thanks,
Khalid
Powered by blists - more mailing lists