linux-kernel - Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 08 Nov 2013 15:54:07 -0800
From:	ebiederm@...ssion.com (Eric W. Biederman)
To:	"Serge E. Hallyn" <serge@...lyn.com>
Cc:	Aditya Kali <adityakali@...gle.com>, viro@...iv.linux.org.uk,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] proc: Fix namespace mountpoint path in /proc/mounts

"Serge E. Hallyn" <serge@...lyn.com> writes:

> Quoting Aditya Kali (adityakali@...gle.com):
>> Commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:
>> "proc: Fix the namespace inode permission checks." converted
>> the namespace files into symlinks. The same commit changed
>> the way namespace bind mounts appear in /proc/mounts:
>>   $ mount --bind /proc/self/ns/ipc /mnt/ipc
>> Originally:
>>   $ cat /proc/mounts | grep ipc
>>   proc /mnt/ipc proc rw,nosuid,nodev,noexec 0 0
>> 
>> After commit bf056bfa80596a5d14b26b17276a56a0dcb080e5:
>>   $ cat /proc/mounts | grep ipc
>>   proc ipc:[4026531839] proc rw,nosuid,nodev,noexec 0 0
>> 
>> This breaks userspace which expects the 2nd field in
>> /proc/mounts to be a valid path. This patch restores the
>> original format of namespace bind-mount entries in
>> /proc/mounts.
>
> Oh, at first I was thinking the mount source was showing up like that,
> not the target.  That is particularly ugly, I agree.
>
> I'm not sure what the purpose was of the ns_dname().  dcache.c says it's
> for filesystems wanting to do 'special "root names"'.  But this file
> gets mounted to real paths, it's not actually rootless (like a pipefs
> inode or anon_inode).  So I think your patch is correct, but I'm waiting
> to hear from Eric, as I'm not sure if you're masking some other effect
> which Eric actually wanted, and maybe this should be fixed another
> way...

My apologies for taking a long time to get back to this one.  I have
been scratching my head on this one.

There is most definitely a bug here, and worth fixing.

But I believe the bug is actually in buried in /proc/mounts. ns_dname
should be irrelevant as we are mounted.

The problem comes down to d_path.

I am not certain which is the best fix at the moment.  It should either
be a case of fixing d_path to see if the dentry is mounted, or making
certain that the dentries have the name ns_dname is giving them when
we allocate the dentries.

I was focusing on the what a ns file descriptor should look like when it
is opened but not mounted, when I wrote ns_dname, and that appearance
really should continue if possible.

I expect the easist way to fix this is to simply modify proc_ns_get_dentry to
compute the dentry name that ns_dname uses today, and pass that name to
d_alloc_psuedo.

At which point we can delete ns_dname without problems.

Eric


>> Signed-off-by: Aditya Kali <adityakali@...gle.com>
>> ---
>>  fs/proc/namespaces.c | 10 ----------
>>  1 file changed, 10 deletions(-)
>> 
>> diff --git a/fs/proc/namespaces.c b/fs/proc/namespaces.c
>> index 49a7fff..d19989d 100644
>> --- a/fs/proc/namespaces.c
>> +++ b/fs/proc/namespaces.c
>> @@ -48,19 +48,9 @@ static int ns_delete_dentry(const struct dentry *dentry)
>>  	return 1;
>>  }
>>  
>> -static char *ns_dname(struct dentry *dentry, char *buffer, int buflen)
>> -{
>> -	struct inode *inode = dentry->d_inode;
>> -	const struct proc_ns_operations *ns_ops = PROC_I(inode)->ns.ns_ops;
>> -
>> -	return dynamic_dname(dentry, buffer, buflen, "%s:[%lu]",
>> -		ns_ops->name, inode->i_ino);
>> -}
>> -
>>  const struct dentry_operations ns_dentry_operations =
>>  {
>>  	.d_delete	= ns_delete_dentry,
>> -	.d_dname	= ns_dname,
>>  };
>>  
>>  static struct dentry *proc_ns_get_dentry(struct super_block *sb,
>> -- 
>> 1.8.4.1
>> 
>> --
>> 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/
--
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/