Message-ID: <3be862ef-f8b7-40d6-a3b1-5e8161801ba1@ghiti.fr>
Date: Wed, 21 May 2025 16:42:56 +0200
From: Alexandre Ghiti <alex@...ti.fr>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
 Palmer Dabbelt <palmer@...belt.com>
Cc: jeffxu@...omium.org, jszhang@...nel.org, akpm@...ux-foundation.org,
 Liam.Howlett@...cle.com, kees@...nel.org,
 Paul Walmsley <paul.walmsley@...ive.com>, aou@...s.berkeley.edu,
 linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
 linux-mm@...ck.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] riscv: enable mseal sysmap for RV64

Hi Lorenzo,

On 5/8/25 20:08, Lorenzo Stoakes wrote:
> On Thu, May 08, 2025 at 10:39:35AM -0700, Palmer Dabbelt wrote:
>> On Wed, 07 May 2025 09:22:09 PDT (-0700), lorenzo.stoakes@...cle.com wrote:
>>> On Wed, May 07, 2025 at 09:18:31AM -0700, Jeff Xu wrote:
>>>> Hi Jisheng
>>>>
>>>> It seems mm maintainers might prefer arch change reviewed by arch
>>>> maintainer and goes to arch tree, according to discussion in [1], I
>>>> don't have an opinion on this,  adding mm maintainers as FYI.
>>> Thanks Jeff, appreciate the ping!
>>>
>>> Jisheng - the main point here is to ensure that the arch doesn't rely in any way
>>> on, within the arch code itself, relocating any of these mappings. It's pretty
>>> easy to eyeball it and get a sense.
>>>
>>> Because if this is the case, the arch will be broken by this change should a
>>> user enable it, and obviously we'd rather avoid that! :)
>>>
>>> It's really likely that you're fine, as it'd be unusual for an arch to do this,
>>> but I strongly suggest you do so.
>>>
>>> And yes, I think these should really go through arch trees as explicitly arch
>>> code.
>>>
>>> Thanks, Lorenzo
>>>
>>>> On Sat, Apr 26, 2025 at 7:16 AM Jisheng Zhang <jszhang@...nel.org> wrote:
>>>>> Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS for RV64, covering the
>>>>> vdso, vvar.
>>>>>
>>>>> Passed sysmap_is_sealed and mseal_test self tests.
>>>>> Passed booting a buildroot rootfs image and a cli debian rootfs image.
>>>>>
>>>> mm maintainers like to get confirmation that the arch doesn't rely on
>>>> remapping the VDSO, VVAR, or any other special mappings, see
>>>> discussion in [2]
>> Do you have some description of what remapping is disallowed here?  There's
>> not a ton in that referenced thread.
> Any remapping of these special VMAs that would be disallowed by mseal that is
> being performed by kernel code.
>
>> We play a few tricks with remapping, including some aliasing to handle
>> different VA widths and text patching (via poke pages).  IIRC those are
>> similar in spirit to what's going on in x86/arm64 land, though sometimes the
>> exact flavor of the trick matters.  If you've got something I can look at it
>> might save me from having to read though the mm code...
> To risk sounding grumpy, and it's not your fault of course, but... this is
> the... fifth? Architecture where somebody's enabled this without seeming to
> understand what the feature does, and I'm a bit tired of it. And every time it's
> been _me_ who goes to check :)
>
> Anyway, I guess let me go check again.
>
> So it looks fine to me - the initial mapping to mm->context.vdso done by
> __setup_additional_pages() in arch/riscv/kernel/vdso.c appears to be final,
> except when vdso_mremap() is invoked, but that's when _userland_ does it,
> which this feature strictly disables, but that's fine.
>
> Note that the patch simply indicates that the architecture _supports_ this
> feature, the feature itself is disabled by default as it breaks a bunch of
> userland, which is why I insisted on it being put behind a config flag for
> users who really know what they're doing.
>
> So perhaps I can link to this again in future, what I'm talking about is:
>
> - Any instance of the architecture code, NOT at the behest of userland
>    (which we expect the sealing to break), but for its OWN purposes, for
>    whatever reason, moving the VDSO, VVAR or other special mapping around in
>    virtual memory.
>
> - So you'd really need to be doing something _weird_ for this to be the
>    case. It's unlikely but until each architecture is checked we are being
>    conservative - so as not to break the kernel (!) - and disabling
>    architectures until we know they're safe.
>
> - Sending a patch setting ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS is _very
>    explicitly_ stating that you have checked and are certain there are _no
>    problems_ with the architecture doing this.
>
> So this is why it's quite annoying to have people enable this with a commit
> message like 'enable mseal system mappings' rather than 'I have carefully
> checked the architecture code and the architecture does not manipulate the
> VDSO, VVAR or special mappings after establishing them, I have also tested
> the feature in practice'.
>
> I feel like anybody setting 'ARCH_SUPPORTS_xxx' should - you know - have
> determined that the architecture supports xxx. But maybe that's
> unreasonable...
>
>> and ya, we'll pick it up through the arch tree once one of us can be
>> convinced this works ;)
>>
> Thanks! :) Your skepticism is appropriate. But I do think this will be fine
> for risc v.
>
> I'd ideally like to hear from somebody who's checked this on a risc v
> system with CONFIG_MSEAL_SYSTEM_MAPPINGS enabled just to be doubly sure.


