linux-kernel - [PATCH 20/35] cifs: Fix permission logic in cifs_rename_pending

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]

Message-Id: <20250831123602.14037-21-pali@kernel.org>
Date: Sun, 31 Aug 2025 14:35:47 +0200
From: Pali Rohár <pali@...nel.org>
To: Steve French <sfrench@...ba.org>,
	Paulo Alcantara <pc@...guebit.com>,
	ronnie sahlberg <ronniesahlberg@...il.com>
Cc: linux-cifs@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: [PATCH 20/35] cifs: Fix permission logic in cifs_rename_pending_delete()

Opening file with DELETE access does not have to be allowed by server for
file with ATTR_READONLY attribute set. So the current logic for clearing
the ATTR_READONLY attribute in cifs_rename_pending_delete() can fail as it
tries to open file with access mask DELETE | FILE_WRITE_ATTRIBUTES.

Fix the permission logic. First clear the ATTR_READONLY attribute and then
the open file with DELETE access. As for the RENAME and SET_DELETE_PENDING
operations is not required the FILE_WRITE_ATTRIBUTES access, do not pass it
into open. This allows to call cifs_rename_pending_delete() also on the
files with ACL permissions which disallows file modification.

For changing attributes use the set_file_info() callback function which
already handles the situation when the caller wants to clear the
ATTR_READONLY flag. Note that the set_file_info() callback also updates the
cifsInode->cifsAttrs member, so remove explicit assignment here.

Signed-off-by: Pali Rohár <pali@...nel.org>
---
 fs/smb/client/inode.c | 48 ++++++++++++++++++++-----------------------
 1 file changed, 22 insertions(+), 26 deletions(-)

diff --git a/fs/smb/client/inode.c b/fs/smb/client/inode.c
index 4658af632098..63ab233517f2 100644
--- a/fs/smb/client/inode.c
+++ b/fs/smb/client/inode.c
@@ -1755,20 +1755,6 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 		goto out;
 	}
 
-	oparms = (struct cifs_open_parms) {
-		.tcon = tcon,
-		.cifs_sb = cifs_sb,
-		.desired_access = DELETE | FILE_WRITE_ATTRIBUTES,
-		.create_options = cifs_create_options(cifs_sb, CREATE_NOT_DIR),
-		.disposition = FILE_OPEN,
-		.path = full_path,
-		.fid = &fid,
-	};
-
-	rc = CIFS_open(xid, &oparms, &oplock, NULL);
-	if (rc != 0)
-		goto out;
-
 	origattr = cifsInode->cifsAttrs & ~ATTR_NORMAL;
 
 	/* clear ATTR_READONLY, needed for opening file with DELETE access */
@@ -1791,16 +1777,27 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 	/* change dosattr, but only if needed */
 	if (dosattr != origattr) {
 		info_buf.Attributes = cpu_to_le32(dosattr);
-		rc = CIFSSMBSetFileInfo(xid, tcon, &info_buf, fid.netfid,
-					current->tgid);
+		rc = tcon->ses->server->ops->set_file_info(inode, full_path, &info_buf, xid);
 		/* although we would like to mark the file hidden
  		   if that fails we will still try to rename it */
-		if (!rc)
-			cifsInode->cifsAttrs = dosattr;
-		else
+		if (rc)
 			dosattr = origattr; /* since not able to change them */
 	}
 
+	oparms = (struct cifs_open_parms) {
+		.tcon = tcon,
+		.cifs_sb = cifs_sb,
+		.desired_access = DELETE,
+		.create_options = cifs_create_options(cifs_sb, CREATE_NOT_DIR),
+		.disposition = FILE_OPEN,
+		.path = full_path,
+		.fid = &fid,
+	};
+
+	rc = CIFS_open(xid, &oparms, &oplock, NULL);
+	if (rc != 0)
+		goto undo_setattr;
+
 	/* rename the file */
 	rc = CIFSSMBRenameOpenFile(xid, tcon, fid.netfid, sillyname,
 				   false /* overwrite */,
@@ -1808,7 +1805,7 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 				   cifs_remap(cifs_sb));
 	if (rc != 0) {
 		rc = -EBUSY;
-		goto undo_setattr;
+		goto undo_close;
 	}
 
 	/* try to set DELETE_PENDING */
@@ -1832,8 +1829,8 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 		set_bit(CIFS_INO_DELETE_PENDING, &cifsInode->flags);
 	}
 
-out_close:
 	CIFSSMBClose(xid, tcon, fid.netfid);
+
 out:
 	cifs_put_tlink(tlink);
 	return rc;
@@ -1847,15 +1844,14 @@ cifs_rename_pending_delete(const char *full_path, struct dentry *dentry,
 	CIFSSMBRenameOpenFile(xid, tcon, fid.netfid, dentry->d_name.name,
 				true /* overwrite */,
 				cifs_sb->local_nls, cifs_remap(cifs_sb));
+undo_close:
+	CIFSSMBClose(xid, tcon, fid.netfid);
 undo_setattr:
 	if (dosattr != origattr) {
 		info_buf.Attributes = cpu_to_le32(origattr);
-		if (!CIFSSMBSetFileInfo(xid, tcon, &info_buf, fid.netfid,
-					current->tgid))
-			cifsInode->cifsAttrs = origattr;
+		tcon->ses->server->ops->set_file_info(inode, full_path, &info_buf, xid);
 	}
-
-	goto out_close;
+	goto out;
 }
 #endif /* CONFIG_CIFS_ALLOW_INSECURE_LEGACY */
 
-- 
2.20.1