Message-ID: <26e04390-058f-4174-ad6f-3f0afd803fb1@lucifer.local>
Date: Thu, 8 May 2025 19:08:57 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: 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,
        alex@...ti.fr, 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

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.

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
> > > >

Powered by blists - more mailing lists