ACK: [Trusty SRU][PATCH 1/2] vfs: Commit to never having exectuables on proc and sysfs.

Colin Ian King colin.king at canonical.com
Tue Sep 5 09:29:15 UTC 2017


On 04/09/17 18:54, Kleber Sacilotto de Souza wrote:
> From: "Eric W. Biederman" <ebiederm at xmission.com>
> 
> CVE-2016-10044
> 
> Today proc and sysfs do not contain any executable files.  Several
> applications today mount proc or sysfs without noexec and nosuid and
> then depend on there being no exectuables files on proc or sysfs.
> Having any executable files show on proc or sysfs would cause
> a user space visible regression, and most likely security problems.
> 
> Therefore commit to never allowing executables on proc and sysfs by
> adding a new flag to mark them as filesystems without executables and
> enforce that flag.
> 
> Test the flag where MNT_NOEXEC is tested today, so that the only user
> visible effect will be that exectuables will be treated as if the
> execute bit is cleared.
> 
> The filesystems proc and sysfs do not currently incoporate any
> executable files so this does not result in any user visible effects.
> 
> This makes it unnecessary to vet changes to proc and sysfs tightly for
> adding exectuable files or changes to chattr that would modify
> existing files, as no matter what the individual file say they will
> not be treated as exectuable files by the vfs.
> 
> Not having to vet changes to closely is important as without this we
> are only one proc_create call (or another goof up in the
> implementation of notify_change) from having problematic executables
> on proc.  Those mistakes are all too easy to make and would create
> a situation where there are security issues or the assumptions of
> some program having to be broken (and cause userspace regressions).
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm at xmission.com>
> (backported from commit 90f8572b0f021fdd1baa68e00a8c30482ee9e5f4)
> Signed-off-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
> ---
>  fs/exec.c           | 10 ++++++++--
>  fs/open.c           |  2 +-
>  fs/proc/root.c      |  2 ++
>  fs/sysfs/mount.c    |  3 +++
>  include/linux/fs.h  |  2 ++
>  kernel/sys.c        |  3 +--
>  mm/mmap.c           |  4 ++--
>  mm/nommu.c          |  2 +-
>  security/security.c |  2 +-
>  9 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index dd7ab64bd5a2..f96daeca68a9 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -107,6 +107,12 @@ bool path_nosuid(const struct path *path)
>  }
>  EXPORT_SYMBOL(path_nosuid);
>  
> +bool path_noexec(const struct path *path)
> +{
> +	return (path->mnt->mnt_flags & MNT_NOEXEC) ||
> +	       (path->mnt->mnt_sb->s_iflags & SB_I_NOEXEC);
> +}
> +
>  /*
>   * Note that a shared library must be both readable and executable due to
>   * security reasons.
> @@ -140,7 +146,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
>  		goto exit;
>  
>  	error = -EACCES;
> -	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (path_noexec(&file->f_path))
>  		goto exit;
>  
>  	fsnotify_open(file);
> @@ -798,7 +804,7 @@ struct file *open_exec(const char *name)
>  	if (!S_ISREG(file_inode(file)->i_mode))
>  		goto exit;
>  
> -	if (file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (path_noexec(&file->f_path))
>  		goto exit;
>  
>  	fsnotify_open(file);
> diff --git a/fs/open.c b/fs/open.c
> index 6dcf14157e27..d8aa5ebbbf84 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -344,7 +344,7 @@ retry:
>  		 * with the "noexec" flag.
>  		 */
>  		res = -EACCES;
> -		if (path.mnt->mnt_flags & MNT_NOEXEC)
> +		if (path_noexec(&path))
>  			goto out_path_release;
>  	}
>  
> diff --git a/fs/proc/root.c b/fs/proc/root.c
> index c198775a5617..3ce05378b960 100644
> --- a/fs/proc/root.c
> +++ b/fs/proc/root.c
> @@ -139,6 +139,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type,
>  		}
>  
>  		sb->s_flags |= MS_ACTIVE;
> +		/* User space would break if executables appear on proc */
> +		sb->s_iflags |= SB_I_NOEXEC;
>  	}
>  
>  	return dget(sb->s_root);
> diff --git a/fs/sysfs/mount.c b/fs/sysfs/mount.c
> index f4d0799b5779..d705cabe9d6a 100644
> --- a/fs/sysfs/mount.c
> +++ b/fs/sysfs/mount.c
> @@ -139,6 +139,9 @@ static struct dentry *sysfs_mount(struct file_system_type *fs_type,
>  		}
>  		sb->s_flags |= MS_ACTIVE;
>  	}
> +	if (sb->s_root)
> +		/* Userspace would break if executables appear on sysfs */
> +		sb->s_root->d_sb->s_iflags |= SB_I_NOEXEC;
>  
>  	return dget(sb->s_root);
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 7e76dbc028d5..67a0bb3f3b19 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1221,6 +1221,7 @@ extern struct list_head super_blocks;
>  extern spinlock_t sb_lock;
>  
>  /* sb->s_iflags */
> +#define SB_I_NOEXEC	0x00000002	/* Ignore executables on this fs */
>  #define SB_I_NOSUID	0x00000004	/* Ignore suid on this fs */
>  
>  /* Possible states of 'frozen' field */
> @@ -2831,5 +2832,6 @@ static inline bool dir_relax(struct inode *inode)
>  }
>  
>  extern bool path_nosuid(const struct path *path);
> +extern bool path_noexec(const struct path *path);
>  
>  #endif /* _LINUX_FS_H */
> diff --git a/kernel/sys.c b/kernel/sys.c
> index 17b7417d0bf6..3b663a209691 100644
> --- a/kernel/sys.c
> +++ b/kernel/sys.c
> @@ -1648,8 +1648,7 @@ static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
>  	 * overall picture.
>  	 */
>  	err = -EACCES;
> -	if (!S_ISREG(inode->i_mode)	||
> -	    exe.file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +	if (!S_ISREG(inode->i_mode) || path_noexec(&exe.file->f_path))
>  		goto exit;
>  
>  	err = inode_permission(inode, MAY_EXEC);
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 72f35bce0f16..8cbdd057bf83 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -1261,7 +1261,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>  	 *  mounted, in which case we dont add PROT_EXEC.)
>  	 */
>  	if ((prot & PROT_READ) && (current->personality & READ_IMPLIES_EXEC))
> -		if (!(file && (file->f_path.mnt->mnt_flags & MNT_NOEXEC)))
> +		if (!(file && path_noexec(&file->f_path)))
>  			prot |= PROT_EXEC;
>  
>  	if (!len)
> @@ -1341,7 +1341,7 @@ unsigned long do_mmap_pgoff(struct file *file, unsigned long addr,
>  		case MAP_PRIVATE:
>  			if (!(file->f_mode & FMODE_READ))
>  				return -EACCES;
> -			if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
> +			if (path_noexec(&file->f_path)) {
>  				if (vm_flags & VM_EXEC)
>  					return -EPERM;
>  				vm_flags &= ~VM_MAYEXEC;
> diff --git a/mm/nommu.c b/mm/nommu.c
> index 4efb31662bd1..e3053616ffad 100644
> --- a/mm/nommu.c
> +++ b/mm/nommu.c
> @@ -1031,7 +1031,7 @@ static int validate_mmap_request(struct file *file,
>  
>  		/* handle executable mappings and implied executable
>  		 * mappings */
> -		if (file->f_path.mnt->mnt_flags & MNT_NOEXEC) {
> +		if (path_noexec(&file->f_path)) {
>  			if (prot & PROT_EXEC)
>  				return -EPERM;
>  		}
> diff --git a/security/security.c b/security/security.c
> index ae6eba6b010d..79d239fdba70 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -719,7 +719,7 @@ static inline unsigned long mmap_prot(struct file *file, unsigned long prot)
>  	 * ditto if it's not on noexec mount, except that on !MMU we need
>  	 * BDI_CAP_EXEC_MMAP (== VM_MAYEXEC) in this case
>  	 */
> -	if (!(file->f_path.mnt->mnt_flags & MNT_NOEXEC)) {
> +	if (!path_noexec(&file->f_path)) {
>  #ifndef CONFIG_MMU
>  		unsigned long caps = 0;
>  		struct address_space *mapping = file->f_mapping;
> 
Looks like a sane backport to me.

Acked-by: Colin Ian King <colin.king at canonical.com>




More information about the kernel-team mailing list