| Message ID | 20250224225246.3712295-6-jeffxu@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mseal system mappings | expand |
On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > From: Jeff Xu <jeffxu@chromium.org> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > the vdso. > > Testing passes on UML. Maybe expand on this by stating that it has been confirmed by Benjamin (I _believe_) that UML has no need for problematic relocation so this is known to be good. > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > Tested-by: Benjamin Berg <benjamin.berg@intel.com> Anyway I know UML has at any rate been confirmed to be good to go + patch looks fine, so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > --- > arch/um/Kconfig | 1 + > arch/x86/um/vdso/vma.c | 6 ++++-- > 2 files changed, 5 insertions(+), 2 deletions(-) > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > index 18051b1cfce0..eb2d439a5334 100644 > --- a/arch/um/Kconfig > +++ b/arch/um/Kconfig > @@ -10,6 +10,7 @@ config UML > select ARCH_HAS_FORTIFY_SOURCE > select ARCH_HAS_GCOV_PROFILE_ALL > select ARCH_HAS_KCOV > + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS > select ARCH_HAS_STRNCPY_FROM_USER > select ARCH_HAS_STRNLEN_USER > select HAVE_ARCH_AUDITSYSCALL > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c > index f238f7b33cdd..fdfba858ffc9 100644 > --- a/arch/x86/um/vdso/vma.c > +++ b/arch/x86/um/vdso/vma.c > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) > { > struct vm_area_struct *vma; > struct mm_struct *mm = current->mm; > + unsigned long vm_flags; > static struct vm_special_mapping vdso_mapping = { > .name = "[vdso]", > }; > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) > return -EINTR; > > vdso_mapping.pages = vdsop; > + vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; > + vm_flags |= VM_SEALED_SYSMAP; > vma = _install_special_mapping(mm, um_vdso_addr, PAGE_SIZE, > - VM_READ|VM_EXEC| > - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, > + vm_flags, > &vdso_mapping); > > mmap_write_unlock(mm); > -- > 2.48.1.658.g4767266eb4-goog >
Hi, On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > From: Jeff Xu <jeffxu@chromium.org> > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > the vdso. > > > > Testing passes on UML. > > Maybe expand on this by stating that it has been confirmed by Benjamin (I > _believe_) that UML has no need for problematic relocation so this is known to > be good. I may well be misreading this message, but this sounds to me that this is a misinterpretation. So, just to clarify in case that is needed. CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. However, the UML kernel is a normal userspace application itself and for this application to run, the host kernel must have the feature disabled. So, UML supports the feature. But it still *cannot* run on a host machine that has the feature enabled. Benjamin > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > Tested-by: Benjamin Berg <benjamin.berg@intel.com> > > Anyway I know UML has at any rate been confirmed to be good to go + > patch looks > fine, so: > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > --- > > arch/um/Kconfig | 1 + > > arch/x86/um/vdso/vma.c | 6 ++++-- > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > > index 18051b1cfce0..eb2d439a5334 100644 > > --- a/arch/um/Kconfig > > +++ b/arch/um/Kconfig > > @@ -10,6 +10,7 @@ config UML > > select ARCH_HAS_FORTIFY_SOURCE > > select ARCH_HAS_GCOV_PROFILE_ALL > > select ARCH_HAS_KCOV > > + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS > > select ARCH_HAS_STRNCPY_FROM_USER > > select ARCH_HAS_STRNLEN_USER > > select HAVE_ARCH_AUDITSYSCALL > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c > > index f238f7b33cdd..fdfba858ffc9 100644 > > --- a/arch/x86/um/vdso/vma.c > > +++ b/arch/x86/um/vdso/vma.c > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct > > linux_binprm *bprm, int uses_interp) > > { > > struct vm_area_struct *vma; > > struct mm_struct *mm = current->mm; > > + unsigned long vm_flags; > > static struct vm_special_mapping vdso_mapping = { > > .name = "[vdso]", > > }; > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct > > linux_binprm *bprm, int uses_interp) > > return -EINTR; > > > > vdso_mapping.pages = vdsop; > > + vm_flags = > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; > > + vm_flags |= VM_SEALED_SYSMAP; > > vma = _install_special_mapping(mm, um_vdso_addr, > > PAGE_SIZE, > > - VM_READ|VM_EXEC| > > - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, > > + vm_flags, > > &vdso_mapping); > > > > mmap_write_unlock(mm); > > -- > > 2.48.1.658.g4767266eb4-goog > > Intel Deutschland GmbH Registered Address: Am Campeon 10, 85579 Neubiberg, Germany Tel: +49 89 99 8853-0, www.intel.de Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva Chairperson of the Supervisory Board: Nicole Lau Registered Office: Munich Commercial Register: Amtsgericht Muenchen HRB 186928
On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > Hi, > > On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > > From: Jeff Xu <jeffxu@chromium.org> > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > > the vdso. > > > > > > Testing passes on UML. > > > > Maybe expand on this by stating that it has been confirmed by Benjamin (I > > _believe_) that UML has no need for problematic relocation so this is known to > > be good. > > I may well be misreading this message, but this sounds to me that this > is a misinterpretation. So, just to clarify in case that is needed. > > CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > However, the UML kernel is a normal userspace application itself and > for this application to run, the host kernel must have the feature > disabled. > > So, UML supports the feature. But it still *cannot* run on a host > machine that has the feature enabled. Sigh ok. Apologies if I misunderstood. Is there any point having this for the 'guest' system? I mean security wise are we concerned about sealing of system mappings? I feel like having this here might just add confusion and churn if it's not useful. If this is useless for UML guest, let's just drop this patch. > > Benjamin > > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > > Tested-by: Benjamin Berg <benjamin.berg@intel.com> > > > > Anyway I know UML has at any rate been confirmed to be good to go + > > patch looks > > fine, so: > > > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> OK guess drop this tag please until we can figure this out, sorry Jeff. > > > > > --- > > > arch/um/Kconfig | 1 + > > > arch/x86/um/vdso/vma.c | 6 ++++-- > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > > > index 18051b1cfce0..eb2d439a5334 100644 > > > --- a/arch/um/Kconfig > > > +++ b/arch/um/Kconfig > > > @@ -10,6 +10,7 @@ config UML > > > select ARCH_HAS_FORTIFY_SOURCE > > > select ARCH_HAS_GCOV_PROFILE_ALL > > > select ARCH_HAS_KCOV > > > + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS > > > select ARCH_HAS_STRNCPY_FROM_USER > > > select ARCH_HAS_STRNLEN_USER > > > select HAVE_ARCH_AUDITSYSCALL > > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c > > > index f238f7b33cdd..fdfba858ffc9 100644 > > > --- a/arch/x86/um/vdso/vma.c > > > +++ b/arch/x86/um/vdso/vma.c > > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct > > > linux_binprm *bprm, int uses_interp) > > > { > > > struct vm_area_struct *vma; > > > struct mm_struct *mm = current->mm; > > > + unsigned long vm_flags; > > > static struct vm_special_mapping vdso_mapping = { > > > .name = "[vdso]", > > > }; > > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct > > > linux_binprm *bprm, int uses_interp) > > > return -EINTR; > > > > > > vdso_mapping.pages = vdsop; > > > + vm_flags = > > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; > > > + vm_flags |= VM_SEALED_SYSMAP; > > > vma = _install_special_mapping(mm, um_vdso_addr, > > > PAGE_SIZE, > > > - VM_READ|VM_EXEC| > > > - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, > > > + vm_flags, > > > &vdso_mapping); > > > > > > mmap_write_unlock(mm); > > > -- > > > 2.48.1.658.g4767266eb4-goog > > > > > Intel Deutschland GmbH > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > Tel: +49 89 99 8853-0, www.intel.de > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva > Chairperson of the Supervisory Board: Nicole Lau > Registered Office: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928
Hi, On Tue, 2025-02-25 at 10:37 +0000, Lorenzo Stoakes wrote: > On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > > Hi, > > > > On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > > > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > > > From: Jeff Xu <jeffxu@chromium.org> > > > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > > > the vdso. > > > > > > > > Testing passes on UML. > > > > > > Maybe expand on this by stating that it has been confirmed by Benjamin (I > > > _believe_) that UML has no need for problematic relocation so this is known to > > > be good. > > > > I may well be misreading this message, but this sounds to me that this > > is a misinterpretation. So, just to clarify in case that is needed. > > > > CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > > However, the UML kernel is a normal userspace application itself and > > for this application to run, the host kernel must have the feature > > disabled. > > > > So, UML supports the feature. But it still *cannot* run on a host > > machine that has the feature enabled. > > Sigh ok. Apologies if I misunderstood. > > Is there any point having this for the 'guest' system? I mean security wise are > we concerned about sealing of system mappings? > > I feel like having this here might just add confusion and churn if it's not > useful. > > If this is useless for UML guest, let's just drop this patch. I figured it is not a lot of churn and there isn't really any cost to enabling the feature. That said, the only possible real-life use case I can see is doing MM subsystem testing using UML. We certainly do not need the feature to run our UML based wireless stack and driver tests. Benjamin > > > > > Benjamin > > > > > > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > > > Tested-by: Benjamin Berg <benjamin.berg@intel.com> > > > > > > Anyway I know UML has at any rate been confirmed to be good to go + > > > patch looks > > > fine, so: > > > > > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > OK guess drop this tag please until we can figure this out, sorry Jeff. > > > > > > > > --- > > > > arch/um/Kconfig | 1 + > > > > arch/x86/um/vdso/vma.c | 6 ++++-- > > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > > > > index 18051b1cfce0..eb2d439a5334 100644 > > > > --- a/arch/um/Kconfig > > > > +++ b/arch/um/Kconfig > > > > @@ -10,6 +10,7 @@ config UML > > > > select ARCH_HAS_FORTIFY_SOURCE > > > > select ARCH_HAS_GCOV_PROFILE_ALL > > > > select ARCH_HAS_KCOV > > > > + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS > > > > select ARCH_HAS_STRNCPY_FROM_USER > > > > select ARCH_HAS_STRNLEN_USER > > > > select HAVE_ARCH_AUDITSYSCALL > > > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c > > > > index f238f7b33cdd..fdfba858ffc9 100644 > > > > --- a/arch/x86/um/vdso/vma.c > > > > +++ b/arch/x86/um/vdso/vma.c > > > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct > > > > linux_binprm *bprm, int uses_interp) > > > > { > > > > struct vm_area_struct *vma; > > > > struct mm_struct *mm = current->mm; > > > > + unsigned long vm_flags; > > > > static struct vm_special_mapping vdso_mapping = { > > > > .name = "[vdso]", > > > > }; > > > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct > > > > linux_binprm *bprm, int uses_interp) > > > > return -EINTR; > > > > > > > > vdso_mapping.pages = vdsop; > > > > + vm_flags = > > > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; > > > > + vm_flags |= VM_SEALED_SYSMAP; > > > > vma = _install_special_mapping(mm, um_vdso_addr, > > > > PAGE_SIZE, > > > > - VM_READ|VM_EXEC| > > > > - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, > > > > + vm_flags, > > > > &vdso_mapping); > > > > > > > > mmap_write_unlock(mm); > > > > -- > > > > 2.48.1.658.g4767266eb4-goog > > > > > > > > Intel Deutschland GmbH > > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > > Tel: +49 89 99 8853-0, www.intel.de > > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva > > Chairperson of the Supervisory Board: Nicole Lau > > Registered Office: Munich > > Commercial Register: Amtsgericht Muenchen HRB 186928
On Tue, Feb 25, 2025 at 01:24:49PM +0100, Benjamin Berg wrote: > Hi, > > On Tue, 2025-02-25 at 10:37 +0000, Lorenzo Stoakes wrote: > > On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > > > Hi, > > > > > > On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > > > > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > > > > From: Jeff Xu <jeffxu@chromium.org> > > > > > > > > > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > > > > the vdso. > > > > > > > > > > Testing passes on UML. > > > > > > > > Maybe expand on this by stating that it has been confirmed by Benjamin (I > > > > _believe_) that UML has no need for problematic relocation so this is known to > > > > be good. > > > > > > I may well be misreading this message, but this sounds to me that this > > > is a misinterpretation. So, just to clarify in case that is needed. > > > > > > CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > > > However, the UML kernel is a normal userspace application itself and > > > for this application to run, the host kernel must have the feature > > > disabled. > > > > > > So, UML supports the feature. But it still *cannot* run on a host > > > machine that has the feature enabled. > > > > Sigh ok. Apologies if I misunderstood. > > > > Is there any point having this for the 'guest' system? I mean security wise are > > we concerned about sealing of system mappings? > > > > I feel like having this here might just add confusion and churn if it's not > > useful. > > > > If this is useless for UML guest, let's just drop this patch. > > I figured it is not a lot of churn and there isn't really any cost to > enabling the feature. > > That said, the only possible real-life use case I can see is doing MM > subsystem testing using UML. We certainly do not need the feature to > run our UML based wireless stack and driver tests. OK ack - my concern is users getting confused about this ironic host vs. client thing, must disable the security feature in the _actual kernel_ to enable it in the client. I'm not sure this is really worth it? I mean I agree this isn't a _huge_ amount added here and I don't want to be difficult - Jeff, Kees are you really keen on having this? Do you have specific use cases in mind or was this just a 'because we can':>) I guess if intent is to slowly add architectures, it's not totally insane since we kinda know this one is ok so if that's what it is, probably won't oppose it _too_ badly. > > Benjamin > > > > > > > > > Benjamin > > > > > > > > > > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@chromium.org> > > > > > Tested-by: Benjamin Berg <benjamin.berg@intel.com> > > > > > > > > Anyway I know UML has at any rate been confirmed to be good to go + > > > > patch looks > > > > fine, so: > > > > > > > > Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > > > > OK guess drop this tag please until we can figure this out, sorry Jeff. (to be clear this is just temporary while we establish what's up with this situation! :>) > > > > > > > > > > > --- > > > > > arch/um/Kconfig | 1 + > > > > > arch/x86/um/vdso/vma.c | 6 ++++-- > > > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/arch/um/Kconfig b/arch/um/Kconfig > > > > > index 18051b1cfce0..eb2d439a5334 100644 > > > > > --- a/arch/um/Kconfig > > > > > +++ b/arch/um/Kconfig > > > > > @@ -10,6 +10,7 @@ config UML > > > > > select ARCH_HAS_FORTIFY_SOURCE > > > > > select ARCH_HAS_GCOV_PROFILE_ALL > > > > > select ARCH_HAS_KCOV > > > > > + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS > > > > > select ARCH_HAS_STRNCPY_FROM_USER > > > > > select ARCH_HAS_STRNLEN_USER > > > > > select HAVE_ARCH_AUDITSYSCALL > > > > > diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c > > > > > index f238f7b33cdd..fdfba858ffc9 100644 > > > > > --- a/arch/x86/um/vdso/vma.c > > > > > +++ b/arch/x86/um/vdso/vma.c > > > > > @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct > > > > > linux_binprm *bprm, int uses_interp) > > > > > { > > > > > struct vm_area_struct *vma; > > > > > struct mm_struct *mm = current->mm; > > > > > + unsigned long vm_flags; > > > > > static struct vm_special_mapping vdso_mapping = { > > > > > .name = "[vdso]", > > > > > }; > > > > > @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct > > > > > linux_binprm *bprm, int uses_interp) > > > > > return -EINTR; > > > > > > > > > > vdso_mapping.pages = vdsop; > > > > > + vm_flags = > > > > > VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; > > > > > + vm_flags |= VM_SEALED_SYSMAP; > > > > > vma = _install_special_mapping(mm, um_vdso_addr, > > > > > PAGE_SIZE, > > > > > - VM_READ|VM_EXEC| > > > > > - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, > > > > > + vm_flags, > > > > > &vdso_mapping); > > > > > > > > > > mmap_write_unlock(mm); > > > > > -- > > > > > 2.48.1.658.g4767266eb4-goog > > > > > > > > > > > Intel Deutschland GmbH > > > Registered Address: Am Campeon 10, 85579 Neubiberg, Germany > > > Tel: +49 89 99 8853-0, www.intel.de > > > Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva > > > Chairperson of the Supervisory Board: Nicole Lau > > > Registered Office: Munich > > > Commercial Register: Amtsgericht Muenchen HRB 186928 >
On Tue, 2025-02-25 at 13:41 +0000, Lorenzo Stoakes wrote: > > I figured it is not a lot of churn and there isn't really any cost to > > enabling the feature. > > > > That said, the only possible real-life use case I can see is doing MM > > subsystem testing using UML. We certainly do not need the feature to > > run our UML based wireless stack and driver tests. > > OK ack - my concern is users getting confused about this ironic host > vs. client thing, must disable the security feature in the _actual kernel_ > to enable it in the client. Well, s/to enable it in the client/to run the client/, I guess. I'm still a bit disappointed in the whole thing anyway - if this does get enabled in e.g. ChromeOS (as it looks like), then it'll mean that gvisor/rr/UML will never run on chromebooks, which ... I mean yeah who's going to do that, so it's more of a purist disappointment I guess. Can't run kunit on a chromebook then, for example. This looks much different for more general purpose distros too. I also don't really want to reopen a discussion that was probably had before, but I did wonder now what the security downsides of having an opt-out, e.g. a new ELF property, for skipping the sealings would be. Perhaps, depending on the impact, even making that mean "no system mappings at all", at least for UML I believe they're not needed in the first place. > I'm not sure this is really worth it? > > I mean I agree this isn't a _huge_ amount added here and I don't want to be > difficult - Jeff, Kees are you really keen on having this? Do you have > specific use cases in mind or was this just a 'because we can':>) There's always kunit that can run with UML, but I don't see tests being added for this feature, in fact the only thing here is _disabling_ a test. Maybe it should come with tests and then it'd be more interesting ;-) The commit says "Testing passes on UML" but I'm not sure I see what testing that might have been, per the cover letter Benjamin did that? > I guess if intent is to slowly add architectures, it's not totally insane > since we kinda know this one is ok so if that's what it is, probably won't > oppose it _too_ badly. I think it still makes _some_ sense to have it for the testing aspect, but perhaps might then make sense to split it out of the series to avoid all the confusion and submit it to UML separately later? Or just leave it since you can always test with qemu. johannes
On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: >> Hi, >> >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: >> > > From: Jeff Xu <jeffxu@chromium.org> >> > > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering >> > > the vdso. >> > > >> > > Testing passes on UML. >> > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I >> > _believe_) that UML has no need for problematic relocation so this is known to >> > be good. >> >> I may well be misreading this message, but this sounds to me that this >> is a misinterpretation. So, just to clarify in case that is needed. >> >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. >> However, the UML kernel is a normal userspace application itself and >> for this application to run, the host kernel must have the feature >> disabled. >> >> So, UML supports the feature. But it still *cannot* run on a host >> machine that has the feature enabled. > >Sigh ok. Apologies if I misunderstood. > >Is there any point having this for the 'guest' system? I mean security wise are >we concerned about sealing of system mappings? UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me. > >I feel like having this here might just add confusion and churn if it's not >useful. > >If this is useless for UML guest, let's just drop this patch. But on the flip side, it's certainly not critical to have UML supported. I guess I just don't see a down side to keeping the patch. -Kees
On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote: > > > On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > >> Hi, > >> > >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > >> > > From: Jeff Xu <jeffxu@chromium.org> > >> > > > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > >> > > the vdso. > >> > > > >> > > Testing passes on UML. > >> > > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I > >> > _believe_) that UML has no need for problematic relocation so this is known to > >> > be good. > >> > >> I may well be misreading this message, but this sounds to me that this > >> is a misinterpretation. So, just to clarify in case that is needed. > >> > >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > >> However, the UML kernel is a normal userspace application itself and > >> for this application to run, the host kernel must have the feature > >> disabled. > >> > >> So, UML supports the feature. But it still *cannot* run on a host > >> machine that has the feature enabled. > > > >Sigh ok. Apologies if I misunderstood. > > > >Is there any point having this for the 'guest' system? I mean security wise are > >we concerned about sealing of system mappings? > > UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me. > 'Having sealing working' you mean system sealing? Because mseal works fine (presumably in UML, not tried myself!) System msealing lacks any test in this series (I did ask for them...), certainly no kunit tests, so this seems a bit theoretical? Unless you're talking about the theoretical interaction of kunit tests and VDSO sealing? I mean can't we just introduce this at the time if we believe this'd be useful? Adding stuff for entirely theoretical reasons is generally frowned upon and isn't that the (at least one) definition of churn? Can you give a really specific reason why this needs to be added? For x86-64, arm64 the reason is obvious (albeit one can argue how much security benefit msealing the VDSO truly gives). For UML it just isn't, at all. > > > >I feel like having this here might just add confusion and churn if it's not > >useful. > > > >If this is useless for UML guest, let's just drop this patch. > > But on the flip side, it's certainly not critical to have UML supported. I guess I just don't see a down side to keeping the patch. Right, presumably you disagree that it might be confusing that to test mseal _system_ sealing (not mseal itself) by enabling mseal system sealing in UML but having to disable it in the host? Because that seems really confusing to me (and why I explicitly cited it as a reason I'm confused here, even the exchange above is confused). If there benefit is basically theoretical/why not/nil, and the downside is confusion + churn (albeit small churn), why not just wait until somebody writes kunit tests? If I saw a list of supported arches including UML, but also saw a note about it not working in UML I'd definitely be confused. Generally I'm not a fan of adding features mid-way through a series, the revisions are meant to be refinements of the original, not an evolving thing. So in general I'd prefer this to be added if + when we need it for something. > > -Kees > > > -- > Kees Cook
On Tue, Feb 25, 2025 at 03:31:06PM +0000, Lorenzo Stoakes wrote: > On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote: > > > > > > On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > > >> Hi, > > >> > > >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > > >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > >> > > From: Jeff Xu <jeffxu@chromium.org> > > >> > > > > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > >> > > the vdso. > > >> > > > > >> > > Testing passes on UML. > > >> > > > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I > > >> > _believe_) that UML has no need for problematic relocation so this is known to > > >> > be good. > > >> > > >> I may well be misreading this message, but this sounds to me that this > > >> is a misinterpretation. So, just to clarify in case that is needed. > > >> > > >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > > >> However, the UML kernel is a normal userspace application itself and > > >> for this application to run, the host kernel must have the feature > > >> disabled. > > >> > > >> So, UML supports the feature. But it still *cannot* run on a host > > >> machine that has the feature enabled. > > > > > >Sigh ok. Apologies if I misunderstood. > > > > > >Is there any point having this for the 'guest' system? I mean security wise are > > >we concerned about sealing of system mappings? > > > > UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me. > > > > 'Having sealing working' you mean system sealing? Because mseal works fine > (presumably in UML, not tried myself!) Sorry, yes, I mean "system mapping msealing". > > System msealing lacks any test in this series (I did ask for them...), certainly > no kunit tests, so this seems a bit theoretical? Unless you're talking about the > theoretical interaction of kunit tests and VDSO sealing? Right, I meant theoretical interaction, but it would be useful for future KUnit tests of system mapping msealing too. > I mean can't we just introduce this at the time if we believe this'd be useful? Perhaps adding it as part of adding some KUnit tests that exercise the system mapping msealing would be the most sensible. > Generally I'm not a fan of adding features mid-way through a series, the > revisions are meant to be refinements of the original, not an evolving thing. > > So in general I'd prefer this to be added if + when we need it for something. Yup, makes sense. And it may be that KUnit tests need to exercise more than what UML can support, so even the KUnit idea may be invalid. Jeff, let's leave off UML for this initial "minimum viable feature" series, unless there is a strong reason to keep it.
On Tue, Feb 25, 2025 at 10:38 AM Kees Cook <kees@kernel.org> wrote: > > On Tue, Feb 25, 2025 at 03:31:06PM +0000, Lorenzo Stoakes wrote: > > On Tue, Feb 25, 2025 at 07:06:13AM -0800, Kees Cook wrote: > > > > > > > > > On February 25, 2025 2:37:11 AM PST, Lorenzo Stoakes <lorenzo.stoakes@oracle.com> wrote: > > > >On Tue, Feb 25, 2025 at 08:45:21AM +0000, Berg, Benjamin wrote: > > > >> Hi, > > > >> > > > >> On Tue, 2025-02-25 at 06:22 +0000, Lorenzo Stoakes wrote: > > > >> > On Mon, Feb 24, 2025 at 10:52:44PM +0000, jeffxu@chromium.org wrote: > > > >> > > From: Jeff Xu <jeffxu@chromium.org> > > > >> > > > > > >> > > Provide support for CONFIG_MSEAL_SYSTEM_MAPPINGS on UML, covering > > > >> > > the vdso. > > > >> > > > > > >> > > Testing passes on UML. > > > >> > > > > >> > Maybe expand on this by stating that it has been confirmed by Benjamin (I > > > >> > _believe_) that UML has no need for problematic relocation so this is known to > > > >> > be good. > > > >> > > > >> I may well be misreading this message, but this sounds to me that this > > > >> is a misinterpretation. So, just to clarify in case that is needed. > > > >> > > > >> CONFIG_MSEAL_SYSTEM_MAPPINGS does work fine for the UML kernel. > > > >> However, the UML kernel is a normal userspace application itself and > > > >> for this application to run, the host kernel must have the feature > > > >> disabled. > > > >> > > > >> So, UML supports the feature. But it still *cannot* run on a host > > > >> machine that has the feature enabled. > > > > > > > >Sigh ok. Apologies if I misunderstood. > > > > > > > >Is there any point having this for the 'guest' system? I mean security wise are > > > >we concerned about sealing of system mappings? > > > > > > UML guests are used for testing. For example, it's the default target for KUnit's scripts. Having sealing working in the guest seems generally useful to me. > > > > > > > 'Having sealing working' you mean system sealing? Because mseal works fine > > (presumably in UML, not tried myself!) > > Sorry, yes, I mean "system mapping msealing". > > > > > System msealing lacks any test in this series (I did ask for them...), certainly > > no kunit tests, so this seems a bit theoretical? Unless you're talking about the > > theoretical interaction of kunit tests and VDSO sealing? > > Right, I meant theoretical interaction, but it would be useful for > future KUnit tests of system mapping msealing too. > > > I mean can't we just introduce this at the time if we believe this'd be useful? > > Perhaps adding it as part of adding some KUnit tests that exercise the > system mapping msealing would be the most sensible. > > > Generally I'm not a fan of adding features mid-way through a series, the > > revisions are meant to be refinements of the original, not an evolving thing. > > > > So in general I'd prefer this to be added if + when we need it for something. > > Yup, makes sense. And it may be that KUnit tests need to exercise more > than what UML can support, so even the KUnit idea may be invalid. > > Jeff, let's leave off UML for this initial "minimum viable feature" > series, unless there is a strong reason to keep it. > Sure. It will be removed unless someone raises a strong reason to keep it. UML can be added when future KUnit tests need it. Thanks -Jeff > -- > Kees Cook
diff --git a/arch/um/Kconfig b/arch/um/Kconfig index 18051b1cfce0..eb2d439a5334 100644 --- a/arch/um/Kconfig +++ b/arch/um/Kconfig @@ -10,6 +10,7 @@ config UML select ARCH_HAS_FORTIFY_SOURCE select ARCH_HAS_GCOV_PROFILE_ALL select ARCH_HAS_KCOV + select ARCH_HAS_MSEAL_SYSTEM_MAPPINGS select ARCH_HAS_STRNCPY_FROM_USER select ARCH_HAS_STRNLEN_USER select HAVE_ARCH_AUDITSYSCALL diff --git a/arch/x86/um/vdso/vma.c b/arch/x86/um/vdso/vma.c index f238f7b33cdd..fdfba858ffc9 100644 --- a/arch/x86/um/vdso/vma.c +++ b/arch/x86/um/vdso/vma.c @@ -54,6 +54,7 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) { struct vm_area_struct *vma; struct mm_struct *mm = current->mm; + unsigned long vm_flags; static struct vm_special_mapping vdso_mapping = { .name = "[vdso]", }; @@ -65,9 +66,10 @@ int arch_setup_additional_pages(struct linux_binprm *bprm, int uses_interp) return -EINTR; vdso_mapping.pages = vdsop; + vm_flags = VM_READ|VM_EXEC|VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC; + vm_flags |= VM_SEALED_SYSMAP; vma = _install_special_mapping(mm, um_vdso_addr, PAGE_SIZE, - VM_READ|VM_EXEC| - VM_MAYREAD|VM_MAYWRITE|VM_MAYEXEC, + vm_flags, &vdso_mapping); mmap_write_unlock(mm);