Re: [RFC] c/r: prctl: Add ability to set new mm_struct::exe_file

[Oleg Nesterov <oleg@redhat.com>]

On 03/04, Cyrill Gorcunov wrote:
>
> +static int prctl_set_mm_exe_file(struct mm_struct *mm, unsigned int fd)
> +{
> +	struct file *exe_file;
> +	struct dentry *dentry;
> +	int err;
> +
> +	exe_file = fget(fd);
> +	if (!exe_file)
> +		return -EBADF;
> +
> +	dentry = exe_file->f_path.dentry;
> +
> +	/*
> +	 * Permissions should be the same as if the
> +	 * file has being opened by the kernel for
> +	 * execution.
> +	 */

Why?

> +	err = -EACCES;
> +	if (!S_ISREG(dentry->d_inode->i_mode)	||
> +	    exe_file->f_path.mnt->mnt_flags & MNT_NOEXEC)
> +		goto exit;
> +
> +	if ((exe_file->f_flags & O_ACCMODE) != O_RDONLY)
> +		goto exit;
> +
> +	err = inode_permission(dentry->d_inode, MAY_EXEC);
> +	if (err)
> +		goto exit;

OK, I won't argue, probably this makes sense to make sure that
admin can't get a heart attack looking at /proc/pid/exe.

But the O_RDONLY check looks strange. We are not going to write
to this file, we only set the name (and that is why I think it
should be mm->exe_path). What is the point to check that the file
was opened without FMODE_WRITE? Even if there were any security
risk the apllication can open this file again with the different
flags.

And btw this check is redundant anyway because you do
deny_write_access() below. However, this deny_write_access() looks
wrong:

> +	down_write(&mm->mmap_sem);
> +	if (mm->num_exe_file_vmas) {
> +
> +		err = deny_write_access(exe_file);

And who does allow_write_access() ?

Oleg.