Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)

PATCH Applied Aurélien Aptel 6 participants 20 messages May 27, 2016 – Jul 29, 2016 10 messages not in the archive

Subsystems (2)

SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Aurélien Aptel

author

2016-05-27 17:43 UTC #0 lore raw

PATCH 1/1 Making shares unaccessible at root level mountable (aka solving bsc#8950 ...again)

Hi all,

I've come up with a new solution for the problem (attached). Between the long, old and irrelevant comments on the bug report and the various mailing-list threads it was getting hard to understand. So I've written a summary which explains the problem in details and the 2 proposed solutions.

http://diobla.info/stuff/bugs/bsc799133/

The new solution is basically switching to the old prefixpath system when the normal method of querying intermediary path fails. It's much more simpler.

*Any* feedback would be nice. I haven't noticed any problems so far but I haven't run the xfs test suite (I still have to figure out how to make it play nice with cifs...). I'm sending to samba-tech too in hopes of reviews/comments.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From 3e9af75bea055bf88a4700fced50a7ba38b39b6f Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but not any of the path components above:

- store the /sub/dir/foo prefix in the cifs super_block info
- in the superblock, set root dentry to the subpath dentry (instead of the share root)
- set a flag in the superblock to remember it
- use prefixpath when building path from a dentry

fixes bso#8950

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 19 +++++++++++++++++++
 fs/cifs/connect.c    |  3 +++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 4 files changed, 41 insertions(+), 2 deletions(-)

fs/cifs/cifs_fs_sb.h +2 -0

index 3182273..02b9ac3 100644
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;

@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */

fs/cifs/cifsfs.c +19 -0

index 5d8b7ed..d1fc593 100644
@@ -649,6 +649,17 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
+	if (IS_ERR(dentry) /* && path accessible */) {
+		/* we know the path is accessible (it is tested
+		 * earlier in cifs_do_mount()) so there must be a perm
+		 * problem */
+		cifs_dbg(VFS, "cannot query directories between root and final path, "
+			 "enabling CIFS_MOUNT_USE_PREFIX_PATH\n");
+		cifs_sb->mnt_cifs_flags |= CIFS_MOUNT_USE_PREFIX_PATH;
+		dentry = dget(sb->s_root);
+	}
+
 	return dentry;
 }

@@ -688,6 +699,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);

fs/cifs/connect.c +3 -0

index 66736f5..90e57ee 100644
@@ -3887,6 +3887,9 @@ cifs_umount(struct cifs_sb_info *cifs_sb)
 
 	bdi_destroy(&cifs_sb->bdi);
 	kfree(cifs_sb->mountdata);
+	if (cifs_sb->prepath) {
+		kfree(cifs_sb->prepath);
+	}
 	call_rcu(&cifs_sb->rcu, delayed_free);
 }

fs/cifs/dir.c +17 -2

index c3eb998..5374253 100644
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);

