cifs: dereferencing first then checking

PATCH In review Dan Carpenter 2 participants 5 messages Oct 27, 2010 – Nov 3, 2010 2 messages not in the archive

Subsystems (3)

Kernel janitors SMB client (CIFS) Kernel SMB3 server (ksmbd)

lore

Share & tools

lore

Dan Carpenter

author

2010-10-27 21:22 UTC #0 lore raw

PATCH 2/2 cifs: dereferencing first then checking

Smatch complained about a couple checking for NULL after dereferencing bugs. I'm not super familiar with the code so I did the conservative thing and move the dereferences after the checks.

The dereferences in cifs_lock() and cifs_fsync() were added in ba00ba64cf0 "cifs: make various routines use the cifsFileInfo->tcon pointer". The dereference in find_writable_file() was added in 6508d904e6f "cifs: have find_readable/writable_file filter by fsuid". The comments there say it's possible to trigger the NULL dereference under stress.

Signed-off-by Dan Carpenter <[email protected]>

fs/cifs/file.c +22 ~~-10~~

index 06a006b..33a19b4 100644
@@ -775,13 +775,13 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 		cFYI(1, "Unknown type of lock");
 
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
-	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 
 	if (file->private_data = NULL) {
 		rc = -EBADF;
 		FreeXid(xid);
 		return rc;
 	}
+	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&

@@ -1179,7 +1179,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 					bool fsuid_only)
 {
 	struct cifsFileInfo *open_file;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+	struct cifs_sb_info *cifs_sb;
 	bool any_available = false;
 	int rc;

@@ -1192,6 +1192,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 		dump_stack();
 		return NULL;
 	}
+	cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
 
 	/* only filter by fsuid on multiuser mounts */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))

@@ -1628,15 +1629,26 @@ int cifs_fsync(struct file *file, int datasync)
 		file->f_path.dentry->d_name.name, datasync);
 
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc = 0) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!rc && tcon && smbfile &&
-		   !(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	if (rc)
+		goto err_out;
+
+	rc = CIFS_I(inode)->write_behind_rc;
+	CIFS_I(inode)->write_behind_rc = 0;
+	if (rc)
+		goto err_out;
+
+	if (CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)
+		goto done;
+	if (!smbfile)
+		goto done;
+	tcon = tlink_tcon(smbfile->tlink);
+	if (!tcon)
+		goto done;
+
+	rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
+err_out:
+done:
 	FreeXid(xid);
 	return rc;
 }

Jeff Layton

2010-10-27 23:51 UTC #1 lore raw

Quoted 18 lines · On Wed, 27 Oct 2010 23:22:53 +0200 Dan Carpenter <[email protected]> wrote:

Smatch complained about a couple checking for NULL after dereferencing bugs. I'm not super familiar with the code so I did the conservative thing and move the dereferences after the checks.
The dereferences in cifs_lock() and cifs_fsync() were added in ba00ba64cf0 "cifs: make various routines use the cifsFileInfo->tcon pointer". The dereference in find_writable_file() was added in 6508d904e6f "cifs: have find_readable/writable_file filter by fsuid". The comments there say it's possible to trigger the NULL dereference under stress.
Signed-off-by Dan Carpenter <[email protected]>
fs/cifs/file.c +0 -0
index 06a006b..33a19b4 100644

fs/cifs/file.c int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock) · line 783

Quoted 10 lines

fs/cifs/file.c +0 -1

@@ -775,13 +775,13 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 		cFYI(1, "Unknown type of lock");
 
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
-	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 
 	if (file->private_data = NULL) {
 		rc = -EBADF;
 		FreeXid(xid);
 		return rc;
 	}

	^^^^^^^

I think the above check can just go away now. If this pointer is NULL then something is badly wrong.

Quoted 14 lines

fs/cifs/file.c +2 -1

+	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&

@@ -1179,7 +1179,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 					bool fsuid_only)
 {
 	struct cifsFileInfo *open_file;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+	struct cifs_sb_info *cifs_sb;
 	bool any_available = false;
 	int rc;

fs/cifs/file.c struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode, · line 1194

Quoted 3 lines

fs/cifs/file.c +0 -0

@@ -1192,6 +1192,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 		dump_stack();
 		return NULL;
 	}

^^^^^^
I think we ought to rid ourselves of the above check too. It's not clear how we'd get a NULL pointer there anyway -- mostly we get cifsInodeInfo pointers from the CIFS_I(inode) macro which does a container_of() on the inode. That should never return a NULL pointer anyway, unless the inode pointer happens to be corrupt in a very specific way.

Quoted 29 lines

fs/cifs/file.c +14 -8

+	cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
 
 	/* only filter by fsuid on multiuser mounts */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))

@@ -1628,15 +1629,26 @@ int cifs_fsync(struct file *file, int datasync)
 		file->f_path.dentry->d_name.name, datasync);
 
 	rc = filemap_write_and_wait(inode->i_mapping);
