From 2477bc58d49edb1c0baf59df7dc093dce682af2b Mon Sep 17 00:00:00 2001 From: Sachin Prabhu Date: Wed, 4 Feb 2015 13:10:26 +0000 Subject: cifs: smb2_clone_range() - exit on unhandled error While attempting to clone a file on a samba server, we receive a STATUS_INVALID_DEVICE_REQUEST. This is mapped to -EOPNOTSUPP which isn't handled in smb2_clone_range(). We end up looping in the while loop making same call to the samba server over and over again. The proposed fix is to exit and return the error value when encountered with an unhandled error. Cc: Signed-off-by: Sachin Prabhu Signed-off-by: Steve French Signed-off-by: Steve French --- fs/cifs/smb2ops.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 96b5d40a2ece..eab05e1aa587 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -684,7 +684,8 @@ smb2_clone_range(const unsigned int xid, /* No need to change MaxChunks since already set to 1 */ chunk_sizes_updated = true; - } + } else + goto cchunk_out; } cchunk_out: -- cgit v1.2.3 From e1e9bda22d7ddf88515e8fe401887e313922823e Mon Sep 17 00:00:00 2001 From: David Disseldorp Date: Fri, 13 Mar 2015 14:20:29 +0100 Subject: cifs: fix use-after-free bug in find_writable_file Under intermittent network outages, find_writable_file() is susceptible to the following race condition, which results in a user-after-free in the cifs_writepages code-path: Thread 1 Thread 2 ======== ======== inv_file = NULL refind = 0 spin_lock(&cifs_file_list_lock) // invalidHandle found on openFileList inv_file = open_file // inv_file->count currently 1 cifsFileInfo_get(inv_file) // inv_file->count = 2 spin_unlock(&cifs_file_list_lock); cifs_reopen_file() cifs_close() // fails (rc != 0) ->cifsFileInfo_put() spin_lock(&cifs_file_list_lock) // inv_file->count = 1 spin_unlock(&cifs_file_list_lock) spin_lock(&cifs_file_list_lock); list_move_tail(&inv_file->flist, &cifs_inode->openFileList); spin_unlock(&cifs_file_list_lock); cifsFileInfo_put(inv_file); ->spin_lock(&cifs_file_list_lock) // inv_file->count = 0 list_del(&cifs_file->flist); // cleanup!! kfree(cifs_file); spin_unlock(&cifs_file_list_lock); spin_lock(&cifs_file_list_lock); ++refind; // refind = 1 goto refind_writable; At this point we loop back through with an invalid inv_file pointer and a refind value of 1. On second pass, inv_file is not overwritten on openFileList traversal, and is subsequently dereferenced. Signed-off-by: David Disseldorp Reviewed-by: Jeff Layton CC: Signed-off-by: Steve French --- fs/cifs/file.c | 1 + 1 file changed, 1 insertion(+) (limited to 'fs') diff --git a/fs/cifs/file.c b/fs/cifs/file.c index a94b3e673182..ca30c391a894 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -1823,6 +1823,7 @@ refind_writable: cifsFileInfo_put(inv_file); spin_lock(&cifs_file_list_lock); ++refind; + inv_file = NULL; goto refind_writable; } } -- cgit v1.2.3 From 2bd50fb3d4d31f5168ecea221f291534cd0a96e9 Mon Sep 17 00:00:00 2001 From: Taesoo Kim Date: Sat, 21 Mar 2015 19:08:30 -0400 Subject: cifs: potential memory leaks when parsing mnt opts For example, when mount opt is redundently specified (e.g., "user=A,user=B,user=C"), kernel kept allocating new key/val with kstrdup() and overwrite previous ptr (to be freed). Althouhg mount.cifs in userspace performs a bit of sanitization (e.g., forcing one user option), current implementation is not robust. Other options such as iocharset and domainanme are similarly vulnerable. Signed-off-by: Taesoo Kim Signed-off-by: Steve French --- fs/cifs/connect.c | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'fs') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index d3aa999ab785..4cb8450e081b 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -1599,6 +1599,8 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, pr_warn("CIFS: username too long\n"); goto cifs_parse_mount_err; } + + kfree(vol->username); vol->username = kstrdup(string, GFP_KERNEL); if (!vol->username) goto cifs_parse_mount_err; @@ -1700,6 +1702,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, goto cifs_parse_mount_err; } + kfree(vol->domainname); vol->domainname = kstrdup(string, GFP_KERNEL); if (!vol->domainname) { pr_warn("CIFS: no memory for domainname\n"); @@ -1731,6 +1734,7 @@ cifs_parse_mount_options(const char *mountdata, const char *devname, } if (strncasecmp(string, "default", 7) != 0) { + kfree(vol->iocharset); vol->iocharset = kstrdup(string, GFP_KERNEL); if (!vol->iocharset) { -- cgit v1.2.3 From 75fdfc849ae0636853add4b7dbdc5753160ad0cb Mon Sep 17 00:00:00 2001 From: Steve French Date: Wed, 25 Mar 2015 18:51:57 -0500 Subject: Fix warning on uninitialized buftype Pointed out by coverity analyzer. resp_buftype is not initialized in one path which can rarely log a spurious warning (buf is null so there will not be a problem with freeing data, but if buf_type were randomly set to wrong value could log a warning) Reported by Coverity (CID 1269144) Signed-off-by: Steve French Acked-by: Shirish Pargaonkar Acked-by: Sachin Prabhu Reviewed-by: Jeff Layton --- fs/cifs/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 3417340bf89e..1b906debe11d 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2114,7 +2114,7 @@ SMB2_query_directory(const unsigned int xid, struct cifs_tcon *tcon, struct kvec iov[2]; int rc = 0; int len; - int resp_buftype; + int resp_buftype = CIFS_NO_BUFFER; unsigned char *bufptr; struct TCP_Server_Info *server; struct cifs_ses *ses = tcon->ses; -- cgit v1.2.3 From f3a31a2bbbfe22db0313a4e270c7ba91baa7b630 Mon Sep 17 00:00:00 2001 From: Steve French Date: Thu, 26 Mar 2015 19:23:20 -0500 Subject: Don't ignore errors on encrypting password in SMBTcon Although unlikely to fail (and tree connect does not commonly send a password since SECMODE_USER is the default for most servers) do not ignore errors on SMBNTEncrypt in SMB Tree Connect. Reported by Coverity (CID 1226853) Signed-off-by: Steve French Acked-by: Shirish Pargaonkar Acked-by: Sachin Prabhu Reviewed-by: Jeff Layton --- fs/cifs/connect.c | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'fs') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 4cb8450e081b..cdb1aaf295b1 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3696,6 +3696,12 @@ CIFSTCon(const unsigned int xid, struct cifs_ses *ses, #endif /* CIFS_WEAK_PW_HASH */ rc = SMBNTencrypt(tcon->password, ses->server->cryptkey, bcc_ptr, nls_codepage); + if (rc) { + cifs_dbg(FYI, "%s Can't generate NTLM rsp. Error: %d\n", + __func__, rc); + cifs_buf_release(smb_buffer); + return rc; + } bcc_ptr += CIFS_AUTH_RESP_SIZE; if (ses->capabilities & CAP_UNICODE) { -- cgit v1.2.3 From 8e3531060556bf5e09444acd2aa5c78bce9c9f70 Mon Sep 17 00:00:00 2001 From: Steve French Date: Thu, 26 Mar 2015 19:47:02 -0500 Subject: Fix dereference before null check warning null tcon is not likely in these paths in current code, but obviously it does clarify the code to check for null (if at all) before derefrencing rather than after. Reported by Coverity (CID 1042666) Signed-off-by: Steve French Acked-by: Shirish Pargaonkar Acked-by: Sachin Prabhu --- fs/cifs/smb2pdu.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'fs') diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 1b906debe11d..78b329f07b3b 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1218,7 +1218,7 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, struct smb2_ioctl_req *req; struct smb2_ioctl_rsp *rsp; struct TCP_Server_Info *server; - struct cifs_ses *ses = tcon->ses; + struct cifs_ses *ses; struct kvec iov[2]; int resp_buftype; int num_iovecs; @@ -1233,6 +1233,11 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, if (plen) *plen = 0; + if (tcon) + ses = tcon->ses; + else + return -EIO; + if (ses && (ses->server)) server = ses->server; else @@ -1296,14 +1301,12 @@ SMB2_ioctl(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, rsp = (struct smb2_ioctl_rsp *)iov[0].iov_base; if ((rc != 0) && (rc != -EINVAL)) { - if (tcon) - cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); + cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); goto ioctl_exit; } else if (rc == -EINVAL) { if ((opcode != FSCTL_SRV_COPYCHUNK_WRITE) && (opcode != FSCTL_SRV_COPYCHUNK)) { - if (tcon) - cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); + cifs_stats_fail_inc(tcon, SMB2_IOCTL_HE); goto ioctl_exit; } } -- cgit v1.2.3 From 064bcc0702a83aed29220bee3b3db3f968c5d8ae Mon Sep 17 00:00:00 2001 From: Steve French Date: Thu, 26 Mar 2015 22:41:59 -0500 Subject: Fix coverity warning Coverity reports a warning for referencing the beginning of the SMB2/SMB3 frame using the ProtocolId field as an array. Although it works the same either way, this patch should quiet the warning and might be a little clearer. Reported by Coverity (CID 741269) Signed-off-by: Steve French Acked-by: Shirish Pargaonkar Acked-by: Sachin Prabhu Reviewed-by: Jeff Layton --- fs/cifs/smb2misc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c index 689f035915cf..22dfdf17d065 100644 --- a/fs/cifs/smb2misc.c +++ b/fs/cifs/smb2misc.c @@ -322,7 +322,7 @@ smb2_get_data_area_len(int *off, int *len, struct smb2_hdr *hdr) /* return pointer to beginning of data area, ie offset from SMB start */ if ((*off != 0) && (*len != 0)) - return hdr->ProtocolId + *off; + return (char *)(&hdr->ProtocolId[0]) + *off; else return NULL; } -- cgit v1.2.3 From c85c35f8fc1d3f5ca60cd713f10af387de73cb94 Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 27 Mar 2015 01:15:02 -0500 Subject: Fix warning on impossible comparison workstation_RFC1001_name is part of the struct and can't be null, remove impossible comparison (array vs. null) Pointed out by Coverity (CID 140095) Signed-off-by: Steve French Reviewed-by: Jeff Layton --- fs/cifs/connect.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'fs') diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index cdb1aaf295b1..480cf9c81d50 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2917,8 +2917,7 @@ ip_rfc1001_connect(struct TCP_Server_Info *server) * calling name ends in null (byte 16) from old smb * convention. */ - if (server->workstation_RFC1001_name && - server->workstation_RFC1001_name[0] != 0) + if (server->workstation_RFC1001_name[0] != 0) rfc1002mangle(ses_init_buf->trailer. session_req.calling_name, server->workstation_RFC1001_name, -- cgit v1.2.3 From 8b7a454443bdb73df4fd200af7f013f3319a3855 Mon Sep 17 00:00:00 2001 From: Steve French Date: Mon, 30 Mar 2015 16:58:17 -0500 Subject: CIFS: session servername can't be null remove impossible check Pointed out by Coverity (CID 115422) Signed-off-by: Steve French Reviewed-by: Jeff Layton --- fs/cifs/cifsencrypt.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/cifs/cifsencrypt.c b/fs/cifs/cifsencrypt.c index 4ac7445e6ec7..aa0dc2573374 100644 --- a/fs/cifs/cifsencrypt.c +++ b/fs/cifs/cifsencrypt.c @@ -1,6 +1,9 @@ /* * fs/cifs/cifsencrypt.c * + * Encryption and hashing operations relating to NTLM, NTLMv2. See MS-NLMP + * for more detailed information + * * Copyright (C) International Business Machines Corp., 2005,2013 * Author(s): Steve French (sfrench@us.ibm.com) * @@ -515,7 +518,8 @@ static int calc_ntlmv2_hash(struct cifs_ses *ses, char *ntlmv2_hash, __func__); return rc; } - } else if (ses->serverName) { + } else { + /* We use ses->serverName if no domain name available */ len = strlen(ses->serverName); server = kmalloc(2 + (len * 2), GFP_KERNEL); -- cgit v1.2.3 From dfebe40076f3b4966e818eaf94021a7ff788df04 Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 27 Mar 2015 01:00:06 -0500 Subject: Fix another dereference before null check warning null tcon is not possible in these paths so remove confusing null check Reported by Coverity (CID 728519) Signed-off-by: Steve French Reviewed-by: Jeff Layton --- fs/cifs/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs') diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 78b329f07b3b..65cd7a84c8bc 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1632,7 +1632,7 @@ SMB2_flush(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, rc = SendReceive2(xid, ses, iov, 1, &resp_buftype, 0); - if ((rc != 0) && tcon) + if (rc != 0) cifs_stats_fail_inc(tcon, SMB2_FLUSH_HE); free_rsp_buf(resp_buftype, iov[0].iov_base); -- cgit v1.2.3 From 4c5930e8056127e9a89bb7836bfa34318a89ab14 Mon Sep 17 00:00:00 2001 From: Steve French Date: Mon, 30 Mar 2015 22:03:06 -0500 Subject: Fix warning Coverity reports a warning due to unitialized attr structure in one code path. Reported by Coverity (CID 728535) Signed-off-by: Steve French Reviewed-by: Jeff Layton --- fs/cifs/inode.c | 2 ++ 1 file changed, 2 insertions(+) (limited to 'fs') diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 2d4f37235ed0..3e126d7bb2ea 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -771,6 +771,8 @@ cifs_get_inode_info(struct inode **inode, const char *full_path, cifs_buf_release(srchinf->ntwrk_buf_start); } kfree(srchinf); + if (rc) + goto cgii_exit; } else goto cgii_exit; -- cgit v1.2.3