Re: [PATCH v7 6/7] mseal, system mappings: uprobe mapping

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Tue, Feb 25, 2025 at 04:06:37PM -0800, Jeff Xu wrote:
> On Mon, Feb 24, 2025 at 10:24 PM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Mon, Feb 24, 2025 at 10:52:45PM +0000, jeffxu@chromium.org wrote:
> > > From: Jeff Xu <jeffxu@chromium.org>
> > >
> > > Provide support to mseal the uprobe mapping.
> > >
> > > Unlike other system mappings, the uprobe mapping is not
> > > established during program startup. However, its lifetime is the same
> > > as the process's lifetime. It could be sealed from creation.
> > >
> >
> > I thought we agreed not to enable this for now? What testing
> > have you done to ensure this is functional?
> >
> I honestly don't know much about uprobe. I don't recall that I agree
> to ignore that though.

OK sorry I realise you have done this from an early version of the series,
my mistake! Apologies.

I'm concerned you don't feel you know much about uprobe, but I guess you
defer to Oleg's views here?

If he's confirmed this is ok, then fine.

>
> As indicated in the cover letter, it is my understanding that uprobe's
> mapping's lifetime are the same as process's lifetime, thus sealable.

> [1]
> Oleg Nesterov, also cc, seems OK with mseal it in the early version of
> this patch [2]
>
> Are there any potential downsides of doing this? If yes, I can remove it.
>
> I'm also looking at Oleg to give more guidance on this :-), or if
> there are some functional tests that I need to do for uprobe.

Yeah, apologies, my mistake I forgot that this was from early, I thought it
was scope creep... but I double-checked and yeah, no haha.

>
>
> [1] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/
> [2] https://lore.kernel.org/all/20241005200741.GA24353@redhat.com/
>
> > I mean is this literally _all_ uprobe mappings now being sealed?
> >
> > I'd really like some more assurances on this one. And what are you
> > mitigating by sealing these? I get VDSO (kinda) but uprobes?
> >
> > You really need to provide more justification here.
>
> Sure. In our threat model, we need to seal all r-x, r--, and --x
> mappings to prevent them from becoming writable. This applies to all
> mappings, regardless of whether they're created by the kernel or
> dynamic linker.

All mappings? :P I mean I guess you mean somehow, all 'system' mappings
right?

I guess you mean that somehow some malicious user could manipulate these
mappings from a sandbox or such using a series of exploits that are maybe
more achievable that arbitrary code execution (rop with syscalls or sth? I
am not a security person - obviously! :)

And then un-sandboxed code could innocently touch and bang.

I mean that to me makes sense and cool, we're good. Something like this in
the doc, just a brief sentence like this for idiots (or perhaps you might
say, idiots when it comes to security :) like me would be great, thanks!

>
>
> > > Signed-off-by: Jeff Xu <jeffxu@chromium.org>
> > > ---
> > >  kernel/events/uprobes.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> > > index 2ca797cbe465..8dcdfa0d306b 100644
> > > --- a/kernel/events/uprobes.c
> > > +++ b/kernel/events/uprobes.c
> > > @@ -1662,6 +1662,7 @@ static const struct vm_special_mapping xol_mapping = {
> > >  static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> > >  {
> > >       struct vm_area_struct *vma;
> > > +     unsigned long vm_flags;
> > >       int ret;
> > >
> > >       if (mmap_write_lock_killable(mm))
> > > @@ -1682,8 +1683,10 @@ static int xol_add_vma(struct mm_struct *mm, struct xol_area *area)
> > >               }
> > >       }
> > >
> > > +     vm_flags = VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO;
> > > +     vm_flags |= VM_SEALED_SYSMAP;
> > >       vma = _install_special_mapping(mm, area->vaddr, PAGE_SIZE,
> > > -                             VM_EXEC|VM_MAYEXEC|VM_DONTCOPY|VM_IO,
> > > +                             vm_flags,
> > >                               &xol_mapping);
> > >       if (IS_ERR(vma)) {
> > >               ret = PTR_ERR(vma);
> > > --
> > > 2.48.1.658.g4767266eb4-goog
> > >