@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {

@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename

@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {

Signature · 1 line

2.1.4

Aurélien Aptel

author

2016-06-09 16:50 UTC #1 lore raw

Small update: I've written a powershell script to reproduce the problem (attached). If you're wondering I'm not using samba see my notes about it [1].

On the window server:
- Edit $Dir (script will create parent dirs)
- Edit $LimitedUser/$AdminUser to an existing one
- Run the script as admin

On the linux client:
- Mount the share sub dir with the limited user credentials:

  mount //lutze/bug8950/sub/dir' /mnt \
        -o 'domain=LURCH,ip=10.160.5.42,username=bill,password=*****,rw'

My second solution fails for the case when the dir *containing* the shared dir restricts the limited user. See "HARD MODE" at the end of the script.

1: http://diobla.info/stuff/bugs/bsc799133/#sec-4

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

Marcus Hoffmann

2016-06-09 19:27 UTC #2 lore raw

Hey Aurélien,
with your script I can reproduce the bug locally now.

I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the restricted user.

(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)

Marcus

Quoted 20 lines · Aurélien Aptel

Small update: I've written a powershell script to reproduce the problem (attached). If you're wondering I'm not using samba see my notes about it [1].
On the window server:
- Edit $Dir (script will create parent dirs)
- Edit $LimitedUser/$AdminUser to an existing one
- Run the script as admin
On the linux client:
- Mount the share sub dir with the limited user credentials:
  mount //lutze/bug8950/sub/dir' /mnt \
        -o 'domain=LURCH,ip=10.160.5.42,username=bill,password=*****,rw'
My second solution fails for the case when the dir *containing* the shared dir restricts the limited user. See "HARD MODE" at the end of the script.
1: http://diobla.info/stuff/bugs/bsc799133/#sec-4

A message that is not in the archive (sent off-list or not yet received): 5759C326.5040508-j/[email protected]

Aurélien Aptel

author

2016-06-10 15:16 UTC #4 lore raw

Quoted 2 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…

Hey Aurélien,
with your script I can reproduce the bug locally now.

Good.

Quoted 4 lines

I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the restricted user.

I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.

I've updated the cifs_root_iget() to use the prefix path when necessary which should take care of the last case (hard mode).

Please test my latest patch (attached).

Quoted 2 lines

(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)

Indeed.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From e858c28b7bf9b1c76c0a9703727c7bd02bf4a434 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but not any of the path components above:

fixes bso#8950

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 15 ++++++++++++++-
 fs/cifs/connect.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 fs/cifs/inode.c      | 17 +++++++++++++++--
 5 files changed, 95 insertions(+), 5 deletions(-)

fs/cifs/cifs_fs_sb.h +2 -0

index 3182273..02b9ac3 100644
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;

@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */

fs/cifs/cifsfs.c +14 -1

index 5d8b7ed..c75d80c 100644
@@ -649,6 +649,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
 	return dentry;
 }

@@ -688,6 +689,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);

@@ -726,7 +735,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) {
+		root = dget(sb->s_root);
+	} else {
+		root = cifs_get_root(volume_info, sb);
+	}
 	if (IS_ERR(root))
 		goto out_super;

fs/cifs/connect.c +47 -0

65 lines not on this page · raw message

fs/cifs/dir.c +17 -2

index c3eb998..5374253 100644
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);

@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {

@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename

@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {

fs/cifs/inode.c +15 -2

index 514dadb..7c6edd7 100644
@@ -1002,10 +1002,21 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;

@@ -1013,7 +1024,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {

@@ -1042,6 +1054,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */

Signature · 1 line

2.1.4

Marcus Hoffmann

2016-06-12 18:01 UTC #5 lore raw

Quoted 12 lines · Aurélien Aptel

Quoted 2 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 4 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the restricted user.

Quoted 8 lines

I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary which should take care of the last case (hard mode).
Please test my latest patch (attached).

I can confirm it works with our setup now.
Any chance in getting this merged?

Quoted 6 lines

Quoted 3 lines
(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)
Indeed.

Thanks for your work on this,
Marcus

Marcus Hoffmann

2016-07-01 15:44 UTC #6 lore raw

In reply to Aurélien Aptel

Quoted 12 lines · Aurélien Aptel

Quoted 2 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 4 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the restricted user.

Quoted 9 lines

I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary which should take care of the last case (hard mode).
Please test my latest patch (attached).

I just wanted to ask what can be done to get this merged.

Quoted 5 lines

Quoted 2 lines
(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)
Indeed.

Marcus

A message that is not in the archive (sent off-list or not yet received): 57768FC3.7020102-j/[email protected]

Steve French

2016-07-01 16:02 UTC #8 lore raw

I will take a look later today.

Quoted 31 lines · Marcus Hoffmann

Quoted 12 lines · Aurélien Aptel
Quoted 2 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 4 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows 7 PC with the restricted user account. (Even in hard mode.)
I can mount the share from Linux-cifs using the admin user but not the restricted user.
Quoted 9 lines
I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary which should take care of the last case (hard mode).
Please test my latest patch (attached).
I just wanted to ask what can be done to get this merged.
Quoted 5 lines
Quoted 2 lines
(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)
Indeed.
Marcus

Signature · 3 lines

Thanks,

Steve

Pavel Shilovsky

2016-07-02 07:02 UTC #9 lore raw

In reply to Aurélien Aptel

2016-06-10 18:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/[email protected]>:
...

Quoted 8 lines

I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary which should take care of the last case (hard mode).
Please test my latest patch (attached).

Hi Aurélien,

Thank you for the patch, it looks good like a right thing to do.

I put my comments below:

@@ -649,6 +649,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
dentry = child;
} while (!IS_ERR(dentry));
kfree(full_path);
+
return dentry;
}

Please remove this unnecessary change - probably it will go to stable some day and may cause extra conflicts.

@@ -1002,10 +1002,21 @@ struct inode *cifs_root_iget(struct super_block *sb)
struct inode *inode = NULL;
long rc;
struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+ char *path = NULL;
+ int len;
+
+ if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) && cifs_sb->prepath) {
+ len = strlen(cifs_sb->prepath);
+ path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+ path[0] = '/';
+ memcpy(path+1, cifs_sb->prepath, len);
+ } else {
+ path = kstrdup("", GFP_KERNEL);
+ }

