| Message ID | 20221209160453.3246150-7-jeffxu@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC | expand |
On 12/9/2022 8:04 AM, jeffxu@chromium.org wrote: > From: Jeff Xu <jeffxu@google.com> > > The new security_memfd_create allows lsm to check flags of > memfd_create. > > The security by default system (such as chromeos) can use this > to implement system wide lsm to allow only non-executable memfd > being created. > > Signed-off-by: Jeff Xu <jeffxu@google.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 4 ++++ > include/linux/security.h | 6 ++++++ > mm/memfd.c | 5 +++++ > security/security.c | 5 +++++ > 5 files changed, 21 insertions(+) > > diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h > index ec119da1d89b..fd40840927c8 100644 > --- a/include/linux/lsm_hook_defs.h > +++ b/include/linux/lsm_hook_defs.h > @@ -164,6 +164,7 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file) > LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file) > LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd, > unsigned long arg) > +LSM_HOOK(int, 0, memfd_create, char *name, unsigned int flags) > LSM_HOOK(int, 0, mmap_addr, unsigned long addr) > LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot, > unsigned long prot, unsigned long flags) > diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h > index 4ec80b96c22e..5a18a6552278 100644 > --- a/include/linux/lsm_hooks.h > +++ b/include/linux/lsm_hooks.h > @@ -543,6 +543,10 @@ > * simple integer value. When @arg represents a user space pointer, it > * should never be used by the security module. > * Return 0 if permission is granted. > + * @memfd_create: > + * @name is the name of memfd file. > + * @flags is the flags used in memfd_create. > + * Return 0 if permission is granted. > * @mmap_addr : > * Check permissions for a mmap operation at @addr. > * @addr contains virtual address that will be used for the operation. > diff --git a/include/linux/security.h b/include/linux/security.h > index ca1b7109c0db..5b87a780822a 100644 > --- a/include/linux/security.h > +++ b/include/linux/security.h > @@ -384,6 +384,7 @@ int security_file_permission(struct file *file, int mask); > int security_file_alloc(struct file *file); > void security_file_free(struct file *file); > int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg); > +int security_memfd_create(char *name, unsigned int flags); > int security_mmap_file(struct file *file, unsigned long prot, > unsigned long flags); > int security_mmap_addr(unsigned long addr); > @@ -963,6 +964,11 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd, > return 0; > } > > +static inline int security_memfd_create(char *name, unsigned int flags) > +{ > + return 0; > +} > + Add a proper kernel doc comment for this function. > static inline int security_mmap_file(struct file *file, unsigned long prot, > unsigned long flags) > { > diff --git a/mm/memfd.c b/mm/memfd.c > index 92f0a5765f7c..f04ed5f0474f 100644 > --- a/mm/memfd.c > +++ b/mm/memfd.c > @@ -356,6 +356,11 @@ SYSCALL_DEFINE2(memfd_create, > goto err_name; > } > > + /* security hook for memfd_create */ > + error = security_memfd_create(name, flags); > + if (error) > + return error; > + > if (flags & MFD_HUGETLB) { > file = hugetlb_file_setup(name, 0, VM_NORESERVE, > HUGETLB_ANONHUGE_INODE, > diff --git a/security/security.c b/security/security.c > index 79d82cb6e469..57788cf94075 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -1010,6 +1010,11 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, > } > EXPORT_SYMBOL(security_sb_clone_mnt_opts); > > +int security_memfd_create(char *name, unsigned int flags) > +{ > + return call_int_hook(memfd_create, 0, name, flags); > +} > + > int security_move_mount(const struct path *from_path, const struct path *to_path) > { > return call_int_hook(move_mount, 0, from_path, to_path);
On Fri, Dec 9, 2022 at 11:05 AM <jeffxu@chromium.org> wrote: > > From: Jeff Xu <jeffxu@google.com> > > The new security_memfd_create allows lsm to check flags of > memfd_create. > > The security by default system (such as chromeos) can use this > to implement system wide lsm to allow only non-executable memfd > being created. > > Signed-off-by: Jeff Xu <jeffxu@google.com> > Reported-by: kernel test robot <lkp@intel.com> > --- > include/linux/lsm_hook_defs.h | 1 + > include/linux/lsm_hooks.h | 4 ++++ > include/linux/security.h | 6 ++++++ > mm/memfd.c | 5 +++++ > security/security.c | 5 +++++ > 5 files changed, 21 insertions(+) We typically require at least one in-tree LSM implementation to accompany a new LSM hook. Beyond simply providing proof that the hook has value, it helps provide a functional example both for reviewers as well as future LSM implementations. Also, while the BPF LSM is definitely "in-tree", its nature is such that the actual implementation lives out-of-tree; something like SELinux, AppArmor, Smack, etc. are much more desirable from an in-tree example perspective.
On 12/13/2022 7:00 AM, Jeff Xu wrote: > On Fri, Dec 9, 2022 at 10:29 AM Paul Moore <paul@paul-moore.com> wrote: >> On Fri, Dec 9, 2022 at 11:05 AM <jeffxu@chromium.org> wrote: >>> From: Jeff Xu <jeffxu@google.com> >>> >>> The new security_memfd_create allows lsm to check flags of >>> memfd_create. >>> >>> The security by default system (such as chromeos) can use this >>> to implement system wide lsm to allow only non-executable memfd >>> being created. >>> >>> Signed-off-by: Jeff Xu <jeffxu@google.com> >>> Reported-by: kernel test robot <lkp@intel.com> >>> --- >>> include/linux/lsm_hook_defs.h | 1 + >>> include/linux/lsm_hooks.h | 4 ++++ >>> include/linux/security.h | 6 ++++++ >>> mm/memfd.c | 5 +++++ >>> security/security.c | 5 +++++ >>> 5 files changed, 21 insertions(+) >> We typically require at least one in-tree LSM implementation to >> accompany a new LSM hook. Beyond simply providing proof that the hook >> has value, it helps provide a functional example both for reviewers as >> well as future LSM implementations. Also, while the BPF LSM is >> definitely "in-tree", its nature is such that the actual >> implementation lives out-of-tree; something like SELinux, AppArmor, >> Smack, etc. are much more desirable from an in-tree example >> perspective. >> > Thanks for the comments. > Would that be OK if I add a new LSM in the kernel to block executable > memfd creation ? > Alternatively, it might be possible to add this into SELinux or > landlock, it will be a larger change. I expect you'll get other opinions, but I'd be happy with a small LSM that does sophisticated memory fd controls. I also expect that the SELinux crew would like to see a hook included there. > > Thanks > > Jeff > > >> -- >> paul-moore.com
On Tue, Dec 13, 2022 at 10:00 AM Jeff Xu <jeffxu@google.com> wrote: > On Fri, Dec 9, 2022 at 10:29 AM Paul Moore <paul@paul-moore.com> wrote: > > On Fri, Dec 9, 2022 at 11:05 AM <jeffxu@chromium.org> wrote: > > > > > > From: Jeff Xu <jeffxu@google.com> > > > > > > The new security_memfd_create allows lsm to check flags of > > > memfd_create. > > > > > > The security by default system (such as chromeos) can use this > > > to implement system wide lsm to allow only non-executable memfd > > > being created. > > > > > > Signed-off-by: Jeff Xu <jeffxu@google.com> > > > Reported-by: kernel test robot <lkp@intel.com> > > > --- > > > include/linux/lsm_hook_defs.h | 1 + > > > include/linux/lsm_hooks.h | 4 ++++ > > > include/linux/security.h | 6 ++++++ > > > mm/memfd.c | 5 +++++ > > > security/security.c | 5 +++++ > > > 5 files changed, 21 insertions(+) > > > > We typically require at least one in-tree LSM implementation to > > accompany a new LSM hook. Beyond simply providing proof that the hook > > has value, it helps provide a functional example both for reviewers as > > well as future LSM implementations. Also, while the BPF LSM is > > definitely "in-tree", its nature is such that the actual > > implementation lives out-of-tree; something like SELinux, AppArmor, > > Smack, etc. are much more desirable from an in-tree example > > perspective. > > Thanks for the comments. > Would that be OK if I add a new LSM in the kernel to block executable > memfd creation ? If you would be proposing the LSM only to meet the requirement of providing an in-tree LSM example, no that would definitely *not* be okay. Proposing a new LSM involves documenting a meaningful security model, implementing it, developing tests, going through a (likely multi-step) review process, and finally accepting the long term maintenance responsibilities of this new LSM. If you are proposing a new LSM because you feel the current LSMs do not provide a security model which meets your needs, then yes, proposing a new LSM might be a good idea. However, if you are proposing a new LSM because you don't want to learn how to add a new hook to an existing LSM, then I suspect you are misguided/misinformed with the amount of work involved in submitting a new LSM. > Alternatively, it might be possible to add this into SELinux or > landlock, it will be a larger change. It will be a much smaller change than submitting a new LSM, and it would have infinitely more value to the community than a throw-away LSM where the only use-case is getting your code merged upstream.
On Tue, Dec 13, 2022 at 11:22 AM Paul Moore <paul@paul-moore.com> wrote: > > On Tue, Dec 13, 2022 at 10:00 AM Jeff Xu <jeffxu@google.com> wrote: > > On Fri, Dec 9, 2022 at 10:29 AM Paul Moore <paul@paul-moore.com> wrote: > > > On Fri, Dec 9, 2022 at 11:05 AM <jeffxu@chromium.org> wrote: > > > > > > > > From: Jeff Xu <jeffxu@google.com> > > > > > > > > The new security_memfd_create allows lsm to check flags of > > > > memfd_create. > > > > > > > > The security by default system (such as chromeos) can use this > > > > to implement system wide lsm to allow only non-executable memfd > > > > being created. > > > > > > > > Signed-off-by: Jeff Xu <jeffxu@google.com> > > > > Reported-by: kernel test robot <lkp@intel.com> > > > > --- > > > > include/linux/lsm_hook_defs.h | 1 + > > > > include/linux/lsm_hooks.h | 4 ++++ > > > > include/linux/security.h | 6 ++++++ > > > > mm/memfd.c | 5 +++++ > > > > security/security.c | 5 +++++ > > > > 5 files changed, 21 insertions(+) > > > > > > We typically require at least one in-tree LSM implementation to > > > accompany a new LSM hook. Beyond simply providing proof that the hook > > > has value, it helps provide a functional example both for reviewers as > > > well as future LSM implementations. Also, while the BPF LSM is > > > definitely "in-tree", its nature is such that the actual > > > implementation lives out-of-tree; something like SELinux, AppArmor, > > > Smack, etc. are much more desirable from an in-tree example > > > perspective. > > > > Thanks for the comments. > > Would that be OK if I add a new LSM in the kernel to block executable > > memfd creation ? > > If you would be proposing the LSM only to meet the requirement of > providing an in-tree LSM example, no that would definitely *not* be > okay. > > Proposing a new LSM involves documenting a meaningful security model, > implementing it, developing tests, going through a (likely multi-step) > review process, and finally accepting the long term maintenance > responsibilities of this new LSM. If you are proposing a new LSM > because you feel the current LSMs do not provide a security model > which meets your needs, then yes, proposing a new LSM might be a good > idea. However, if you are proposing a new LSM because you don't want > to learn how to add a new hook to an existing LSM, then I suspect you > are misguided/misinformed with the amount of work involved in > submitting a new LSM. > > > Alternatively, it might be possible to add this into SELinux or > > landlock, it will be a larger change. > > It will be a much smaller change than submitting a new LSM, and it > would have infinitely more value to the community than a throw-away > LSM where the only use-case is getting your code merged upstream. > Thanks, my original thought is this LSM will be used by ChromeOS, since all of its memfd shall be non-executable. That said, I see the community will benefit more with this in SELinux. I will work to add this in SELinux, appreciate help while I'm learning to add this. Jeff > -- > paul-moore.com
diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h index ec119da1d89b..fd40840927c8 100644 --- a/include/linux/lsm_hook_defs.h +++ b/include/linux/lsm_hook_defs.h @@ -164,6 +164,7 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file) LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file) LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd, unsigned long arg) +LSM_HOOK(int, 0, memfd_create, char *name, unsigned int flags) LSM_HOOK(int, 0, mmap_addr, unsigned long addr) LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot, unsigned long prot, unsigned long flags) diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h index 4ec80b96c22e..5a18a6552278 100644 --- a/include/linux/lsm_hooks.h +++ b/include/linux/lsm_hooks.h @@ -543,6 +543,10 @@ * simple integer value. When @arg represents a user space pointer, it * should never be used by the security module. * Return 0 if permission is granted. + * @memfd_create: + * @name is the name of memfd file. + * @flags is the flags used in memfd_create. + * Return 0 if permission is granted. * @mmap_addr : * Check permissions for a mmap operation at @addr. * @addr contains virtual address that will be used for the operation. diff --git a/include/linux/security.h b/include/linux/security.h index ca1b7109c0db..5b87a780822a 100644 --- a/include/linux/security.h +++ b/include/linux/security.h @@ -384,6 +384,7 @@ int security_file_permission(struct file *file, int mask); int security_file_alloc(struct file *file); void security_file_free(struct file *file); int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg); +int security_memfd_create(char *name, unsigned int flags); int security_mmap_file(struct file *file, unsigned long prot, unsigned long flags); int security_mmap_addr(unsigned long addr); @@ -963,6 +964,11 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd, return 0; } +static inline int security_memfd_create(char *name, unsigned int flags) +{ + return 0; +} + static inline int security_mmap_file(struct file *file, unsigned long prot, unsigned long flags) { diff --git a/mm/memfd.c b/mm/memfd.c index 92f0a5765f7c..f04ed5f0474f 100644 --- a/mm/memfd.c +++ b/mm/memfd.c @@ -356,6 +356,11 @@ SYSCALL_DEFINE2(memfd_create, goto err_name; } + /* security hook for memfd_create */ + error = security_memfd_create(name, flags); + if (error) + return error; + if (flags & MFD_HUGETLB) { file = hugetlb_file_setup(name, 0, VM_NORESERVE, HUGETLB_ANONHUGE_INODE, diff --git a/security/security.c b/security/security.c index 79d82cb6e469..57788cf94075 100644 --- a/security/security.c +++ b/security/security.c @@ -1010,6 +1010,11 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb, } EXPORT_SYMBOL(security_sb_clone_mnt_opts); +int security_memfd_create(char *name, unsigned int flags) +{ + return call_int_hook(memfd_create, 0, name, flags); +} + int security_move_mount(const struct path *from_path, const struct path *to_path) { return call_int_hook(move_mount, 0, from_path, to_path);