Skip to contentSkip to conversation

cifs: dir: fix memory leak in cifs_mknod

PATCH In review Gustavo A. R. Silva 3 participants 4 messages Apr 20, 2018

Subsystems (3)
Kernel janitors SMB client (CIFS) Kernel SMB3 server (ksmbd)
lore
Share & tools
lore

Gustavo A. R. Silva

author
#0 lore raw

PATCH 1/1 cifs: dir: fix memory leak in cifs_mknod

Free allocated memory for full_path and xid before return.

Addresses-Coverity-ID: 1468029 ("Resource leak")

Fixes 49162bfde140 ("cifs: do not allow creating sockets except with

SMB1 posix exensions")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 fs/cifs/dir.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/cifs/dir.c +4 -1
index f0a759d..71e32d9 100644
@@ -684,8 +684,11 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
 		goto mknod_out;
 	}
 
-	if (!S_ISCHR(mode) && !S_ISBLK(mode))
+	if (!S_ISCHR(mode) && !S_ISBLK(mode)) {
+		kfree(full_path);
+		free_xid(xid);
 		return -EPERM;
+	}
 
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
 		goto mknod_out;
Signature · 1 line
2.7.4
#1 lore raw

I noticed a similar problem with the tcon link leak on that (which Colin and Gustavo pointed out - thank you!) but also in another return statement, so updated the original patch of Ronnie's merging the fixes

https://git.samba.org/sfrench/cifs-2.6.git/?p=sfrench/cifs-2.6.git;a=commit;h=167bc5de08dc97695f9d5c7069c3e69f409ff80b

Let me know if you see any problems with it.

Quoted 34 lines · Gustavo A. R. Silva

Free allocated memory for full_path and xid before return.

Addresses-Coverity-ID: 1468029 ("Resource leak")

Fixes 49162bfde140 ("cifs: do not allow creating sockets except with

SMB1 posix exensions")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
 fs/cifs/dir.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
fs/cifs/dir.c +4 -2
index f0a759d..71e32d9 100644
@@ -684,8 +684,11 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
                goto mknod_out;
        }
 
-       if (!S_ISCHR(mode) && !S_ISBLK(mode))
+       if (!S_ISCHR(mode) && !S_ISBLK(mode)) {
+               kfree(full_path);
+               free_xid(xid);
                return -EPERM;
+       }
 
        if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
                goto mknod_out;
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 3 lines
Thanks,

Steve

From 167bc5de08dc97695f9d5c7069c3e69f409ff80b Mon Sep 17 00:00:00 2001
From: Steve French <[email protected]>
Date: Fri, 20 Apr 2018 12:19:07 -0500
Subject: [PATCH 1/2] cifs: do not allow creating sockets except with SMB1
posix exensions

RHBZ: 1453123

Since at least the 3.10 kernel and likely a lot earlier we have not been able to create unix domain sockets in a cifs share when mounted using the SFU mount option (except when mounted with the cifs unix extensions to Samba e.g.)
Trying to create a socket, for example using the af_unix command from xfstests will cause :

BUG: unable to handle kernel NULL pointer dereference at 00000000

00000040

Since no one uses or depends on being able to create unix domains sockets on a cifs share the easiest fix to stop this vulnerability is to simply not allow creation of any other special files than char or block devices when sfu is used.

Added update to Ronnie's patch to handle a tcon link leak, and to address a buf leak noticed by Gustavo and Colin.

CC Gustavo A. R. Silva <[email protected]>
CC Colin Ian King <[email protected]>
Reported-by Eryu Guan <[email protected]>
Signed-off-by Ronnie Sahlberg <[email protected]>
Signed-off-by Steve French <[email protected]>
Cc [email protected]
---
 fs/cifs/dir.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
fs/cifs/dir.c +5 -4
index 81ba6e0d88d8..925844343038 100644
@@ -684,6 +684,9 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
 		goto mknod_out;
 	}
 
+	if (!S_ISCHR(mode) && !S_ISBLK(mode))
+		goto mknod_out;
+
 	if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
 		goto mknod_out;
 

@@ -692,10 +695,8 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
 
 	buf = kmalloc(sizeof(FILE_ALL_INFO), GFP_KERNEL);
 	if (buf == NULL) {
-		kfree(full_path);
 		rc = -ENOMEM;
-		free_xid(xid);
-		return rc;
+		goto mknod_out;
 	}
 
 	if (backup_cred(cifs_sb))

@@ -742,7 +743,7 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
 		pdev->minor = cpu_to_le64(MINOR(device_number));
 		rc = tcon->ses->server->ops->sync_write(xid, &fid, &io_parms,
 							&bytes_written, iov, 1);
-	} /* else if (S_ISFIFO) */
+	}
 	tcon->ses->server->ops->close(xid, tcon, &fid);
 	d_drop(direntry);
Signature · 1 line
2.14.1
author
#2 lore raw

Hi Steve,

Quoted 9 lines · Steve French

I noticed a similar problem with the tcon link leak on that (which Colin and Gustavo pointed out - thank you!) but also in another return statement, so updated the original patch of Ronnie's merging the fixes

https://git.samba.org/sfrench/cifs-2.6.git/?p=sfrench/cifs-2.6.git;a=commit;h7bc5de08dc97695f9d5c7069c3e69f409ff80b

Let me know if you see any problems with it.

Looks correct to me.

Thanks
--
Gustavo

Quoted 39 lines
Quoted 34 lines · Gustavo A. R. Silva

Free allocated memory for full_path and xid before return.

Addresses-Coverity-ID: 1468029 ("Resource leak")

Fixes 49162bfde140 ("cifs: do not allow creating sockets except with

SMB1 posix exensions")

Signed-off-by Gustavo A. R. Silva <[email protected]>
---
  fs/cifs/dir.c | 5 ++++-
  1 file changed, 4 insertions(+), 1 deletion(-)
fs/cifs/dir.c +4 -2
index f0a759d..71e32d9 100644
@@ -684,8 +684,11 @@ int cifs_mknod(struct inode *inode, struct dentry *direntry, umode_t mode,
                 goto mknod_out;
         }
 
-       if (!S_ISCHR(mode) && !S_ISBLK(mode))
+       if (!S_ISCHR(mode) && !S_ISBLK(mode)) {
+               kfree(full_path);
+               free_xid(xid);
                 return -EPERM;
+       }
 
         if (!(cifs_sb->mnt_cifs_flags & CIFS_MOUNT_UNX_EMUL))
                 goto mknod_out;
--

2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Reviewed-by
#3 lore raw
In reply to Steve French

2018-04-20 10:37 GMT-07:00 Steve French <[email protected]>:

Quoted 9 lines

I noticed a similar problem with the tcon link leak on that (which Colin and Gustavo pointed out - thank you!) but also in another return statement, so updated the original patch of Ronnie's merging the fixes

https://git.samba.org/sfrench/cifs-2.6.git/?p=sfrench/cifs-2.6.git;a=commit;h7bc5de08dc97695f9d5c7069c3e69f409ff80b

Let me know if you see any problems with it.

Looks good.

Reviewed-by Pavel Shilovsky <[email protected]>

--
Best regards,
Pavel Shilovsky