The above code should check for possible memory allocation failure.

Signature · 2 lines

Best regards,
Pavel Shilovsky

A message that is not in the archive (sent off-list or not yet received): CAKywueRMvJ4B6ojqA1TduS4nGFTr5m4wLO2=0M_EVv=vw2T1pw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Aurélien Aptel

author

2016-07-18 14:38 UTC #11 lore raw

Quoted 4 lines · On Sat, 2 Jul 2016 10:02:36 +0300 Pavel Shilovsky…

Please remove this unnecessary change - probably it will go to stable some day and may cause extra conflicts.
The above code should check for possible memory allocation failure.

I've added (new patch attached) NULL checks and removed the gratuitous empty line.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From 46164fc278332f9dce945271048ec6f530d52635 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but not any of the path components above:

fixes bso#8950

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifs_fs_sb.h |  2 ++
 fs/cifs/cifsfs.c     | 15 ++++++++++++++-
 fs/cifs/connect.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 19 +++++++++++++++++--
 fs/cifs/inode.c      | 23 +++++++++++++++++++++--
 5 files changed, 101 insertions(+), 5 deletions(-)

fs/cifs/cifs_fs_sb.h +2 -0

index 3182273..02b9ac3 100644
@@ -46,6 +46,7 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible root mountable */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;

@@ -67,5 +68,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */

fs/cifs/cifsfs.c +14 -1

index 5d841f3..cce6eac 100644
@@ -650,6 +650,7 @@ cifs_get_root(struct smb_vol *vol, struct super_block *sb)
 		dentry = child;
 	} while (!IS_ERR(dentry));
 	kfree(full_path);
+
 	return dentry;
 }

@@ -689,6 +690,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);

@@ -727,7 +736,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) {
+		root = dget(sb->s_root);
+	} else {
+		root = cifs_get_root(volume_info, sb);
+	}
 	if (IS_ERR(root))
 		goto out_super;

fs/cifs/connect.c +47 -0

65 lines not on this page · raw message

fs/cifs/dir.c +17 -2

index fb0903f..fb4de01 100644
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);