So I double checked and tested this so I'll merge it for 6.16 and:

Tested-by: Alexandre Ghiti <alexghiti@...osinc.com>

Thanks,

Alex


>
> Thanks, Lorenzo
>
>>>>> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
>>>>> Cc: Jeff Xu <jeffxu@...omium.org>
>>>>> ---
>>>>>   arch/riscv/Kconfig       | 1 +
>>>>>   arch/riscv/kernel/vdso.c | 2 +-
>>>>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>>> Please consider updating document as part of your patch:
>>>> features/core/mseal_sys_mappings/arch-support.txt
>>>> Documentation/userspace-api/mseal.rst
>>>>
>>>> Sample change in [3]
>>>>
>>>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>>>> index bbec87b79309..3cb0b05eef62 100644
>>>>> --- a/arch/riscv/Kconfig
>>>>> +++ b/arch/riscv/Kconfig
>>>>> @@ -70,6 +70,7 @@ config RISCV
>>>>>          # LLD >= 14: https://github.com/llvm/llvm-project/issues/50505
>>>>>          select ARCH_SUPPORTS_LTO_CLANG if LLD_VERSION >= 140000
>>>>>          select ARCH_SUPPORTS_LTO_CLANG_THIN if LLD_VERSION >= 140000
>>>>> +       select ARCH_SUPPORTS_MSEAL_SYSTEM_MAPPINGS if 64BIT && MMU
>>>> The "if 64BIT && MMU" are not needed here.
>>>>
>>>> MMU is not checked by MSEAL_SYSTEM_MAPPINGS, which we should,  this
>>>> can go to security/Kconfig separately. If you'd like, please submit a
>>>> fix to mm tree directly.
>>>>
>>>> [1] https://lore.kernel.org/all/7EB087B72C4FBDD3+20250417132410.404043-1-wangyuli@uniontech.com/,
>>>> [2] https://lore.kernel.org/all/3de559d6-be19-44bc-ba8f-4c52d4bca684@lucifer.local/
>>>> [3] https://lore.kernel.org/all/648AB3031B5618C0+20250415153903.570662-1-wangyuli@uniontech.com/
>>>>
>>>> Thanks
>>>> -Jeff
>>>>
>>>>>          select ARCH_SUPPORTS_PAGE_TABLE_CHECK if MMU
>>>>>          select ARCH_SUPPORTS_PER_VMA_LOCK if MMU
>>>>>          select ARCH_SUPPORTS_RT
>>>>> diff --git a/arch/riscv/kernel/vdso.c b/arch/riscv/kernel/vdso.c
>>>>> index cc2895d1fbc2..3a8e038b10a2 100644
>>>>> --- a/arch/riscv/kernel/vdso.c
>>>>> +++ b/arch/riscv/kernel/vdso.c
>>>>> @@ -136,7 +136,7 @@ static int __setup_additional_pages(struct mm_struct *mm,
>>>>>
>>>>>          ret =
>>>>>             _install_special_mapping(mm, vdso_base, vdso_text_len,
>>>>> -               (VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC),
>>>>> +               (VM_READ | VM_EXEC | VM_MAYREAD | VM_MAYWRITE | VM_MAYEXEC | VM_SEALED_SYSMAP),
>>>>>                  vdso_info->cm);
>>>>>
>>>>>          if (IS_ERR(ret))
>>>>> --
>>>>> 2.47.2
>>>>>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Powered by blists - more mailing lists