| 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: <fc65fc29-4cd8-4e41-93e4-a35e3c8998d8@sifive.com>
Date: Wed, 14 Aug 2024 02:06:43 -0500
From: Samuel Holland <samuel.holland@...ive.com>
To: Alexandre Ghiti <alex@...ti.fr>, Palmer Dabbelt <palmer@...belt.com>,
linux-riscv@...ts.infradead.org
Cc: devicetree@...r.kernel.org, Catalin Marinas <catalin.marinas@....com>,
linux-kernel@...r.kernel.org, Anup Patel <anup@...infault.org>,
Conor Dooley <conor@...nel.org>, kasan-dev@...glegroups.com,
Atish Patra <atishp@...shpatra.org>, Evgenii Stepanov <eugenis@...gle.com>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Rob Herring <robh+dt@...nel.org>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>
Subject: Re: [PATCH v2 04/10] riscv: Add support for userspace pointer masking
On 2024-08-13 8:54 PM, Samuel Holland wrote:
> Hi Alex,
>
> Thanks for the review!
>
> On 2024-08-13 3:58 AM, Alexandre Ghiti wrote:
>> Hi Samuel,
>>
>> On 25/06/2024 23:09, Samuel Holland wrote:
>>> RISC-V supports pointer masking with a variable number of tag bits
>>> (which is called "PMLEN" in the specification) and which is configured
>>> at the next higher privilege level.
>>>
>>> Wire up the PR_SET_TAGGED_ADDR_CTRL and PR_GET_TAGGED_ADDR_CTRL prctls
>>> so userspace can request a lower bound on the number of tag bits and
>>> determine the actual number of tag bits. As with arm64's
>>> PR_TAGGED_ADDR_ENABLE, the pointer masking configuration is
>>> thread-scoped, inherited on clone() and fork() and cleared on execve().
>>>
>>> Signed-off-by: Samuel Holland <samuel.holland@...ive.com>
>>> ---
>>>
>>> Changes in v2:
>>> - Rebase on riscv/linux.git for-next
>>> - Add and use the envcfg_update_bits() helper function
>>> - Inline flush_tagged_addr_state()
>>>
>>> arch/riscv/Kconfig | 11 ++++
>>> arch/riscv/include/asm/processor.h | 8 +++
>>> arch/riscv/include/asm/switch_to.h | 11 ++++
>>> arch/riscv/kernel/process.c | 99 ++++++++++++++++++++++++++++++
>>> include/uapi/linux/prctl.h | 3 +
>>> 5 files changed, 132 insertions(+)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index b94176e25be1..8f9980f81ea5 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -505,6 +505,17 @@ config RISCV_ISA_C
>>> If you don't know what to do here, say Y.
>>> +config RISCV_ISA_POINTER_MASKING
>>> + bool "Smmpm, Smnpm, and Ssnpm extensions for pointer masking"
>>> + depends on 64BIT
>>> + default y
>>> + help
>>> + Add support for the pointer masking extensions (Smmpm, Smnpm,
>>> + and Ssnpm) when they are detected at boot.
>>> +
>>> + If this option is disabled, userspace will be unable to use
>>> + the prctl(PR_{SET,GET}_TAGGED_ADDR_CTRL) API.
>>> +
>>> config RISCV_ISA_SVNAPOT
>>> bool "Svnapot extension support for supervisor mode NAPOT pages"
>>> depends on 64BIT && MMU
>>> diff --git a/arch/riscv/include/asm/processor.h
>>> b/arch/riscv/include/asm/processor.h
>>> index 0838922bd1c8..4f99c85d29ae 100644
>>> --- a/arch/riscv/include/asm/processor.h
>>> +++ b/arch/riscv/include/asm/processor.h
>>> @@ -194,6 +194,14 @@ extern int set_unalign_ctl(struct task_struct *tsk,
>>> unsigned int val);
>>> #define RISCV_SET_ICACHE_FLUSH_CTX(arg1, arg2)
>>> riscv_set_icache_flush_ctx(arg1, arg2)
>>> extern int riscv_set_icache_flush_ctx(unsigned long ctx, unsigned long
>>> per_thread);
>>> +#ifdef CONFIG_RISCV_ISA_POINTER_MASKING
>>> +/* PR_{SET,GET}_TAGGED_ADDR_CTRL prctl */
>>> +long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg);
>>> +long get_tagged_addr_ctrl(struct task_struct *task);
>>> +#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(current, arg)
>>> +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl(current)
>>> +#endif
>>> +
>>> #endif /* __ASSEMBLY__ */
>>> #endif /* _ASM_RISCV_PROCESSOR_H */
>>> diff --git a/arch/riscv/include/asm/switch_to.h
>>> b/arch/riscv/include/asm/switch_to.h
>>> index 9685cd85e57c..94e33216b2d9 100644
>>> --- a/arch/riscv/include/asm/switch_to.h
>>> +++ b/arch/riscv/include/asm/switch_to.h
>>> @@ -70,6 +70,17 @@ static __always_inline bool has_fpu(void) { return false; }
>>> #define __switch_to_fpu(__prev, __next) do { } while (0)
>>> #endif
>>> +static inline void envcfg_update_bits(struct task_struct *task,
>>> + unsigned long mask, unsigned long val)
>>> +{
>>> + unsigned long envcfg;
>>> +
>>> + envcfg = (task->thread.envcfg & ~mask) | val;
>>> + task->thread.envcfg = envcfg;
>>> + if (task == current)
>>> + csr_write(CSR_ENVCFG, envcfg);
>>> +}
>>> +
>>> static inline void __switch_to_envcfg(struct task_struct *next)
>>> {
>>> asm volatile (ALTERNATIVE("nop", "csrw " __stringify(CSR_ENVCFG) ", %0",
>>> diff --git a/arch/riscv/kernel/process.c b/arch/riscv/kernel/process.c
>>> index e4bc61c4e58a..dec5ccc44697 100644
>>> --- a/arch/riscv/kernel/process.c
>>> +++ b/arch/riscv/kernel/process.c
>>> @@ -7,6 +7,7 @@
>>> * Copyright (C) 2017 SiFive
>>> */
>>> +#include <linux/bitfield.h>
>>> #include <linux/cpu.h>
>>> #include <linux/kernel.h>
>>> #include <linux/sched.h>
>>> @@ -171,6 +172,10 @@ void flush_thread(void)
>>> memset(¤t->thread.vstate, 0, sizeof(struct __riscv_v_ext_state));
>>> clear_tsk_thread_flag(current, TIF_RISCV_V_DEFER_RESTORE);
>>> #endif
>>> +#ifdef CONFIG_RISCV_ISA_POINTER_MASKING
>>> + if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
>>> + envcfg_update_bits(current, ENVCFG_PMM, ENVCFG_PMM_PMLEN_0);
>>> +#endif
>>
>> if (IS_ENABLED(CONFIG_RISCV_ISA_POINTER_MASKING) &&
>> riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
>
> I will update this.
>
>>> }
>>> void arch_release_task_struct(struct task_struct *tsk)
>>> @@ -233,3 +238,97 @@ void __init arch_task_cache_init(void)
>>> {
>>> riscv_v_setup_ctx_cache();
>>> }
>>> +
>>> +#ifdef CONFIG_RISCV_ISA_POINTER_MASKING
>>> +static bool have_user_pmlen_7;
>>> +static bool have_user_pmlen_16;
>>> +
>>> +long set_tagged_addr_ctrl(struct task_struct *task, unsigned long arg)
>>> +{
>>> + unsigned long valid_mask = PR_PMLEN_MASK;
>>> + struct thread_info *ti = task_thread_info(task);
>>> + unsigned long pmm;
>>> + u8 pmlen;
>>> +
>>> + if (is_compat_thread(ti))
>>> + return -EINVAL;
>>> +
>>> + if (arg & ~valid_mask)
>>> + return -EINVAL;
>>> +
>>> + pmlen = FIELD_GET(PR_PMLEN_MASK, arg);
>>> + if (pmlen > 16) {
>>> + return -EINVAL;
>>> + } else if (pmlen > 7) {
>>> + if (have_user_pmlen_16)
>>> + pmlen = 16;
>>> + else
>>> + return -EINVAL;
>>> + } else if (pmlen > 0) {
>>> + /*
>>> + * Prefer the smallest PMLEN that satisfies the user's request,
>>> + * in case choosing a larger PMLEN has a performance impact.
>>> + */
>>> + if (have_user_pmlen_7)
>>> + pmlen = 7;
>>> + else if (have_user_pmlen_16)
>>> + pmlen = 16;
>>> + else
>>> + return -EINVAL;
>>> + }
>>> +
>>> + if (pmlen == 7)
>>> + pmm = ENVCFG_PMM_PMLEN_7;
>>> + else if (pmlen == 16)
>>> + pmm = ENVCFG_PMM_PMLEN_16;
>>> + else
>>> + pmm = ENVCFG_PMM_PMLEN_0;
>>> +
>>> + envcfg_update_bits(task, ENVCFG_PMM, pmm);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +long get_tagged_addr_ctrl(struct task_struct *task)
>>> +{
>>> + struct thread_info *ti = task_thread_info(task);
>>> + long ret = 0;
>>> +
>>> + if (is_compat_thread(ti))
>>> + return -EINVAL;
>>> +
>>> + switch (task->thread.envcfg & ENVCFG_PMM) {
>>> + case ENVCFG_PMM_PMLEN_7:
>>> + ret |= FIELD_PREP(PR_PMLEN_MASK, 7);
>>> + break;
>>> + case ENVCFG_PMM_PMLEN_16:
>>> + ret |= FIELD_PREP(PR_PMLEN_MASK, 16);
>>> + break;
>>> + }
>>
>>
>> No need for the |=
>
> This is used in the next patch since the returned value may include
> PR_TAGGED_ADDR_ENABLE as well, but it's not needed here, so I will make this change.
>
>>> +
>>> + return ret;
>>> +}
>>
>>
>> In all the code above, I'd use a macro for 7 and 16, something like PMLEN[7|16]?
>
> I've done this using an enum in v4. Please let me know if it looks good to you.
Obviously I meant to say v3 here.
>>> +
>>> +static bool try_to_set_pmm(unsigned long value)
>>> +{
>>> + csr_set(CSR_ENVCFG, value);
>>> + return (csr_read_clear(CSR_ENVCFG, ENVCFG_PMM) & ENVCFG_PMM) == value;
>>> +}
>>> +
>>> +static int __init tagged_addr_init(void)
>>> +{
>>> + if (!riscv_has_extension_unlikely(RISCV_ISA_EXT_SUPM))
>>> + return 0;
>>> +
>>> + /*
>>> + * envcfg.PMM is a WARL field. Detect which values are supported.
>>> + * Assume the supported PMLEN values are the same on all harts.
>>> + */
>>> + csr_clear(CSR_ENVCFG, ENVCFG_PMM);
>>> + have_user_pmlen_7 = try_to_set_pmm(ENVCFG_PMM_PMLEN_7);
>>> + have_user_pmlen_16 = try_to_set_pmm(ENVCFG_PMM_PMLEN_16);
>>
>>
>> Shouldn't this depend on the satp mode? sv57 does not allow 16bits for the tag.
>
> No, late last year the pointer masking spec was changed so that the valid values
> for PMM can no longer dynamically depend on satp.MODE. If an implementation
> chooses to support both Sv57 and PMLEN==16, then it does so by masking off some
> of the valid bits in the virtual address. (This is a valid if unusual use case
> considering that pointer masking does not apply to instruction fetches, so an
> application could place code at addresses above 2^47-1 and use the whole masked
> virtual address space for data. Or it could enable pointer masking for only
> certain threads, and those threads would be limited to a subset of data.)
>
>>> +
>>> + return 0;
>>> +}
>>> +core_initcall(tagged_addr_init);
>>
>>
>> Any reason it's not called from setup_arch()? I see the vector extension does
>> the same; just wondering if we should not centralize all this early extensions
>> decisions in setup_arch() (in my Zacas series, I choose the spinlock
>> implementation in setup_arch()).
Forgot to reply: no special reason, I copied this part of the code from arm64.
This code doesn't need to be called especially early; the only requirement is
that it runs before userspace starts. One advantage of core_initcall() is that
it happens after SMP bringup, so this way will have less impact on boot time.
Regards,
Samuel
>>> +#endif /* CONFIG_RISCV_ISA_POINTER_MASKING */
>>> diff --git a/include/uapi/linux/prctl.h b/include/uapi/linux/prctl.h
>>> index 35791791a879..6e84c827869b 100644
>>> --- a/include/uapi/linux/prctl.h
>>> +++ b/include/uapi/linux/prctl.h
>>> @@ -244,6 +244,9 @@ struct prctl_mm_map {
>>> # define PR_MTE_TAG_MASK (0xffffUL << PR_MTE_TAG_SHIFT)
>>> /* Unused; kept only for source compatibility */
>>> # define PR_MTE_TCF_SHIFT 1
>>> +/* RISC-V pointer masking tag length */
>>> +# define PR_PMLEN_SHIFT 24
>>> +# define PR_PMLEN_MASK (0x7fUL << PR_PMLEN_SHIFT)
>>
>>
>> I don't understand the need for this shift, can't userspace pass the pmlen value
>> directly without worrying about this?
>
> No, because the PR_TAGGED_ADDR_ENABLE flag (bit 0, defined just a few lines
> above) is part of the the same argument word. It's just not used until the next
> patch.
>
> Regards,
> Samuel
>
Powered by blists - more mailing lists