@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {

@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename

@@ -153,6 +158,16 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {

fs/cifs/inode.c +21 -2

index 514dadb..f2f4af0 100644
@@ -1002,10 +1002,27 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH) && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		if (path == NULL) {
+			return ERR_PTR(-ENOMEM);
+		}
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+		if (path == NULL) {
+			return ERR_PTR(-ENOMEM);
+		}
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;

@@ -1013,7 +1030,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {

@@ -1042,6 +1060,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */

Signature · 1 line

2.1.4

Pavel Shilovsky

2016-07-19 19:21 UTC #12 lore raw

2016-07-18 17:38 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/[email protected]>:

Quoted 10 lines

Quoted 4 lines · On Sat, 2 Jul 2016 10:02:36 +0300 Pavel Shilovsky…
Please remove this unnecessary change - probably it will go to stable some day and may cause extra conflicts.
The above code should check for possible memory allocation failure.
I've added (new patch attached) NULL checks and removed the gratuitous empty line.

Quoted 6 lines

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

I suggest you to run scripts/checkpatch.pl against your patch - it has several warnings like {} braces for single statement blocks. Also the patch from the attachment still has an empty line in fs/cifs/cifsfs.c.

Signature · 2 lines

Best regards,
Pavel Shilovsky

A message that is not in the archive (sent off-list or not yet received): CAKywueRFMu9nvwi_01Yz0HpOqhrK2yZVaLT2JMqw4622irQzNw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Aurélien Aptel

author

2016-07-20 10:57 UTC #14 lore raw

Quoted 3 lines · On Tue, 19 Jul 2016 22:21:26 +0300 Pavel Shilovsky…

I suggest you to run scripts/checkpatch.pl against your patch - it has several warnings like {} braces for single statement blocks. Also the patch from the attachment still has an empty line in fs/cifs/cifsfs.c.

Oops. Okay this time checkpatch.pl only complains about a quoted string split across lines wich I'm not really sure how to fix so I'm leaving that as is.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

From 8035f8f1b8771e93fb66b4fe8db954fb0c5d5f43 Mon Sep 17 00:00:00 2001
From: Aurelien Aptel <aaptel-IBi9RG/[email protected]>
Date: Wed, 25 May 2016 19:59:09 +0200
Subject: [PATCH] fs/cifs: make share unaccessible at root level mountable

if, when mounting //HOST/share/sub/dir/foo we can query /sub/dir/foo but not any of the path components above:

fixes bso#8950

Signed-off-by Aurelien Aptel <aaptel-IBi9RG/[email protected]>

---
 fs/cifs/cifs_fs_sb.h |  4 ++++
 fs/cifs/cifsfs.c     | 14 +++++++++++++-
 fs/cifs/connect.c    | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 fs/cifs/dir.c        | 20 ++++++++++++++++++--
 fs/cifs/inode.c      | 22 ++++++++++++++++++++--
 5 files changed, 104 insertions(+), 5 deletions(-)

fs/cifs/cifs_fs_sb.h +4 -0

index 3182273..1418daa 100644
@@ -46,6 +46,9 @@
 #define CIFS_MOUNT_CIFS_BACKUPUID 0x200000 /* backup intent bit for a user */
 #define CIFS_MOUNT_CIFS_BACKUPGID 0x400000 /* backup intent bit for a group */
 #define CIFS_MOUNT_MAP_SFM_CHR	0x800000 /* SFM/MAC mapping for illegal chars */
+#define CIFS_MOUNT_USE_PREFIX_PATH 0x1000000 /* make subpath with unaccessible
+					      * root mountable
+					      */
 
 struct cifs_sb_info {
 	struct rb_root tlink_tree;

@@ -67,5 +70,6 @@ struct cifs_sb_info {
 	struct backing_dev_info bdi;
 	struct delayed_work prune_tlinks;
 	struct rcu_head rcu;
+	char *prepath;
 };
 #endif				/* _CIFS_FS_SB_H */

fs/cifs/cifsfs.c +13 -1

index 5d841f3..6bbec5e 100644
@@ -689,6 +689,14 @@ cifs_do_mount(struct file_system_type *fs_type,
 		goto out_cifs_sb;
 	}
 
+	if (volume_info->prepath) {
+		cifs_sb->prepath = kstrdup(volume_info->prepath, GFP_KERNEL);
+		if (cifs_sb->prepath == NULL) {
+			root = ERR_PTR(-ENOMEM);
+			goto out_cifs_sb;
+		}
+	}
+
 	cifs_setup_cifs_sb(volume_info, cifs_sb);
 
 	rc = cifs_mount(cifs_sb, volume_info);

@@ -727,7 +735,11 @@ cifs_do_mount(struct file_system_type *fs_type,
 		sb->s_flags |= MS_ACTIVE;
 	}
 
-	root = cifs_get_root(volume_info, sb);
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		root = dget(sb->s_root);
+	else
+		root = cifs_get_root(volume_info, sb);
+
 	if (IS_ERR(root))
 		goto out_super;

fs/cifs/connect.c +49 -0

67 lines not on this page · raw message

fs/cifs/dir.c +18 -2

index fb0903f..b8dbbe0 100644
@@ -84,6 +84,7 @@ build_path_from_dentry(struct dentry *direntry)
 	struct dentry *temp;
 	int namelen;
 	int dfsplen;
+	int pplen = 0;
 	char *full_path;
 	char dirsep;
 	struct cifs_sb_info *cifs_sb = CIFS_SB(direntry->d_sb);

@@ -95,8 +96,12 @@ build_path_from_dentry(struct dentry *direntry)
 		dfsplen = strnlen(tcon->treeName, MAX_TREE_SIZE + 1);
 	else
 		dfsplen = 0;
+
+	if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+		pplen = cifs_sb->prepath ? strlen(cifs_sb->prepath) + 1 : 0;
+
 cifs_bp_rename_retry:
-	namelen = dfsplen;
+	namelen = dfsplen + pplen;
 	seq = read_seqbegin(&rename_lock);
 	rcu_read_lock();
 	for (temp = direntry; !IS_ROOT(temp);) {

@@ -137,7 +142,7 @@ cifs_bp_rename_retry:
 		}
 	}
 	rcu_read_unlock();
-	if (namelen != dfsplen || read_seqretry(&rename_lock, seq)) {
+	if (namelen != dfsplen + pplen || read_seqretry(&rename_lock, seq)) {
 		cifs_dbg(FYI, "did not end path lookup where expected. namelen=%ddfsplen=%d\n",
 			 namelen, dfsplen);
 		/* presumably this is only possible if racing with a rename

@@ -153,6 +158,17 @@ cifs_bp_rename_retry:
 	   those safely to '/' if any are found in the middle of the prepath */
 	/* BB test paths to Windows with '/' in the midst of prepath */
 
+	if (pplen) {
+		int i;
+
+		cifs_dbg(FYI, "using cifs_sb prepath <%s>\n", cifs_sb->prepath);
+		memcpy(full_path+dfsplen+1, cifs_sb->prepath, pplen-1);
+		full_path[dfsplen] = '\\';
+		for (i = 0; i < pplen-1; i++)
+			if (full_path[dfsplen+1+i] == '/')
+				full_path[dfsplen+1+i] = CIFS_DIR_SEP(cifs_sb);
+	}
+
 	if (dfsplen) {
 		strncpy(full_path, tcon->treeName, dfsplen);
 		if (cifs_sb->mnt_cifs_flags & CIFS_MOUNT_POSIX_PATHS) {

fs/cifs/inode.c +20 -2

index 514dadb..b87efd0 100644
@@ -1002,10 +1002,26 @@ struct inode *cifs_root_iget(struct super_block *sb)
 	struct inode *inode = NULL;
 	long rc;
 	struct cifs_tcon *tcon = cifs_sb_master_tcon(cifs_sb);
+	char *path = NULL;
+	int len;
+
+	if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_USE_PREFIX_PATH)
+	    && cifs_sb->prepath) {
+		len = strlen(cifs_sb->prepath);
+		path = kzalloc(len + 2 /* leading sep + null */, GFP_KERNEL);
+		if (path == NULL)
+			return ERR_PTR(-ENOMEM);
+		path[0] = '/';
+		memcpy(path+1, cifs_sb->prepath, len);
+	} else {
+		path = kstrdup("", GFP_KERNEL);
+		if (path == NULL)
+			return ERR_PTR(-ENOMEM);
+	}
 
 	xid = get_xid();
 	if (tcon->unix_ext) {
-		rc = cifs_get_inode_info_unix(&inode, "", sb, xid);
+		rc = cifs_get_inode_info_unix(&inode, path, sb, xid);
 		/* some servers mistakenly claim POSIX support */
 		if (rc != -EOPNOTSUPP)
 			goto iget_no_retry;

@@ -1013,7 +1029,8 @@ struct inode *cifs_root_iget(struct super_block *sb)
 		tcon->unix_ext = false;
 	}
 
-	rc = cifs_get_inode_info(&inode, "", NULL, sb, xid, NULL);
+	convert_delimiter(path, CIFS_DIR_SEP(cifs_sb));
+	rc = cifs_get_inode_info(&inode, path, NULL, sb, xid, NULL);
 
 iget_no_retry:
 	if (!inode) {

@@ -1042,6 +1059,7 @@ iget_no_retry:
 	}
 
 out:
+	kfree(path);
 	/* can not call macro free_xid here since in a void func
 	 * TODO: This is no longer true
 	 */

Signature · 1 line

2.1.4

Aurélien Aptel

author

2016-07-20 12:16 UTC #15 lore raw

Quoted 3 lines · On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel…

Oops. Okay this time checkpatch.pl only complains about a quoted string split across lines wich I'm not really sure how to fix so I'm leaving that as is.

...because not splitting it would make the line >80 chars. The whole point of doing this is AFAIU to be able to grep the message so using "\" wouldn't fix it really.

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

Pavel Shilovsky

Reviewed-by

2016-07-20 18:28 UTC #16 lore raw

2016-07-20 15:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/[email protected]>:

Quoted 10 lines

Quoted 3 lines · On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel <[email protected]> wrote:
Oops. Okay this time checkpatch.pl only complains about a quoted string split across lines wich I'm not really sure how to fix so I'm leaving that as is.
...because not splitting it would make the line >80 chars. The whole point of doing this is AFAIU to be able to grep the message so using "\" wouldn't fix it really.

Quoted 6 lines

--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)

The quoted string should not be split even if it is >80 chars.

Anyway, the patch looks good. Thanks for fixing this long-term issue.

Reviewed-by Pavel Shilovsky <[email protected]>

Signature · 2 lines

Best regards,
Pavel Shilovsky

A message that is not in the archive (sent off-list or not yet received): CAKywueTOSD0G1k+EU-Qo_9D7S5bBw6g6T=dbQpWYWdOhr5Lsrg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Steve French

2016-07-26 18:04 UTC #18 lore raw

thoughts on stable?

Quoted 27 lines · Pavel Shilovsky

2016-07-20 15:16 GMT+03:00 Aurélien Aptel <aaptel-IBi9RG/[email protected]>:
Quoted 10 lines
Quoted 3 lines · On Wed, 20 Jul 2016 12:57:02 +0200 Aurélien Aptel <[email protected]> wrote:
Oops. Okay this time checkpatch.pl only complains about a quoted string split across lines wich I'm not really sure how to fix so I'm leaving that as is.
...because not splitting it would make the line >80 chars. The whole point of doing this is AFAIU to be able to grep the message so using "\" wouldn't fix it really.
Quoted 6 lines
--
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97 8C99 03C8 A49B 521B D5D3 SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nürnberg)
The quoted string should not be split even if it is >80 chars.
Anyway, the patch looks good. Thanks for fixing this long-term issue.
Reviewed-by Pavel Shilovsky <[email protected]>
--
Best regards,
Pavel Shilovsky

Signature · 3 lines

Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mviretFGDaHOre8BiZLmKhqwnfv9sdaiqoAG1xahbVjKA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Pavel Shilovsky

2016-07-26 19:10 UTC #20 lore raw

2016-07-26 21:04 GMT+03:00 Steve French <[email protected]>:

Quoted 2 lines

thoughts on stable?

Yes, I think it should go to stable as well.

Signature · 2 lines

Best regards,
Pavel Shilovsky

A message that is not in the archive (sent off-list or not yet received): CAKywueR7K5OR7+NnzEtqpWGR0gApoR3X0Y6C6ACzTf1y7JOcsA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Steve French

Applied

2016-07-28 05:02 UTC #22 lore raw

Pushed to cifs-2.6.git

Quoted 9 lines · Pavel Shilovsky

2016-07-26 21:04 GMT+03:00 Steve French <[email protected]>:
Quoted 2 lines
thoughts on stable?
Yes, I think it should go to stable as well.
--
Best regards,
Pavel Shilovsky

Signature · 3 lines

Thanks,

Steve

A message that is not in the archive (sent off-list or not yet received): CAH2r5mtiZNDyeRe_rYy4Pcg1WhbGaZtdweM=p8fG1uc0xZcAeg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Aurélien Aptel

author

2016-07-28 08:28 UTC #24 lore raw

Quoted 1 line · On Thu, 28 Jul 2016 00:02:47 -0500 Steve French…

Pushed to cifs-2.6.git

Yay, thanks Steve and Pavel!

Signature · 5 lines

Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Linux GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
GF: Felix Imendörffer, Jane Smithard, Graham Norton, HRB 21284 (AG
Nürnberg)

Sachin Prabhu

2016-07-29 13:11 UTC #25 lore raw

In reply to Aurélien Aptel

Quoted 23 lines · Aurélien Aptel

Quoted 3 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 7 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows 7
PC with the restricted user account. (Even in hard mode.) I can mount the share from Linux-cifs using the admin user but not the
restricted user.
I've moved some things around. All of the prefix path components are now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary
which should take care of the last case (hard mode).

Quoted 7 lines

Please test my latest patch (attached).
Quoted 3 lines
(I noticed though that no user has access to the file in the shared dir. But this doesn't really matter for the test.)
Indeed.

Hello,

Sorry for the late reply but this has to be a NACK from me.

We need to check for CIFS_MOUNT_USE_PREFIX_PATH and if set, check cifs_sb->prepath for both old and new in cifs_match_super().

Else we have the following bug:

Consider 2 different mounts on a server where root access is limited. I used the reproducer for this case but simply created a separate folder in the root directory to which the user has access. I then attempt to mount the 2 separate folders in 2 different locations.

# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub/dir /mnt
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub2/ /mnt2

# grep mnt /proc/mounts
//vm140-52/test2/sub/dir /mnt cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mode=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=60
,actimeo=1 0 0
//vm140-52/test2/sub2/ /mnt2 cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mode=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=60
,actimeo=1 0 0

but since we do not compare the prepath, we end up with the same share mounted at both mount points. This is the share mounted first.

To confirm.

# date >/mnt/test
# cat /mnt/test /mnt2/test
Fri 29 Jul 14:05:19 BST 2016
Fri 29 Jul 14:05:19 BST 2016

Steve,

Can you recall the earlier patch or should I write a fix for this?

Sachin Prabhu

A message that is not in the archive (sent off-list or not yet received): 1469797864.14723.15.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Sachin Prabhu

2016-07-29 13:31 UTC #27 lore raw

Quoted 91 lines · Sachin Prabhu

Quoted 29 lines · Aurélien Aptel
Quoted 4 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 10 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows
7
PC with the restricted user account. (Even in hard mode.) I can mount the share from Linux-cifs using the admin user but not
the
restricted user.
I've moved some things around. All of the prefix path components are
now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary
which should take care of the last case (hard mode).
Quoted 9 lines
Please test my latest patch (attached).
Quoted 5 lines
(I noticed though that no user has access to the file in the shared
dir. But this doesn't really matter for the test.)
Indeed.
Hello,
Sorry for the late reply but this has to be a NACK from me.
We need to check for CIFS_MOUNT_USE_PREFIX_PATH and if set, check cifs_sb->prepath for both old and new in cifs_match_super().
Else we have the following bug:
Consider 2 different mounts on a server where root access is limited. I
used the reproducer for this case but simply created a separate folder
in the root directory to which the user has access. I then attempt to mount the 2 separate folders in 2 different locations.
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub/dir /mnt
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub2/ /mnt2
# grep mnt /proc/mounts
//vm140-52/test2/sub/dir /mnt cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
e=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
60
,actimeo=1 0 0
//vm140-52/test2/sub2/ /mnt2 cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
e=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
60
,actimeo=1 0 0
but since we do not compare the prepath, we end up with the same share
mounted at both mount points. This is the share mounted first.
To confirm.
# date >/mnt/test
# cat /mnt/test /mnt2/test
Fri 29 Jul 14:05:19 BST 2016
Fri 29 Jul 14:05:19 BST 2016

Quoted 5 lines

Steve,
Can you recall the earlier patch or should I write a fix for this?
Sachin Prabhu

This bug in the patch was masked by another issue which was fixed by the patch

cifs: unbreak TCP session reuse
by Rabin Vincent which has been posted to go into upstream at the same time as this patch.

Sachin Prabhu

A message that is not in the archive (sent off-list or not yet received): 1469799107.14723.18.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Steve French

2016-07-29 20:20 UTC #29 lore raw

Let's add your fix as a followon patch

Quoted 106 lines · Sachin Prabhu

Quoted 91 lines · Sachin Prabhu
Quoted 29 lines · Aurélien Aptel
Quoted 4 lines · On Thu, 9 Jun 2016 21:27:34 +0200 Marcus Hoffmann…
Hey Aurélien,
with your script I can reproduce the bug locally now.
Good.
Quoted 10 lines
I can mount the share (which is on a Windows 8.1 vm) with a Windows
7
PC with the restricted user account. (Even in hard mode.) I can mount the share from Linux-cifs using the admin user but not
the
restricted user.
I've moved some things around. All of the prefix path components are
now checked for accessibility in cifs_do_mount(). This is more robust and it lets us set the CIFS_MOUNT_USE_PREFIX_PATH flag earlier.
I've updated the cifs_root_iget() to use the prefix path when necessary
which should take care of the last case (hard mode).
Quoted 9 lines
Please test my latest patch (attached).
Quoted 5 lines
(I noticed though that no user has access to the file in the shared
dir. But this doesn't really matter for the test.)
Indeed.
Hello,
Sorry for the late reply but this has to be a NACK from me.
We need to check for CIFS_MOUNT_USE_PREFIX_PATH and if set, check cifs_sb->prepath for both old and new in cifs_match_super().
Else we have the following bug:
Consider 2 different mounts on a server where root access is limited. I
used the reproducer for this case but simply created a separate folder
in the root directory to which the user has access. I then attempt to mount the 2 separate folders in 2 different locations.
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub/dir /mnt
# mount -t cifs -vvv -o username=wintest1,password=xxx //vm140-
52/test2/sub2/ /mnt2
# grep mnt /proc/mounts
//vm140-52/test2/sub/dir /mnt cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
e=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
60
,actimeo=1 0 0
//vm140-52/test2/sub2/ /mnt2 cifs
rw,relatime,vers=1.0,cache=strict,username=wintest1,domain=ENG1,uid=0
,n
oforceuid,gid=0,noforcegid,addr=192.168.140.52,file_mode=0755,dir_mod
e=
0755,nounix,serverino,mapposix,rsize=61440,wsize=16580,echo_interval=
60
,actimeo=1 0 0
but since we do not compare the prepath, we end up with the same share
mounted at both mount points. This is the share mounted first.
To confirm.
# date >/mnt/test
# cat /mnt/test /mnt2/test
Fri 29 Jul 14:05:19 BST 2016
Fri 29 Jul 14:05:19 BST 2016
Quoted 5 lines
Steve,
Can you recall the earlier patch or should I write a fix for this?
Sachin Prabhu
This bug in the patch was masked by another issue which was fixed by the patch
cifs: unbreak TCP session reuse
by Rabin Vincent which has been posted to go into upstream at the same time as this patch.
Sachin Prabhu

Signature · 3 lines

Thanks,

Steve