linux-kernel - Re: [patch 1/2] autofs4: allow autofs to work outside the initial PID namespace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131116160334.GA19309@redhat.com>
Date:	Sat, 16 Nov 2013 17:03:34 +0100
From:	Oleg Nesterov <oleg@...hat.com>
To:	akpm@...ux-foundation.org
Cc:	raven@...maw.net, sukadev@...ibm.com, ebiederm@...ssion.com,
	mszeredi@...e.cz, serge.hallyn@...onical.com,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 1/2] autofs4: allow autofs to work outside the initial
	PID namespace

On 11/15, Andrew Morton wrote:
>
> Enable autofs4 to work in a "container".  oz_pgrp is converted from pid_t
> to struct pid and this is stored at mount time based on the "pgrp=" option
> or if the option is missing then the current pgrp.

I don't understand this code, so I am probably wrong. And this is minor
anyway, but...

> @@ -357,7 +358,17 @@ static int autofs_dev_ioctl_setpipefd(st
>  		mutex_unlock(&sbi->wq_mutex);
>  		return -EBUSY;
>  	} else {
> -		struct file *pipe = fget(pipefd);
> +		struct file *pipe;
> +
> +		new_pid = get_task_pid(current, PIDTYPE_PGID);
> +
> +		if (ns_of_pid(new_pid) != ns_of_pid(sbi->oz_pgrp)) {
> +			AUTOFS_WARN("Not allowed to change PID namespace");
> +			err = -EINVAL;
> +			goto out;
> +		}
> +
> +		pipe = fget(pipefd);
>  		if (!pipe) {
>  			err = -EBADF;
>  			goto out;
> @@ -367,12 +378,13 @@ static int autofs_dev_ioctl_setpipefd(st
>  			fput(pipe);
>  			goto out;
>  		}
> -		sbi->oz_pgrp = task_pgrp_nr(current);
> +		swap(sbi->oz_pgrp, new_pid);
>  		sbi->pipefd = pipefd;
>  		sbi->pipe = pipe;
>  		sbi->catatonic = 0;
>  	}
>  out:
> +	put_pid(new_pid);

This looks suspicious, put_pid() can actually kfree the old sbi->oz_pgrp
swapped above. IOW, this assumes we can't race with any user of ->oz_pgrp.

For example,

> @@ -80,7 +83,7 @@ static int autofs4_show_options(struct s
>  	if (!gid_eq(root_inode->i_gid, GLOBAL_ROOT_GID))
>  		seq_printf(m, ",gid=%u",
>  			from_kgid_munged(&init_user_ns, root_inode->i_gid));
> -	seq_printf(m, ",pgrp=%d", sbi->oz_pgrp);
> +	seq_printf(m, ",pgrp=%d", pid_vnr(sbi->oz_pgrp));

Can't this race with autofs_dev_ioctl_setpipefd() above?

Oleg.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/