From 6f582b273ec23332074d970a7fb25bef835df71f Mon Sep 17 00:00:00 2001 From: Pavel Shilovsky Date: Wed, 27 Nov 2019 16:18:39 -0800 Subject: CIFS: Fix NULL-pointer dereference in smb2_push_mandatory_locks Currently when the client creates a cifsFileInfo structure for a newly opened file, it allocates a list of byte-range locks with a pointer to the new cfile and attaches this list to the inode's lock list. The latter happens before initializing all other fields, e.g. cfile->tlink. Thus a partially initialized cifsFileInfo structure becomes available to other threads that walk through the inode's lock list. One example of such a thread may be an oplock break worker thread that tries to push all cached byte-range locks. This causes NULL-pointer dereference in smb2_push_mandatory_locks() when accessing cfile->tlink: [598428.945633] BUG: kernel NULL pointer dereference, address: 0000000000000038 ... [598428.945749] Workqueue: cifsoplockd cifs_oplock_break [cifs] [598428.945793] RIP: 0010:smb2_push_mandatory_locks+0xd6/0x5a0 [cifs] ... [598428.945834] Call Trace: [598428.945870] ? cifs_revalidate_mapping+0x45/0x90 [cifs] [598428.945901] cifs_oplock_break+0x13d/0x450 [cifs] [598428.945909] process_one_work+0x1db/0x380 [598428.945914] worker_thread+0x4d/0x400 [598428.945921] kthread+0x104/0x140 [598428.945925] ? process_one_work+0x380/0x380 [598428.945931] ? kthread_park+0x80/0x80 [598428.945937] ret_from_fork+0x35/0x40 Fix this by reordering initialization steps of the cifsFileInfo structure: initialize all the fields first and then add the new byte-range lock list to the inode's lock list. Cc: Stable Signed-off-by: Pavel Shilovsky Reviewed-by: Aurelien Aptel Signed-off-by: Steve French --- fs/cifs/file.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/fs/cifs/file.c b/fs/cifs/file.c index f1fe9c44d298..9ae41042fa3d 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -315,9 +315,6 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, INIT_LIST_HEAD(&fdlocks->locks); fdlocks->cfile = cfile; cfile->llist = fdlocks; - cifs_down_write(&cinode->lock_sem); - list_add(&fdlocks->llist, &cinode->llist); - up_write(&cinode->lock_sem); cfile->count = 1; cfile->pid = current->tgid; @@ -342,6 +339,10 @@ cifs_new_fileinfo(struct cifs_fid *fid, struct file *file, oplock = 0; } + cifs_down_write(&cinode->lock_sem); + list_add(&fdlocks->llist, &cinode->llist); + up_write(&cinode->lock_sem); + spin_lock(&tcon->open_file_lock); if (fid->pending_open->oplock != CIFS_OPLOCK_NO_CHANGE && oplock) oplock = fid->pending_open->oplock; -- cgit v1.2.3-59-g8ed1b From 69738cfdfa7032f45d9e7462d24490e61cf163dd Mon Sep 17 00:00:00 2001 From: Deepa Dinamani Date: Fri, 29 Nov 2019 21:30:25 -0800 Subject: fs: cifs: Fix atime update check vs mtime According to the comment in the code and commit log, some apps expect atime >= mtime; but the introduced code results in atime==mtime. Fix the comparison to guard against atime Cc: stfrench@microsoft.com Cc: linux-cifs@vger.kernel.org Signed-off-by: Steve French --- fs/cifs/inode.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/inode.c b/fs/cifs/inode.c index 8a76195e8a69..ca76a9287456 100644 --- a/fs/cifs/inode.c +++ b/fs/cifs/inode.c @@ -163,7 +163,7 @@ cifs_fattr_to_inode(struct inode *inode, struct cifs_fattr *fattr) spin_lock(&inode->i_lock); /* we do not want atime to be less than mtime, it broke some apps */ - if (timespec64_compare(&fattr->cf_atime, &fattr->cf_mtime)) + if (timespec64_compare(&fattr->cf_atime, &fattr->cf_mtime) < 0) inode->i_atime = fattr->cf_mtime; else inode->i_atime = fattr->cf_atime; -- cgit v1.2.3-59-g8ed1b From a9f76cf82719aea0f41bfcae57e17ffec39743d0 Mon Sep 17 00:00:00 2001 From: Colin Ian King Date: Mon, 2 Dec 2019 18:59:42 +0000 Subject: cifs: remove redundant assignment to pointer pneg_ctxt The pointer pneg_ctxt is being initialized with a value that is never read and it is being updated later with a new value. The assignment is redundant and can be removed. Addresses-Coverity: ("Unused value") Signed-off-by: Colin Ian King Signed-off-by: Steve French --- fs/cifs/smb2pdu.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index ed77f94dbf1d..be0de8a63e57 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -554,7 +554,7 @@ static void assemble_neg_contexts(struct smb2_negotiate_req *req, struct TCP_Server_Info *server, unsigned int *total_len) { - char *pneg_ctxt = (char *)req; + char *pneg_ctxt; unsigned int ctxt_len; if (*total_len > 200) { -- cgit v1.2.3-59-g8ed1b From 9e8fae2597405ab1deac8909928eb8e99876f639 Mon Sep 17 00:00:00 2001 From: Steve French Date: Mon, 2 Dec 2019 17:55:41 -0600 Subject: smb3: remove unused flag passed into close functions close was relayered to allow passing in an async flag which is no longer needed in this path. Remove the unneeded parameter "flags" passed in on close. Signed-off-by: Steve French Reviewed-by: Pavel Shilovsky Reviewed-by: Ronnie Sahlberg --- fs/cifs/smb2pdu.c | 19 +++++-------------- fs/cifs/smb2proto.h | 2 -- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index be0de8a63e57..cec53eb8a6da 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2959,8 +2959,8 @@ SMB2_close_free(struct smb_rqst *rqst) } int -SMB2_close_flags(const unsigned int xid, struct cifs_tcon *tcon, - u64 persistent_fid, u64 volatile_fid, int flags) +SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, + u64 persistent_fid, u64 volatile_fid) { struct smb_rqst rqst; struct smb2_close_rsp *rsp = NULL; @@ -2969,6 +2969,7 @@ SMB2_close_flags(const unsigned int xid, struct cifs_tcon *tcon, struct kvec rsp_iov; int resp_buftype = CIFS_NO_BUFFER; int rc = 0; + int flags = 0; cifs_dbg(FYI, "Close\n"); @@ -3007,27 +3008,17 @@ SMB2_close_flags(const unsigned int xid, struct cifs_tcon *tcon, close_exit: SMB2_close_free(&rqst); free_rsp_buf(resp_buftype, rsp); - return rc; -} - -int -SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, - u64 persistent_fid, u64 volatile_fid) -{ - int rc; - int tmp_rc; - - rc = SMB2_close_flags(xid, tcon, persistent_fid, volatile_fid, 0); /* retry close in a worker thread if this one is interrupted */ if (rc == -EINTR) { + int tmp_rc; + tmp_rc = smb2_handle_cancelled_close(tcon, persistent_fid, volatile_fid); if (tmp_rc) cifs_dbg(VFS, "handle cancelled close fid 0x%llx returned error %d\n", persistent_fid, tmp_rc); } - return rc; } diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index d21a5fcc8d06..5caeaa99cb7c 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -157,8 +157,6 @@ extern int SMB2_change_notify(const unsigned int xid, struct cifs_tcon *tcon, extern int SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_file_id, u64 volatile_file_id); -extern int SMB2_close_flags(const unsigned int xid, struct cifs_tcon *tcon, - u64 persistent_fid, u64 volatile_fid, int flags); extern int SMB2_close_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, u64 persistent_file_id, u64 volatile_file_id); extern void SMB2_close_free(struct smb_rqst *rqst); -- cgit v1.2.3-59-g8ed1b From 43f8a6a74ee2442b9410ed297f5d4c77e7cb5ace Mon Sep 17 00:00:00 2001 From: Steve French Date: Mon, 2 Dec 2019 21:46:54 -0600 Subject: smb3: query attributes on file close Since timestamps on files on most servers can be updated at close, and since timestamps on our dentries default to one second we can have stale timestamps in some common cases (e.g. open, write, close, stat, wait one second, stat - will show different mtime for the first and second stat). The SMB2/SMB3 protocol allows querying timestamps at close so add the code to request timestamp and attr information (which is cheap for the server to provide) to be returned when a file is closed (it is not needed for the many paths that call SMB2_close that are from compounded query infos and close nor is it needed for some of the cases where a directory close immediately follows a directory open. Signed-off-by: Steve French Acked-by: Ronnie Sahlberg Reviewed-by: Aurelien Aptel Reviewed-by: Pavel Shilovsky --- fs/cifs/cifsglob.h | 3 +++ fs/cifs/file.c | 4 +++- fs/cifs/smb2inode.c | 2 +- fs/cifs/smb2ops.c | 49 +++++++++++++++++++++++++++++++++++++++++++++---- fs/cifs/smb2pdu.c | 38 ++++++++++++++++++++++++++++++-------- fs/cifs/smb2pdu.h | 11 +++++++++++ fs/cifs/smb2proto.h | 5 ++++- 7 files changed, 97 insertions(+), 15 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index d34a4ed8c57d..5b976e01dd6b 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -368,6 +368,9 @@ struct smb_version_operations { /* close a file */ void (*close)(const unsigned int, struct cifs_tcon *, struct cifs_fid *); + /* close a file, returning file attributes and timestamps */ + void (*close_getattr)(const unsigned int xid, struct cifs_tcon *tcon, + struct cifsFileInfo *pfile_info); /* send a flush request to the server */ int (*flush)(const unsigned int, struct cifs_tcon *, struct cifs_fid *); /* async read from the server */ diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 9ae41042fa3d..043288b5c728 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -496,7 +496,9 @@ void _cifsFileInfo_put(struct cifsFileInfo *cifs_file, unsigned int xid; xid = get_xid(); - if (server->ops->close) + if (server->ops->close_getattr) + server->ops->close_getattr(xid, tcon, cifs_file); + else if (server->ops->close) server->ops->close(xid, tcon, &cifs_file->fid); _free_xid(xid); } diff --git a/fs/cifs/smb2inode.c b/fs/cifs/smb2inode.c index 4121ac1163ca..18c7a33adceb 100644 --- a/fs/cifs/smb2inode.c +++ b/fs/cifs/smb2inode.c @@ -313,7 +313,7 @@ smb2_compound_op(const unsigned int xid, struct cifs_tcon *tcon, rqst[num_rqst].rq_iov = close_iov; rqst[num_rqst].rq_nvec = 1; rc = SMB2_close_init(tcon, &rqst[num_rqst], COMPOUND_FID, - COMPOUND_FID); + COMPOUND_FID, false); smb2_set_related(&rqst[num_rqst]); if (rc) goto finished; diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index a7f328f79c6f..a5c96bc522cb 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -1178,7 +1178,7 @@ smb2_set_ea(const unsigned int xid, struct cifs_tcon *tcon, memset(&close_iov, 0, sizeof(close_iov)); rqst[2].rq_iov = close_iov; rqst[2].rq_nvec = 1; - rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID); + rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID, false); smb2_set_related(&rqst[2]); rc = compound_send_recv(xid, ses, flags, 3, rqst, @@ -1332,6 +1332,45 @@ smb2_close_file(const unsigned int xid, struct cifs_tcon *tcon, SMB2_close(xid, tcon, fid->persistent_fid, fid->volatile_fid); } +static void +smb2_close_getattr(const unsigned int xid, struct cifs_tcon *tcon, + struct cifsFileInfo *cfile) +{ + struct smb2_file_network_open_info file_inf; + struct inode *inode; + int rc; + + rc = __SMB2_close(xid, tcon, cfile->fid.persistent_fid, + cfile->fid.volatile_fid, &file_inf); + if (rc) + return; + + inode = d_inode(cfile->dentry); + + spin_lock(&inode->i_lock); + CIFS_I(inode)->time = jiffies; + + /* Creation time should not need to be updated on close */ + if (file_inf.LastWriteTime) + inode->i_mtime = cifs_NTtimeToUnix(file_inf.LastWriteTime); + if (file_inf.ChangeTime) + inode->i_ctime = cifs_NTtimeToUnix(file_inf.ChangeTime); + if (file_inf.LastAccessTime) + inode->i_atime = cifs_NTtimeToUnix(file_inf.LastAccessTime); + + /* + * i_blocks is not related to (i_size / i_blksize), + * but instead 512 byte (2**9) size is required for + * calculating num blocks. + */ + if (le64_to_cpu(file_inf.AllocationSize) > 4096) + inode->i_blocks = + (512 - 1 + le64_to_cpu(file_inf.AllocationSize)) >> 9; + + /* End of file and Attributes should not have to be updated on close */ + spin_unlock(&inode->i_lock); +} + static int SMB2_request_res_key(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, u64 volatile_fid, @@ -1512,7 +1551,7 @@ smb2_ioctl_query_info(const unsigned int xid, rqst[2].rq_iov = close_iov; rqst[2].rq_nvec = 1; - rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID); + rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID, false); if (rc) goto iqinf_exit; smb2_set_related(&rqst[2]); @@ -2241,7 +2280,7 @@ smb2_query_info_compound(const unsigned int xid, struct cifs_tcon *tcon, rqst[2].rq_iov = close_iov; rqst[2].rq_nvec = 1; - rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID); + rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID, false); if (rc) goto qic_exit; smb2_set_related(&rqst[2]); @@ -2654,7 +2693,7 @@ smb2_query_symlink(const unsigned int xid, struct cifs_tcon *tcon, rqst[2].rq_iov = close_iov; rqst[2].rq_nvec = 1; - rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID); + rc = SMB2_close_init(tcon, &rqst[2], COMPOUND_FID, COMPOUND_FID, false); if (rc) goto querty_exit; @@ -4707,6 +4746,7 @@ struct smb_version_operations smb30_operations = { .open = smb2_open_file, .set_fid = smb2_set_fid, .close = smb2_close_file, + .close_getattr = smb2_close_getattr, .flush = smb2_flush_file, .async_readv = smb2_async_readv, .async_writev = smb2_async_writev, @@ -4816,6 +4856,7 @@ struct smb_version_operations smb311_operations = { .open = smb2_open_file, .set_fid = smb2_set_fid, .close = smb2_close_file, + .close_getattr = smb2_close_getattr, .flush = smb2_flush_file, .async_readv = smb2_async_readv, .async_writev = smb2_async_writev, diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index cec53eb8a6da..187a5ce68806 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2932,7 +2932,7 @@ SMB2_set_compression(const unsigned int xid, struct cifs_tcon *tcon, int SMB2_close_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, - u64 persistent_fid, u64 volatile_fid) + u64 persistent_fid, u64 volatile_fid, bool query_attrs) { struct smb2_close_req *req; struct kvec *iov = rqst->rq_iov; @@ -2945,6 +2945,10 @@ SMB2_close_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, req->PersistentFileId = persistent_fid; req->VolatileFileId = volatile_fid; + if (query_attrs) + req->Flags = SMB2_CLOSE_FLAG_POSTQUERY_ATTRIB; + else + req->Flags = 0; iov[0].iov_base = (char *)req; iov[0].iov_len = total_len; @@ -2959,8 +2963,9 @@ SMB2_close_free(struct smb_rqst *rqst) } int -SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, - u64 persistent_fid, u64 volatile_fid) +__SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, + u64 persistent_fid, u64 volatile_fid, + struct smb2_file_network_open_info *pbuf) { struct smb_rqst rqst; struct smb2_close_rsp *rsp = NULL; @@ -2970,6 +2975,7 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, int resp_buftype = CIFS_NO_BUFFER; int rc = 0; int flags = 0; + bool query_attrs = false; cifs_dbg(FYI, "Close\n"); @@ -2984,8 +2990,13 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, rqst.rq_iov = iov; rqst.rq_nvec = 1; + /* check if need to ask server to return timestamps in close response */ + if (pbuf) + query_attrs = true; + trace_smb3_close_enter(xid, persistent_fid, tcon->tid, ses->Suid); - rc = SMB2_close_init(tcon, &rqst, persistent_fid, volatile_fid); + rc = SMB2_close_init(tcon, &rqst, persistent_fid, volatile_fid, + query_attrs); if (rc) goto close_exit; @@ -2997,14 +3008,18 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, trace_smb3_close_err(xid, persistent_fid, tcon->tid, ses->Suid, rc); goto close_exit; - } else + } else { trace_smb3_close_done(xid, persistent_fid, tcon->tid, ses->Suid); + /* + * Note that have to subtract 4 since struct network_open_info + * has a final 4 byte pad that close response does not have + */ + if (pbuf) + memcpy(pbuf, (char *)&rsp->CreationTime, sizeof(*pbuf) - 4); + } atomic_dec(&tcon->num_remote_opens); - - /* BB FIXME - decode close response, update inode for caching */ - close_exit: SMB2_close_free(&rqst); free_rsp_buf(resp_buftype, rsp); @@ -3022,6 +3037,13 @@ close_exit: return rc; } +int +SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, + u64 persistent_fid, u64 volatile_fid) +{ + return __SMB2_close(xid, tcon, persistent_fid, volatile_fid, NULL); +} + int smb2_validate_iov(unsigned int offset, unsigned int buffer_length, struct kvec *iov, unsigned int min_buf_size) diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index f264e1d36fe1..fa2533da316d 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -1570,6 +1570,17 @@ struct smb2_file_eof_info { /* encoding of request for level 10 */ __le64 EndOfFile; /* new end of file value */ } __packed; /* level 20 Set */ +struct smb2_file_network_open_info { + __le64 CreationTime; + __le64 LastAccessTime; + __le64 LastWriteTime; + __le64 ChangeTime; + __le64 AllocationSize; + __le64 EndOfFile; + __le32 Attributes; + __le32 Reserved; +} __packed; /* level 34 Query also similar returned in close rsp and open rsp */ + extern char smb2_padding[7]; #endif /* _SMB2PDU_H */ diff --git a/fs/cifs/smb2proto.h b/fs/cifs/smb2proto.h index 5caeaa99cb7c..a18272c987fe 100644 --- a/fs/cifs/smb2proto.h +++ b/fs/cifs/smb2proto.h @@ -155,10 +155,13 @@ extern int SMB2_change_notify(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_fid, u64 volatile_fid, bool watch_tree, u32 completion_filter); +extern int __SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, + u64 persistent_fid, u64 volatile_fid, + struct smb2_file_network_open_info *pbuf); extern int SMB2_close(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_file_id, u64 volatile_file_id); extern int SMB2_close_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, - u64 persistent_file_id, u64 volatile_file_id); + u64 persistent_fid, u64 volatile_fid, bool query_attrs); extern void SMB2_close_free(struct smb_rqst *rqst); extern int SMB2_flush(const unsigned int xid, struct cifs_tcon *tcon, u64 persistent_file_id, u64 volatile_file_id); -- cgit v1.2.3-59-g8ed1b From 3345bb44bacd99413a3dc0dcd9a99449d88d4dda Mon Sep 17 00:00:00 2001 From: "Paulo Alcantara (SUSE)" Date: Wed, 4 Dec 2019 11:25:06 -0300 Subject: cifs: Fix lookup of SMB connections on multichannel With the addition of SMB session channels, we introduced new TCP server pointers that have no sessions or tcons associated with them. In this case, when we started looking for TCP connections, we might end up picking session channel rather than the master connection, hence failing to get either a session or a tcon. In order to fix that, this patch introduces a new "is_channel" field to TCP_Server_Info structure so we can skip session channels during lookup of connections. Signed-off-by: Paulo Alcantara (SUSE) Reviewed-by: Aurelien Aptel Signed-off-by: Steve French --- fs/cifs/cifsglob.h | 1 + fs/cifs/connect.c | 6 +++++- fs/cifs/sess.c | 3 +++ 3 files changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 5b976e01dd6b..fd0262ce5ad5 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -777,6 +777,7 @@ struct TCP_Server_Info { */ int nr_targets; bool noblockcnt; /* use non-blocking connect() */ + bool is_channel; /* if a session channel */ }; struct cifs_credits { diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 86d1baedf21c..05ea0e2b7e0e 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -2712,7 +2712,11 @@ cifs_find_tcp_session(struct smb_vol *vol) spin_lock(&cifs_tcp_ses_lock); list_for_each_entry(server, &cifs_tcp_ses_list, tcp_ses_list) { - if (!match_server(server, vol)) + /* + * Skip ses channels since they're only handled in lower layers + * (e.g. cifs_send_recv). + */ + if (server->is_channel || !match_server(server, vol)) continue; ++server->srv_count; diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index fb3bdc44775c..d95137304224 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -213,6 +213,9 @@ cifs_ses_add_channel(struct cifs_ses *ses, struct cifs_server_iface *iface) chan->server = NULL; goto out; } + spin_lock(&cifs_tcp_ses_lock); + chan->server->is_channel = true; + spin_unlock(&cifs_tcp_ses_lock); /* * We need to allocate the server crypto now as we will need -- cgit v1.2.3-59-g8ed1b From 9a7d5a9e6d7921e1854b4606ce8c3e17d565f463 Mon Sep 17 00:00:00 2001 From: Aurelien Aptel Date: Wed, 4 Dec 2019 16:14:54 +0100 Subject: cifs: fix possible uninitialized access and race on iface_list iface[0] was accessed regardless of the count value and without locking. * check count before accessing any ifaces * make copy of iface list (it's a simple POD array) and use it without locking. Signed-off-by: Aurelien Aptel Signed-off-by: Steve French Reviewed-by: Paulo Alcantara (SUSE) --- fs/cifs/sess.c | 29 ++++++++++++++++++++++++++--- 1 file changed, 26 insertions(+), 3 deletions(-) diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c index d95137304224..f0795c856d8f 100644 --- a/fs/cifs/sess.c +++ b/fs/cifs/sess.c @@ -77,6 +77,8 @@ int cifs_try_adding_channels(struct cifs_ses *ses) int i = 0; int rc = 0; int tries = 0; + struct cifs_server_iface *ifaces = NULL; + size_t iface_count; if (left <= 0) { cifs_dbg(FYI, @@ -90,6 +92,26 @@ int cifs_try_adding_channels(struct cifs_ses *ses) return 0; } + /* + * Make a copy of the iface list at the time and use that + * instead so as to not hold the iface spinlock for opening + * channels + */ + spin_lock(&ses->iface_lock); + iface_count = ses->iface_count; + if (iface_count <= 0) { + spin_unlock(&ses->iface_lock); + cifs_dbg(FYI, "no iface list available to open channels\n"); + return 0; + } + ifaces = kmemdup(ses->iface_list, iface_count*sizeof(*ifaces), + GFP_ATOMIC); + if (!ifaces) { + spin_unlock(&ses->iface_lock); + return 0; + } + spin_unlock(&ses->iface_lock); + /* * Keep connecting to same, fastest, iface for all channels as * long as its RSS. Try next fastest one if not RSS or channel @@ -105,9 +127,9 @@ int cifs_try_adding_channels(struct cifs_ses *ses) break; } - iface = &ses->iface_list[i]; + iface = &ifaces[i]; if (is_ses_using_iface(ses, iface) && !iface->rss_capable) { - i = (i+1) % ses->iface_count; + i = (i+1) % iface_count; continue; } @@ -115,7 +137,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) if (rc) { cifs_dbg(FYI, "failed to open extra channel on iface#%d rc=%d\n", i, rc); - i = (i+1) % ses->iface_count; + i = (i+1) % iface_count; continue; } @@ -124,6 +146,7 @@ int cifs_try_adding_channels(struct cifs_ses *ses) left--; } + kfree(ifaces); return ses->chan_count - old_chan_count; } -- cgit v1.2.3-59-g8ed1b From fdef665ba44ad5ed154af2acfb19ae2ee3bf5dcc Mon Sep 17 00:00:00 2001 From: Steve French Date: Fri, 6 Dec 2019 02:02:38 -0600 Subject: smb3: fix mode passed in on create for modetosid mount option When using the special SID to store the mode bits in an ACE (See http://technet.microsoft.com/en-us/library/hh509017(v=ws.10).aspx) which is enabled with mount parm "modefromsid" we were not passing in the mode via SMB3 create (although chmod was enabled). SMB3 create allows a security descriptor context to be passed in (which is more atomic and thus preferable to setting the mode bits after create via a setinfo). This patch enables setting the mode bits on create when using modefromsid mount option. In addition it fixes an endian error in the definition of the Control field flags in the SMB3 security descriptor. It also makes the ACE type of the special SID better match the documentation (and behavior of servers which use this to store mode bits in SMB3 ACLs). Signed-off-by: Steve French Acked-by: Ronnie Sahlberg Reviewed-by: Pavel Shilovsky --- fs/cifs/cifsacl.c | 42 +++++++++++++++++++------------ fs/cifs/cifsacl.h | 32 ++++++++++++------------ fs/cifs/cifsproto.h | 1 + fs/cifs/smb2pdu.c | 71 +++++++++++++++++++++++++++++++++++++++++++++++++++-- fs/cifs/smb2pdu.h | 10 ++++++++ 5 files changed, 122 insertions(+), 34 deletions(-) diff --git a/fs/cifs/cifsacl.c b/fs/cifs/cifsacl.c index 06ffe52bdcfa..96ae72b556ac 100644 --- a/fs/cifs/cifsacl.c +++ b/fs/cifs/cifsacl.c @@ -802,6 +802,31 @@ static void parse_dacl(struct cifs_acl *pdacl, char *end_of_acl, return; } +/* + * Fill in the special SID based on the mode. See + * http://technet.microsoft.com/en-us/library/hh509017(v=ws.10).aspx + */ +unsigned int setup_special_mode_ACE(struct cifs_ace *pntace, __u64 nmode) +{ + int i; + unsigned int ace_size = 28; + + pntace->type = ACCESS_DENIED_ACE_TYPE; + pntace->flags = 0x0; + pntace->access_req = 0; + pntace->sid.num_subauth = 3; + pntace->sid.revision = 1; + for (i = 0; i < NUM_AUTHS; i++) + pntace->sid.authority[i] = sid_unix_NFS_mode.authority[i]; + + pntace->sid.sub_auth[0] = sid_unix_NFS_mode.sub_auth[0]; + pntace->sid.sub_auth[1] = sid_unix_NFS_mode.sub_auth[1]; + pntace->sid.sub_auth[2] = cpu_to_le32(nmode & 07777); + + /* size = 1 + 1 + 2 + 4 + 1 + 1 + 6 + (psid->num_subauth*4) */ + pntace->size = cpu_to_le16(ace_size); + return ace_size; +} static int set_chmod_dacl(struct cifs_acl *pndacl, struct cifs_sid *pownersid, struct cifs_sid *pgrpsid, __u64 nmode, bool modefromsid) @@ -815,23 +840,8 @@ static int set_chmod_dacl(struct cifs_acl *pndacl, struct cifs_sid *pownersid, if (modefromsid) { struct cifs_ace *pntace = (struct cifs_ace *)((char *)pnndacl + size); - int i; - pntace->type = ACCESS_ALLOWED; - pntace->flags = 0x0; - pntace->access_req = 0; - pntace->sid.num_subauth = 3; - pntace->sid.revision = 1; - for (i = 0; i < NUM_AUTHS; i++) - pntace->sid.authority[i] = - sid_unix_NFS_mode.authority[i]; - pntace->sid.sub_auth[0] = sid_unix_NFS_mode.sub_auth[0]; - pntace->sid.sub_auth[1] = sid_unix_NFS_mode.sub_auth[1]; - pntace->sid.sub_auth[2] = cpu_to_le32(nmode & 07777); - - /* size = 1 + 1 + 2 + 4 + 1 + 1 + 6 + (psid->num_subauth*4) */ - pntace->size = cpu_to_le16(28); - size += 28; + size += setup_special_mode_ACE(pntace, nmode); num_aces++; } diff --git a/fs/cifs/cifsacl.h b/fs/cifs/cifsacl.h index 439b99cefeb0..21d7dee98d01 100644 --- a/fs/cifs/cifsacl.h +++ b/fs/cifs/cifsacl.h @@ -147,22 +147,22 @@ struct smb3_sd { } __packed; /* Meaning of 'Control' field flags */ -#define ACL_CONTROL_SR 0x0001 /* Self relative */ -#define ACL_CONTROL_RM 0x0002 /* Resource manager control bits */ -#define ACL_CONTROL_PS 0x0004 /* SACL protected from inherits */ -#define ACL_CONTROL_PD 0x0008 /* DACL protected from inherits */ -#define ACL_CONTROL_SI 0x0010 /* SACL Auto-Inherited */ -#define ACL_CONTROL_DI 0x0020 /* DACL Auto-Inherited */ -#define ACL_CONTROL_SC 0x0040 /* SACL computed through inheritance */ -#define ACL_CONTROL_DC 0x0080 /* DACL computed through inheritence */ -#define ACL_CONTROL_SS 0x0100 /* Create server ACL */ -#define ACL_CONTROL_DT 0x0200 /* DACL provided by trusteed source */ -#define ACL_CONTROL_SD 0x0400 /* SACL defaulted */ -#define ACL_CONTROL_SP 0x0800 /* SACL is present on object */ -#define ACL_CONTROL_DD 0x1000 /* DACL defaulted */ -#define ACL_CONTROL_DP 0x2000 /* DACL is present on object */ -#define ACL_CONTROL_GD 0x4000 /* Group was defaulted */ -#define ACL_CONTROL_OD 0x8000 /* User was defaulted */ +#define ACL_CONTROL_SR 0x8000 /* Self relative */ +#define ACL_CONTROL_RM 0x4000 /* Resource manager control bits */ +#define ACL_CONTROL_PS 0x2000 /* SACL protected from inherits */ +#define ACL_CONTROL_PD 0x1000 /* DACL protected from inherits */ +#define ACL_CONTROL_SI 0x0800 /* SACL Auto-Inherited */ +#define ACL_CONTROL_DI 0x0400 /* DACL Auto-Inherited */ +#define ACL_CONTROL_SC 0x0200 /* SACL computed through inheritance */ +#define ACL_CONTROL_DC 0x0100 /* DACL computed through inheritence */ +#define ACL_CONTROL_SS 0x0080 /* Create server ACL */ +#define ACL_CONTROL_DT 0x0040 /* DACL provided by trusted source */ +#define ACL_CONTROL_SD 0x0020 /* SACL defaulted */ +#define ACL_CONTROL_SP 0x0010 /* SACL is present on object */ +#define ACL_CONTROL_DD 0x0008 /* DACL defaulted */ +#define ACL_CONTROL_DP 0x0004 /* DACL is present on object */ +#define ACL_CONTROL_GD 0x0002 /* Group was defaulted */ +#define ACL_CONTROL_OD 0x0001 /* User was defaulted */ /* Meaning of AclRevision flags */ #define ACL_REVISION 0x02 /* See section 2.4.4.1 of MS-DTYP */ diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 1ed695336f62..9c229408a251 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -213,6 +213,7 @@ extern struct cifs_ntsd *get_cifs_acl_by_fid(struct cifs_sb_info *, const struct cifs_fid *, u32 *); extern int set_cifs_acl(struct cifs_ntsd *, __u32, struct inode *, const char *, int); +extern unsigned int setup_special_mode_ACE(struct cifs_ace *pace, __u64 nmode); extern void dequeue_mid(struct mid_q_entry *mid, bool malformed); extern int cifs_read_from_socket(struct TCP_Server_Info *server, char *buf, diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 187a5ce68806..b77643e02157 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2191,6 +2191,72 @@ add_twarp_context(struct kvec *iov, unsigned int *num_iovec, __u64 timewarp) return 0; } +/* See MS-SMB2 2.2.13.2.2 and MS-DTYP 2.4.6 */ +static struct crt_sd_ctxt * +create_sd_buf(umode_t mode, unsigned int *len) +{ + struct crt_sd_ctxt *buf; + struct cifs_ace *pace; + unsigned int sdlen, acelen; + + *len = roundup(sizeof(struct crt_sd_ctxt) + sizeof(struct cifs_ace), 8); + buf = kzalloc(*len, GFP_KERNEL); + if (buf == NULL) + return buf; + + sdlen = sizeof(struct smb3_sd) + sizeof(struct smb3_acl) + + sizeof(struct cifs_ace); + + buf->ccontext.DataOffset = cpu_to_le16(offsetof + (struct crt_sd_ctxt, sd)); + buf->ccontext.DataLength = cpu_to_le32(sdlen); + buf->ccontext.NameOffset = cpu_to_le16(offsetof + (struct crt_sd_ctxt, Name)); + buf->ccontext.NameLength = cpu_to_le16(4); + /* SMB2_CREATE_SD_BUFFER_TOKEN is "SecD" */ + buf->Name[0] = 'S'; + buf->Name[1] = 'e'; + buf->Name[2] = 'c'; + buf->Name[3] = 'D'; + buf->sd.Revision = 1; /* Must be one see MS-DTYP 2.4.6 */ + /* + * ACL is "self relative" ie ACL is stored in contiguous block of memory + * and "DP" ie the DACL is present + */ + buf->sd.Control = cpu_to_le16(ACL_CONTROL_SR | ACL_CONTROL_DP); + + /* offset owner, group and Sbz1 and SACL are all zero */ + buf->sd.OffsetDacl = cpu_to_le32(sizeof(struct smb3_sd)); + buf->acl.AclRevision = ACL_REVISION; /* See 2.4.4.1 of MS-DTYP */ + + /* create one ACE to hold the mode embedded in reserved special SID */ + pace = (struct cifs_ace *)(sizeof(struct crt_sd_ctxt) + (char *)buf); + acelen = setup_special_mode_ACE(pace, (__u64)mode); + buf->acl.AclSize = cpu_to_le16(sizeof(struct cifs_acl) + acelen); + buf->acl.AceCount = cpu_to_le16(1); + return buf; +} + +static int +add_sd_context(struct kvec *iov, unsigned int *num_iovec, umode_t mode) +{ + struct smb2_create_req *req = iov[0].iov_base; + unsigned int num = *num_iovec; + unsigned int len = 0; + + iov[num].iov_base = create_sd_buf(mode, &len); + if (iov[num].iov_base == NULL) + return -ENOMEM; + iov[num].iov_len = len; + if (!req->CreateContextsOffset) + req->CreateContextsOffset = cpu_to_le32( + sizeof(struct smb2_create_req) + + iov[num - 1].iov_len); + le32_add_cpu(&req->CreateContextsLength, len); + *num_iovec = num + 1; + return 0; +} + static struct crt_query_id_ctxt * create_query_id_buf(void) { @@ -2563,7 +2629,7 @@ SMB2_open_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, __u8 *oplock, return rc; } - if ((oparms->disposition == FILE_CREATE) && + if ((oparms->disposition != FILE_OPEN) && (oparms->mode != ACL_NO_MODE)) { if (n_iov > 2) { struct create_context *ccontext = @@ -2572,7 +2638,8 @@ SMB2_open_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, __u8 *oplock, cpu_to_le32(iov[n_iov-1].iov_len); } - /* rc = add_sd_context(iov, &n_iov, oparms->mode); */ + cifs_dbg(FYI, "add sd with mode 0x%x\n", oparms->mode); + rc = add_sd_context(iov, &n_iov, oparms->mode); if (rc) return rc; } diff --git a/fs/cifs/smb2pdu.h b/fs/cifs/smb2pdu.h index fa2533da316d..7b1c379fdf7a 100644 --- a/fs/cifs/smb2pdu.h +++ b/fs/cifs/smb2pdu.h @@ -25,6 +25,7 @@ #define _SMB2PDU_H #include +#include /* * Note that, due to trying to use names similar to the protocol specifications, @@ -855,6 +856,15 @@ struct crt_query_id_ctxt { __u8 Name[8]; } __packed; +struct crt_sd_ctxt { + struct create_context ccontext; + __u8 Name[8]; + struct smb3_sd sd; + struct smb3_acl acl; + /* Followed by at least 4 ACEs */ +} __packed; + + #define COPY_CHUNK_RES_KEY_SIZE 24 struct resume_key_req { char ResumeKey[COPY_CHUNK_RES_KEY_SIZE]; -- cgit v1.2.3-59-g8ed1b From 231e2a0ba56733c95cb77d8920e76502b2134e72 Mon Sep 17 00:00:00 2001 From: Steve French Date: Sat, 7 Dec 2019 17:38:22 -0600 Subject: smb3: improve check for when we send the security descriptor context on create We had cases in the previous patch where we were sending the security descriptor context on SMB3 open (file create) in cases when we hadn't mounted with with "modefromsid" mount option. Add check for that mount flag before calling ad_sd_context in open init. Signed-off-by: Steve French Reviewed-by: Pavel Shilovsky --- fs/cifs/smb2pdu.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index b77643e02157..0ab6b1200288 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2630,6 +2630,8 @@ SMB2_open_init(struct cifs_tcon *tcon, struct smb_rqst *rqst, __u8 *oplock, } if ((oparms->disposition != FILE_OPEN) && + (oparms->cifs_sb) && + (oparms->cifs_sb->mnt_cifs_flags & CIFS_MOUNT_MODE_FROM_SID) && (oparms->mode != ACL_NO_MODE)) { if (n_iov > 2) { struct create_context *ccontext = -- cgit v1.2.3-59-g8ed1b