[v3] mm/memfd: security hook for memfd_create

  From: Jeff Xu <jeffxu@chromium.org>

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@chromium.org>
---
 include/linux/lsm_hook_defs.h | 1 +
 include/linux/lsm_hooks.h     | 4 ++++
 include/linux/security.h      | 6 ++++++
 mm/memfd.c                    | 5 +++++
 4 files changed, 16 insertions(+)
  

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/jeffxu-chromium-org/mm-memfd-security-hook-for-memfd_create/20221202-094044
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20221202013404.163143-6-jeffxu%40google.com
patch subject: [PATCH v3] mm/memfd: security hook for memfd_create
config: x86_64-defconfig
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/63daa2490ad1a865f02ff504c8c3fcd6fd72c0c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review jeffxu-chromium-org/mm-memfd-security-hook-for-memfd_create/20221202-094044
        git checkout 63daa2490ad1a865f02ff504c8c3fcd6fd72c0c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: mm/memfd.o: in function `__do_sys_memfd_create':
>> memfd.c:(.text+0xe4): undefined reference to `security_memfd_create'
  

Hi,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on akpm-mm/mm-everything]

url:    https://github.com/intel-lab-lkp/linux/commits/jeffxu-chromium-org/mm-memfd-security-hook-for-memfd_create/20221202-094044
base:   https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git mm-everything
patch link:    https://lore.kernel.org/r/20221202013404.163143-6-jeffxu%40google.com
patch subject: [PATCH v3] mm/memfd: security hook for memfd_create
config: i386-randconfig-a001
compiler: gcc-11 (Debian 11.3.0-8) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/63daa2490ad1a865f02ff504c8c3fcd6fd72c0c3
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review jeffxu-chromium-org/mm-memfd-security-hook-for-memfd_create/20221202-094044
        git checkout 63daa2490ad1a865f02ff504c8c3fcd6fd72c0c3
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: mm/memfd.o: in function `__do_sys_memfd_create':
>> mm/memfd.c:316: undefined reference to `security_memfd_create'


vim +316 mm/memfd.c

   265	
   266	SYSCALL_DEFINE2(memfd_create,
   267			const char __user *, uname,
   268			unsigned int, flags)
   269	{
   270		unsigned int *file_seals;
   271		struct file *file;
   272		int fd, error;
   273		char *name;
   274		long len;
   275	
   276		if (!(flags & MFD_HUGETLB)) {
   277			if (flags & ~(unsigned int)MFD_ALL_FLAGS)
   278				return -EINVAL;
   279		} else {
   280			/* Allow huge page size encoding in flags. */
   281			if (flags & ~(unsigned int)(MFD_ALL_FLAGS |
   282					(MFD_HUGE_MASK << MFD_HUGE_SHIFT)))
   283				return -EINVAL;
   284		}
   285	
   286		/* length includes terminating zero */
   287		len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
   288		if (len <= 0)
   289			return -EFAULT;
   290		if (len > MFD_NAME_MAX_LEN + 1)
   291			return -EINVAL;
   292	
   293		name = kmalloc(len + MFD_NAME_PREFIX_LEN, GFP_KERNEL);
   294		if (!name)
   295			return -ENOMEM;
   296	
   297		strcpy(name, MFD_NAME_PREFIX);
   298		if (copy_from_user(&name[MFD_NAME_PREFIX_LEN], uname, len)) {
   299			error = -EFAULT;
   300			goto err_name;
   301		}
   302	
   303		/* terminating-zero may have changed after strnlen_user() returned */
   304		if (name[len + MFD_NAME_PREFIX_LEN - 1]) {
   305			error = -EFAULT;
   306			goto err_name;
   307		}
   308	
   309		fd = get_unused_fd_flags((flags & MFD_CLOEXEC) ? O_CLOEXEC : 0);
   310		if (fd < 0) {
   311			error = fd;
   312			goto err_name;
   313		}
   314	
   315		/* security hook for memfd_create */
 > 316		error = security_memfd_create(name, flags);
  

On Fri, Dec 02, 2022 at 01:34:03AM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@chromium.org>
> 
> 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@chromium.org>
> ---
>  include/linux/lsm_hook_defs.h | 1 +
>  include/linux/lsm_hooks.h     | 4 ++++
>  include/linux/security.h      | 6 ++++++
>  mm/memfd.c                    | 5 +++++
>  4 files changed, 16 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;
> +}

I think this is missing the security/security.c changes for the
non-inline version?

-Kees

> +
>  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 69e897dea6d5..96dcfbfed09e 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -346,6 +346,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,
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
>
  

On Fri, Dec 2, 2022 at 2:58 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Dec 02, 2022 at 01:34:03AM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@chromium.org>
> >
> > 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@chromium.org>
> > ---
> >  include/linux/lsm_hook_defs.h | 1 +
> >  include/linux/lsm_hooks.h     | 4 ++++
> >  include/linux/security.h      | 6 ++++++
> >  mm/memfd.c                    | 5 +++++
> >  4 files changed, 16 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;
> > +}
>
> I think this is missing the security/security.c changes for the
> non-inline version?
>
Yes. I will add that in V4.

> -Kees
>
> > +
> >  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 69e897dea6d5..96dcfbfed09e 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -346,6 +346,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,
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog
> >
>
> --
> Kees Cook