Re: [CFT][PATCH 11/10] mnt: Avoid unnecessary regressions in fs_fully_visible (take 2)

On Wed, Jun 03, 2015 at 11:35:30PM -0500, Eric W. Biederman wrote:
> 
> Not allowing programs to clear nosuid and noexec on new mounts of
> sysfs or proc will cause lxc and libvirt-lxc to fail to start (a
> regression).  There are no executables files on sysfs or proc today
> which means clearing these flags is harmless today.
> 
> Instead of failing the fresh mounts of sysfs and proc emit a warning
> when these flags are improprely cleared.  We only reach this point
> because lxc and libvirt-lxc clear flags they mount flags had not
> intended to.
> 
> In a couple of kernel releases when lxc and libvirt-lxc have been
> fixed we can start failing fresh mounts proc and sysfs that clear
> nosuid and noexec.  Userspace clearly means to enforce those
> attributes and enforcing these attributes have historically avoided
> bugs in the setattr implementations of proc and sysfs.
> 
> Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Signed-off-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
> ---
> 
> Now with warning on problematic remounts as well.
> nodev is also ignored because it is not currently problematic.
> 
>  fs/namespace.c        | 33 +++++++++++++++++++++++++++++++++
>  include/linux/mount.h |  5 +++++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/fs/namespace.c b/fs/namespace.c
> index eccd925c6e82..3c3f8172c734 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -2162,6 +2162,18 @@ static int do_remount(struct path *path, int flags, int mnt_flags,
>  	    ((mnt->mnt.mnt_flags & MNT_ATIME_MASK) != (mnt_flags & MNT_ATIME_MASK))) {
>  		return -EPERM;
>  	}
> +	if ((mnt->mnt.mnt_flags & MNT_WARN_NOSUID) &&
> +	    !(mnt_flags & MNT_NOSUID) && printk_ratelimit()) {
> +		printk(KERN_INFO
> +		       "warning: process `%s' clears nosuid in remount of %s\n",
> +		       current->comm, sb->s_type->name);
> +	}
> +	if ((mnt->mnt.mnt_flags & MNT_WARN_NOEXEC) &&
> +	    !(mnt_flags & MNT_NOEXEC) && printk_ratelimit()) {
> +		printk(KERN_INFO
> +		       "warning: process `%s' clears noexec in remount of %s\n",
> +		       current->comm, sb->s_type->name);
> +	}
>  
>  	err = security_sb_remount(sb, data);
>  	if (err)
> @@ -3201,12 +3213,14 @@ static bool fs_fully_visible(struct file_system_type *type, int
*new_mnt_flags)
>  		if ((mnt->mnt.mnt_flags & MNT_LOCK_NODEV) &&
>  		    !(new_flags & MNT_NODEV))
>  			continue;
> +#if 0		/* Avoid unnecessary regressions */
>  		if ((mnt->mnt.mnt_flags & MNT_LOCK_NOSUID) &&
>  		    !(new_flags & MNT_NOSUID))
>  			continue;
>  		if ((mnt->mnt.mnt_flags & MNT_LOCK_NOEXEC) &&
>  		    !(new_flags & MNT_NOEXEC))
>  			continue;
> +#endif
>  		if ((mnt->mnt.mnt_flags & MNT_LOCK_ATIME) &&
>  		    ((mnt->mnt.mnt_flags & MNT_ATIME_MASK) != (new_flags & MNT_ATIME_MASK)))
>  			continue;
> @@ -3227,9 +3241,28 @@ static bool fs_fully_visible(struct file_system_type *type, int
*new_mnt_flags)
>  		/* Preserve the locked attributes */
>  		*new_mnt_flags |= mnt->mnt.mnt_flags & (MNT_LOCK_READONLY | \
>  							MNT_LOCK_NODEV    | \
> +						/* Avoid unnecessary regressions \
>  							MNT_LOCK_NOSUID   | \
>  							MNT_LOCK_NOEXEC   | \
> +						 */ \
>  							MNT_LOCK_ATIME);
> +		/* For now, warn about the "harmless" but invalid mnt flags */
> +		if (mnt->mnt.mnt_flags & MNT_LOCK_NOSUID) {
> +			*new_mnt_flags |= MNT_WARN_NOSUID;
> +			if (!(new_flags & MNT_NOSUID) && printk_ratelimit()) {
> +				printk(KERN_INFO
> +				       "warning: process `%s' clears nosuid in mount of %s\n",
> +				       current->comm, type->name);
> +			}
> +		}
> +		if (mnt->mnt.mnt_flags & MNT_LOCK_NOEXEC) {
> +			*new_mnt_flags |= MNT_WARN_NOEXEC;
> +			if (!(new_flags & MNT_NOEXEC) && printk_ratelimit()) {
> +				printk(KERN_INFO
> +				       "warning: process `%s' clears noexec in mount of %s\n",
> +				       current->comm, type->name);
> +			}
> +		}

Adding this to a stable kernel is not going to be ok, sorry.  We can't
start being noisy in system logs for things that were working just fine.

greg k-h