Re: [RFC PATCH v2 1/1] exec: seal system mappings

[Jeff Xu <jeffxu@chromium.org>]

Hi Liam

On Thu, Oct 17, 2024 at 9:01 AM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:

> > > Does it make sense for this to live in exec?  Couldn't you put it in the
> > > mm/mseal.c file?  It's vma flags for mappings and you've put it in
> > > fs/exec?
> > >
> > If you are referring to utilities related to kernel cmdline, they
> > should be in this file.
>
> You created a wrapper for the command line, but then included the user
> in this file as well.
>
> hugetlbfs reads the command line as well, in cmdline_parse_hugetlb_cma.
> That parser lives with the rest of the hugetlb code in hugetlb.c
>
> I think this has to do with your view as this is an exec thing, where I
> think it's an mm thing.  My arguments are that you are directly adding
> flags to vmas and it's dealing with mseal which has memory in the name
> with the file living in the mm/ directory.  If I wanted to know what's
> using mseal, I'd start there and totally miss what you are adding here.
>
> Besides applying a vma flag to exec mappings, why do you feel like it
> belongs in fs/ ?
>
The vdso/vvar/stack/heap alike are type of mappings belonging to
processes, and are created during execve() syscall which is in
fs/exec.c.

mm/mseal.c provides core memory sealing functionality and exec.c uses
it. IMO, it is better to keep the provider (mm/mseal.c) and consumer
(executable) separate.

To make modulization better, I can do below adjustment:
if (seal_system_mapping_enabled()) <-- implemented by fs/exec.c
   add_vm_sealed() <- keep in include/linux/mm.h

However, if you have a strong opinion on this, I could move the
parsing logic to mm/mseal.

> > > > +void update_seal_exec_system_mappings(unsigned long *
> > > The name is also very long and a bit odd, it could be used for other
> > > reasons, but you have _system_mappings on the end, and you use seal but
> > > it's mseal (or vm_seal)?  Would mseal_flag() work?
> > >
> > It could be longer :-)
> >  it means update_sealing_flag_for_executable_system_mappings.
> > mseal_flag is too short and not descriptive.
>
> mseal_exec_flags() ?
>
It needs to be more descriptive because there are also stacks and
heaps to be sealed. I suggest to use below name to make it shorter:

if (seal_system_mapping_enabled())
   add_vm_sealed()

> > > > diff --git a/mm/mmap.c b/mm/mmap.c
> > > > index 57fd5ab2abe7..d4717e34a60d 100644
> > > > --- a/mm/mmap.c
> > > > +++ b/mm/mmap.c
> > > > @@ -2133,6 +2133,7 @@ struct vm_area_struct *_install_special_mapping(
> > > >       unsigned long addr, unsigned long len,
> > > >       unsigned long vm_flags, const struct vm_special_mapping *spec)
> > > >  {
> > > > +     update_seal_exec_system_mappings(&vm_flags);
> > > >       return __install_special_mapping(mm, addr, len, vm_flags, (void *)spec,
> > > >                                       &special_mapping_vmops);
> > >
> > > If you were to return a flag, you could change the vm_flags argument to
> > > vm_flags | mseal_flag()
> > >
> > passing pointer seems to be the most efficient way.
>
> I disagree.  Here is the godbolt.org output for gcc x86-64 14.2 of your
> code (with some added #defines to make it compile)
>
> seal_system_mappings:
>         .long   1
> seal_system_mappings_enabled:
>         push    rbp
>         mov     rbp, rsp
>         mov     eax, DWORD PTR seal_system_mappings[rip]
>         cmp     eax, 1
>         jne     .L2
>         mov     eax, 1
>         jmp     .L3
> .L2:
>         mov     eax, 0
> .L3:
>         pop     rbp
>         ret
> update_seal_exec_system_mappings:
>         push    rbp
>         mov     rbp, rsp
>         sub     rsp, 8
>         mov     QWORD PTR [rbp-8], rdi
>         mov     rax, QWORD PTR [rbp-8]
>         mov     rax, QWORD PTR [rax]
>         and     eax, 2
>         test    rax, rax
>         jne     .L6
>         call    seal_system_mappings_enabled
>         test    al, al
>         je      .L6
>         mov     rax, QWORD PTR [rbp-8]
>         mov     rax, QWORD PTR [rax]
>         or      rax, 2
>         mov     rdx, rax
>         mov     rax, QWORD PTR [rbp-8]
>         mov     QWORD PTR [rax], rdx
> .L6:
>         nop
>         leave
>         ret
> main:
>         push    rbp
>         mov     rbp, rsp
>         sub     rsp, 16
>         mov     QWORD PTR [rbp-8], 0
>         lea     rax, [rbp-8]
>         mov     rdi, rax
>         call    update_seal_exec_system_mappings
>         mov     rax, QWORD PTR [rbp-8]
>         leave
>         ret
>
> ----- 48 lines -----
> Here is what I am suggesting to do with replacing the passing of a
> pointer with a concise "vm_flags | mseal_exec_flags()" (with the same
> added #defines to make it compile)
>
> seal_system_mappings:
>         .long   1
> mseal_exec_flags:
>         push    rbp
>         mov     rbp, rsp
>         mov     eax, DWORD PTR seal_system_mappings[rip]
>         cmp     eax, 1
>         jne     .L2
>         mov     eax, 2
>         jmp     .L3
> .L2:
>         mov     eax, 0
> .L3:
>         pop     rbp
>         ret
> main:
>         push    rbp
>         mov     rbp, rsp
>         sub     rsp, 16
>         mov     QWORD PTR [rbp-8], 0
>         call    mseal_exec_flags
>         mov     edx, eax
>         mov     rax, QWORD PTR [rbp-8]
>         or      eax, edx
>         leave
>         ret
>
> ----- 26 lines -----
>
> So as you can see, there are less instructions in my version; there are
> 47.92% less lines of assembly.
>
vm_flags already  run out of space in 32 bit, sooner or later we will
need to change that to *** a struct ***,  passing address will be
becoming necessary with struct.  Since this is not a performance
sensitive code path, 3 or 4 times during execve(), I think it would be
good to start  here.

-Jeff