-	if (rc = 0) {
-		rc = CIFS_I(inode)->write_behind_rc;
-		CIFS_I(inode)->write_behind_rc = 0;
-		tcon = tlink_tcon(smbfile->tlink);
-		if (!rc && tcon && smbfile &&
-		   !(CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC))
-			rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
-	}
+	if (rc)
+		goto err_out;
+
+	rc = CIFS_I(inode)->write_behind_rc;
+	CIFS_I(inode)->write_behind_rc = 0;
+	if (rc)
+		goto err_out;
+
+	if (CIFS_SB(inode->i_sb)->mnt_cifs_flags & CIFS_MOUNT_NOSSYNC)
+		goto done;
+	if (!smbfile)
+		goto done;
+	tcon = tlink_tcon(smbfile->tlink);

fs/cifs/file.c int cifs_fsync(struct file *file, int datasync) · line 1654

Quoted 10 lines

fs/cifs/file.c +6 -0

+	if (!tcon)
+		goto done;
+
+	rc = CIFSSMBFlush(xid, tcon, smbfile->netfid);
 
+err_out:
+done:
 	FreeXid(xid);
 	return rc;
 }

The above delta shouldn't be needed once the patches in Steve's tree are pushed to Linus. I think what we should probably do is wait for Steve to push to Linus and then do a patch to remove the first two checks.

Sound like a plan?

Signature · 1 line

Jeff Layton <[email protected]>

A message that is not in the archive (sent off-list or not yet received): 20101027195123.0a712a53-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org

Dan Carpenter

author

2010-10-28 03:44 UTC #3 lore raw

Quoted 8 lines · Jeff Layton

The above delta shouldn't be needed once the patches in Steve's tree are pushed to Linus. I think what we should probably do is wait for Steve to push to Linus and then do a patch to remove the first two checks.
Sound like a plan?

Sounds like a plan.

regards,
dan carpenter

Quoted 2 lines

Signature · 1 line
Jeff Layton <[email protected]>

Jeff Layton

2010-11-02 20:22 UTC #4 lore raw

Quoted 7 lines · On Thu, 28 Oct 2010 05:44:34 +0200 Dan Carpenter <[email protected]> wrote:

Quoted 6 lines · Jeff Layton
The above delta shouldn't be needed once the patches in Steve's tree are pushed to Linus. I think what we should probably do is wait for Steve to push to Linus and then do a patch to remove the first two checks.

Quoted 8 lines

Quoted 2 lines
Sound like a plan?
Sounds like a plan.
regards,
dan carpenter

Here's a respun patch that should fix these problems. I chickened out on removing the check in find_writable_file.

It still seems like a bogus cargo-cult sort of thing. It should probably be a BUG() if you call that function with a NULL pointer, but I don't feel like tackling that just yet.

Dan, does this look ok to you? If so, I'll resend to Steve as an "official" patch.

---------------------[snip]-----------------------

cifs: dereferencing first then checking (try #2)

This patch is based on Dan's original patch. His original description is below:

Smatch complained about a couple checking for NULL after dereferencing bugs. I'm not super familiar with the code so I did the conservative thing and move the dereferences after the checks.

Signed-off-by Dan Carpenter <[email protected]>

Signed-off-by Jeff Layton <[email protected]>

---
 fs/cifs/file.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

fs/cifs/file.c +3 -7

index a566f15..a59b53f 100644
@@ -755,12 +755,6 @@ int cifs_lock(struct file *file, int cmd, struct file_lock *pfLock)
 
 	cifs_sb = CIFS_SB(file->f_path.dentry->d_sb);
 	tcon = tlink_tcon(((struct cifsFileInfo *)file->private_data)->tlink);
-
-	if (file->private_data = NULL) {
-		rc = -EBADF;
-		FreeXid(xid);
-		return rc;
-	}
 	netfid = ((struct cifsFileInfo *)file->private_data)->netfid;
 
 	if ((tcon->ses->capabilities & CAP_UNIX) &&

@@ -1155,7 +1149,7 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 					bool fsuid_only)
 {
 	struct cifsFileInfo *open_file;
-	struct cifs_sb_info *cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+	struct cifs_sb_info *cifs_sb;
 	bool any_available = false;
 	int rc;

@@ -1169,6 +1163,8 @@ struct cifsFileInfo *find_writable_file(struct cifsInodeInfo *cifs_inode,
 		return NULL;
 	}
 
+	cifs_sb = CIFS_SB(cifs_inode->vfs_inode.i_sb);
+
 	/* only filter by fsuid on multiuser mounts */
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MULTIUSER))
 		fsuid_only = false;

Signature · 1 line

1.7.2.3

A message that is not in the archive (sent off-list or not yet received): 20101102162250.3a2aa670-9yPaYZwiELC+kQycOl6kW4xkIHaj4LzF@public.gmane.org

Dan Carpenter

author

2010-11-03 16:40 UTC #6 lore raw

Quoted 10 lines · Jeff Layton

Here's a respun patch that should fix these problems. I chickened out on removing the check in find_writable_file.
It still seems like a bogus cargo-cult sort of thing. It should probably be a BUG() if you call that function with a NULL pointer, but I don't feel like tackling that just yet.
Dan, does this look ok to you? If so, I'll resend to Steve as an "official" patch.

Looks fine to me. Obviously, I've already said that I don't know if the checks are needed or not. I defer to your greater knowledge. ;)

regards,
dan carpenter