Re: [RFC PATCH v5 1/7] mseal, system mappings: kernel config and header change

[Liam R. Howlett]

* Liam R. Howlett <Liam.Howlett@xxxxxxxxxx> [250213 19:14]:
> * Jeff Xu <jeffxu@xxxxxxxxxxxx> [250213 17:00]:
> > On Thu, Feb 13, 2025 at 12:54 PM Liam R. Howlett
> > <Liam.Howlett@xxxxxxxxxx> wrote:
> > 
> > > > > >
> > > > > > VM_SEALED isn't defined in 32-bit systems, and mseal.c isn't part of
> > > > > > the build. This is intentional. Any 32-bit code trying to use the
> > > > > > sealing function or the VM_SEALED flag will immediately fail
> > > > > > compilation. This makes it easier to identify incorrect usage.
> > > > >
> > > > > So you are against using the #define because the VM_SYSTEM_SEAL will be
> > > > > defined, even though it will be VM_NONE?  This is no worse than a
> > > > > function that returns 0, and it aligns better with what we have today in
> > > > > that it can be put in the list of other flags.
> > > >
> > > > When I was reading through all of this and considering the history of
> > > > its development goals, it strikes me that a function is easier for the
> > > > future if/when this can be made a boot-time setting.
> > > >
> > >
> > > Reworking this change to function as a boot-time parameter, or whatever,
> > > would not be a significant amount of work, if/when the time comes.
> > > Since we don't know what the future holds, it it unnecessary to engineer
> > > in a potential change for a future version when the change is easy
> > > enough to make later and keep the code cleaner now.
> > >
> > Sure, I will put the function in mm.h for this patch. We can find a
> > proper place when it is time to implement the boot-time parameter
> > change.
> > 
> > The call stack for sealing system mapping is something like below:
> > 
> > install_special_mapping (mm/mmap.c)
> > map_vdso (arch/x86/entry/vdso/vma.c)
> > load_elf_binary (fs/binfmt_elf.c)
> > load_misc_binary (fs/binfmt_misc.c)
> > bprm_execve (fs/exec.c)
> > do_execveat_common
> > __x64_sys_execve
> > do_syscall_64
> > 
> > IMO, there's a clear divide between the API implementer and the API user.
> > mm and mm.h are the providers, offering the core mm functionality
> > through APIs/data structures like install_special_mapping().
> > 
> > The exe layer (bprm_execve, map_vdso, etc)  is the consumer of the
> > install_special_mapping.
> > The logic related to checking if sealing system mapping is enabled
> > belongs to the exe layer.
> 
> Since this is an all or nothing enabling, there is no reason to have
> each caller check the same thing and do the same action. You should put
> the logic into the provider - they all end up doing the same thing.
> 
> Also, this is a compile time option so it doesn't even need to be
> checked on execution - just apply it in the first place, at the source.
> Your static inline function was already doing this...?
> 
> I'm confused as to what you are arguing here because it goes against
> what you had and what I suggested.  The alternative you are suggesting
> is more code, more instructions, and the best outcome is the same
> result.

I think I understand what you are saying now: the interface to
install_special_mapping() needs to take the vma flag, as it does today.
What I don't understand is that what you proposed and what I proposed
both do that.

What I'm saying is that, since system mappings are enabled, we can
already know implicitly by having VM_SYSTEM_SEAL either VM_NONE or
VM_SEAL.

Turning this:

@@ -264,11 +266,12 @@ static int map_vdso(const struct vdso_image *image, unsigned long addr)
 	/*
 	 * MAYWRITE to allow gdb to COW and set breakpoints
 	 */
+	vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC;
+	vm_flags |= mseal_system_mappings();
 	vma = _install_special_mapping(mm,
 				       text_start,
 				       image->size,
-				       VM_READ|VM_EXEC|
-				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
+				       vm_flags,
 				       &vdso_mapping);

to this:

 	/*
 	 * MAYWRITE to allow gdb to COW and set breakpoints
 	 */
 	vma = _install_special_mapping(mm,
 				       text_start,
 				       image->size,
				       VM_READ|VM_EXEC|
-				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC,
+				       VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC|
+				       VM_SYSTEM_SEAL,
 				       &vdso_mapping);

No unsigned long vm_flags needed.  It's easier to read and I don't think
it's any more hidden than the vm_flags |= function call option.

> 
> > 
> > >
> > > > If mm maintainers prefer a #define for now, that's fine of course. The
> > > > value of VM_SYSTEM_SEAL can be VM_NONE on 32-bit.
> > >
> > > Thanks.  I think having a flag with VM_NONE on 32-bit is just as sane as
> > > a "flags |= system_seal()" call that unconditionally returns 0 on
> > > 32-bit.
> > >
> > Consider the case below in src/third_party/kernel/v6.6/fs/proc/task_mmu.c,
> > 
> > #ifdef CONFIG_64BIT
> > [ilog2(VM_SEALED)] = "sl",
> > #endif
> > 
> > If #ifdef CONFIG_64BIT is missing, it won't be detected during compile time.
> > 
> > Setting VM_SEALED to VM_NONE could simplify the coding in some cases
> > (get/set case), but it might make other cases error prone.
> > 
> > I would prefer to not have VM_SEALED for 32 bit.
> 
> But what I posted leaves VM_SEALED undefined for 32 bit, it defines
> VM_SYSTEM_SEALED which can be placed, for instance, into
> _install_special_mapping() arguments directly.  Reducing the change to
> just adding a new flag to the list.  And it's applied to all system
> mappings based on if it's enabled on compile or not.
> 
> Also:
> include/linux/mm.h:#define VM_NONE              0x00000000
> so, I'm not sure what evaluation you are concerned about?  It would be
> defined as 0.
> 
> Thanks,
> Liam
> 
>