From 350be257ea83029daee974c72b1fe2e6f1f8e615 Mon Sep 17 00:00:00 2001 From: Pavel Shilovsky Date: Mon, 10 Apr 2017 10:31:33 -0700 Subject: [PATCH 1/6] CIFS: Fix null pointer deref during read resp processing Currently during receiving a read response mid->resp_buf can be NULL when it is being passed to cifs_discard_remaining_data() from cifs_readv_discard(). Fix it by always passing server->smallbuf instead and initializing mid->resp_buf at the end of read response processing. Signed-off-by: Pavel Shilovsky CC: Stable Acked-by: Sachin Prabhu Signed-off-by: Steve French --- fs/cifs/cifsproto.h | 3 +-- fs/cifs/cifssmb.c | 15 ++++++++------- fs/cifs/smb2ops.c | 4 ++-- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index ec5e5e514fdd..97e5d236d265 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -79,8 +79,7 @@ extern void cifs_delete_mid(struct mid_q_entry *mid); extern void cifs_wake_up_task(struct mid_q_entry *mid); extern int cifs_handle_standard(struct TCP_Server_Info *server, struct mid_q_entry *mid); -extern int cifs_discard_remaining_data(struct TCP_Server_Info *server, - char *buf); +extern int cifs_discard_remaining_data(struct TCP_Server_Info *server); extern int cifs_call_async(struct TCP_Server_Info *server, struct smb_rqst *rqst, mid_receive_t *receive, mid_callback_t *callback, diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index 967b92631807..5d21f00ae341 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -1400,9 +1400,9 @@ openRetry: * current bigbuf. */ int -cifs_discard_remaining_data(struct TCP_Server_Info *server, char *buf) +cifs_discard_remaining_data(struct TCP_Server_Info *server) { - unsigned int rfclen = get_rfc1002_length(buf); + unsigned int rfclen = get_rfc1002_length(server->smallbuf); int remaining = rfclen + 4 - server->total_read; while (remaining > 0) { @@ -1426,8 +1426,10 @@ cifs_readv_discard(struct TCP_Server_Info *server, struct mid_q_entry *mid) int length; struct cifs_readdata *rdata = mid->callback_data; - length = cifs_discard_remaining_data(server, mid->resp_buf); + length = cifs_discard_remaining_data(server); dequeue_mid(mid, rdata->result); + mid->resp_buf = server->smallbuf; + server->smallbuf = NULL; return length; } @@ -1459,7 +1461,7 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) if (server->ops->is_status_pending && server->ops->is_status_pending(buf, server, 0)) { - cifs_discard_remaining_data(server, buf); + cifs_discard_remaining_data(server); return -1; } @@ -1519,9 +1521,6 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) cifs_dbg(FYI, "0: iov_base=%p iov_len=%u\n", rdata->iov[0].iov_base, server->total_read); - mid->resp_buf = server->smallbuf; - server->smallbuf = NULL; - /* how much data is in the response? */ data_len = server->ops->read_data_length(buf); if (data_offset + data_len > buflen) { @@ -1544,6 +1543,8 @@ cifs_readv_receive(struct TCP_Server_Info *server, struct mid_q_entry *mid) return cifs_readv_discard(server, mid); dequeue_mid(mid, false); + mid->resp_buf = server->smallbuf; + server->smallbuf = NULL; return length; } diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c index 7b12a727947e..152e37f2ad92 100644 --- a/fs/cifs/smb2ops.c +++ b/fs/cifs/smb2ops.c @@ -2195,7 +2195,7 @@ receive_encrypted_read(struct TCP_Server_Info *server, struct mid_q_entry **mid) if (rc) goto free_pages; - rc = cifs_discard_remaining_data(server, buf); + rc = cifs_discard_remaining_data(server); if (rc) goto free_pages; @@ -2221,7 +2221,7 @@ free_pages: kfree(pages); return rc; discard_data: - cifs_discard_remaining_data(server, buf); + cifs_discard_remaining_data(server); goto free_pages; } From 40920c2bb119fd49ba03e2f97a172171781be442 Mon Sep 17 00:00:00 2001 From: Mark Syms Date: Tue, 29 Nov 2016 11:36:46 +0000 Subject: [PATCH 2/6] CIFS: handle guest access errors to Windows shares Commit 1a967d6c9b39c226be1b45f13acd4d8a5ab3dc44 ("correctly to anonymous authentication for the NTLM(v2) authentication") introduces a regression in handling errors related to attempting a guest connection to a Windows share which requires authentication. This should result in a permission denied error but actually causes the kernel module to enter a never-ending loop trying to follow a DFS referal which doesn't exist. The base cause of this is the failure now occurs later in the process during tree connect and not at the session setup setup and all errors in tree connect are interpreted as needing to follow the DFS paths which isn't in this case correct. So, check the returned error against EACCES and fail if this is returned error. Feedback from Aurelien: PS> net user guest /activate:no PS> mkdir C:\guestshare PS> icacls C:\guestshare /grant 'Everyone:(OI)(CI)F' PS> new-smbshare -name guestshare -path C:\guestshare -fullaccess Everyone I've tested v3.10, v4.4, master, master+your patch using default options (empty or no user "NU") and user=abc (U). NT_LOGON_FAILURE in session setup: LF This is what you seem to have in 3.10. NT_ACCESS_DENIED in tree connect to the share: AD This is what you get before your infinite loop. | NU U -------------------------------- 3.10 | LF LF 4.4 | LF LF master | AD LF master+patch | AD LF No infinite DFS loop :( All these issues result in mount failing very fast with permission denied. I guess it could be from either the Windows version or the share/folder ACL. A deeper analysis of the packets might reveal more. In any case I did not notice any issues for on a basic DFS setup with the patch so I don't think it introduced any regressions, which is probably all that matters. It still bothers me a little I couldn't hit the bug. I've included kernel output w/ debugging output and network capture of my tests if anyone want to have a look at it. (master+patch = ml-guestfix). Signed-off-by: Mark Syms Reviewed-by: Aurelien Aptel Tested-by: Aurelien Aptel Acked-by: Pavel Shilovsky Signed-off-by: Steve French --- fs/cifs/connect.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 0c7596cef4b8..d82467cfb0e2 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -3753,6 +3753,9 @@ try_mount_again: if (IS_ERR(tcon)) { rc = PTR_ERR(tcon); tcon = NULL; + if (rc == -EACCES) + goto mount_fail_check; + goto remote_path_check; } From 18ea43113f5b74a97dd4be9bddbac10d68b1a6ce Mon Sep 17 00:00:00 2001 From: Germano Percossi Date: Fri, 7 Apr 2017 12:29:36 +0100 Subject: [PATCH 3/6] CIFS: reconnect thread reschedule itself In case of error, smb2_reconnect_server reschedule itself with a delay, to avoid being too aggressive. Signed-off-by: Germano Percossi Reviewed-by: Pavel Shilovsky CC: Stable Signed-off-by: Steve French --- fs/cifs/smb2pdu.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 66fa1b941cdf..d09e98bb8584 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -2181,6 +2181,9 @@ void smb2_reconnect_server(struct work_struct *work) struct cifs_tcon *tcon, *tcon2; struct list_head tmp_list; int tcon_exist = false; + int rc; + int resched = false; + /* Prevent simultaneous reconnects that can corrupt tcon->rlist list */ mutex_lock(&server->reconnect_mutex); @@ -2208,13 +2211,18 @@ void smb2_reconnect_server(struct work_struct *work) spin_unlock(&cifs_tcp_ses_lock); list_for_each_entry_safe(tcon, tcon2, &tmp_list, rlist) { - if (!smb2_reconnect(SMB2_INTERNAL_CMD, tcon)) + rc = smb2_reconnect(SMB2_INTERNAL_CMD, tcon); + if (!rc) cifs_reopen_persistent_handles(tcon); + else + resched = true; list_del_init(&tcon->rlist); cifs_put_tcon(tcon); } cifs_dbg(FYI, "Reconnecting tcons finished\n"); + if (resched) + queue_delayed_work(cifsiod_wq, &server->reconnect, 2 * HZ); mutex_unlock(&server->reconnect_mutex); /* now we can safely release srv struct */ From a0918f1ce6a43ac980b42b300ec443c154970979 Mon Sep 17 00:00:00 2001 From: Germano Percossi Date: Fri, 7 Apr 2017 12:29:37 +0100 Subject: [PATCH 4/6] CIFS: remove bad_network_name flag STATUS_BAD_NETWORK_NAME can be received during node failover, causing the flag to be set and making the reconnect thread always unsuccessful, thereafter. Once the only place where it is set is removed, the remaining bits are rendered moot. Removing it does not prevent "mount" from failing when a non existent share is passed. What happens when the share really ceases to exist while the share is mounted is undefined now as much as it was before. Signed-off-by: Germano Percossi Reviewed-by: Pavel Shilovsky CC: Stable Signed-off-by: Steve French --- fs/cifs/cifsglob.h | 1 - fs/cifs/smb2pdu.c | 5 ----- 2 files changed, 6 deletions(-) diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index d07f13a63369..37f5a41cc50c 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -948,7 +948,6 @@ struct cifs_tcon { bool use_persistent:1; /* use persistent instead of durable handles */ #ifdef CONFIG_CIFS_SMB2 bool print:1; /* set if connection to printer share */ - bool bad_network_name:1; /* set if ret status STATUS_BAD_NETWORK_NAME */ __le32 capabilities; __u32 share_flags; __u32 maximal_access; diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index d09e98bb8584..1bd5d3033fc8 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -1171,9 +1171,6 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree, else return -EIO; - if (tcon && tcon->bad_network_name) - return -ENOENT; - unc_path = kmalloc(MAX_SHARENAME_LENGTH * 2, GFP_KERNEL); if (unc_path == NULL) return -ENOMEM; @@ -1277,8 +1274,6 @@ tcon_exit: tcon_error_exit: if (rsp->hdr.sync_hdr.Status == STATUS_BAD_NETWORK_NAME) { cifs_dbg(VFS, "BAD_NETWORK_NAME: %s\n", tree); - if (tcon) - tcon->bad_network_name = true; } goto tcon_exit; } From 1fa839b4986d648b907d117275869a0e46c324b9 Mon Sep 17 00:00:00 2001 From: Germano Percossi Date: Fri, 7 Apr 2017 12:29:38 +0100 Subject: [PATCH 5/6] CIFS: store results of cifs_reopen_file to avoid infinite wait This fixes Continuous Availability when errors during file reopen are encountered. cifs_user_readv and cifs_user_writev would wait for ever if results of cifs_reopen_file are not stored and for later inspection. In fact, results are checked and, in case of errors, a chain of function calls leading to reads and writes to be scheduled in a separate thread is skipped. These threads will wake up the corresponding waiters once reads and writes are done. However, given the return value is not stored, when rc is checked for errors a previous one (always zero) is inspected instead. This leads to pending reads/writes added to the list, making cifs_user_readv and cifs_user_writev wait for ever. Signed-off-by: Germano Percossi Reviewed-by: Pavel Shilovsky CC: Stable Signed-off-by: Steve French --- fs/cifs/file.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fs/cifs/file.c b/fs/cifs/file.c index aa3debbba826..21d404535739 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -2597,7 +2597,7 @@ cifs_write_from_iter(loff_t offset, size_t len, struct iov_iter *from, wdata->credits = credits; if (!wdata->cfile->invalidHandle || - !cifs_reopen_file(wdata->cfile, false)) + !(rc = cifs_reopen_file(wdata->cfile, false))) rc = server->ops->async_writev(wdata, cifs_uncached_writedata_release); if (rc) { @@ -3022,7 +3022,7 @@ cifs_send_async_read(loff_t offset, size_t len, struct cifsFileInfo *open_file, rdata->credits = credits; if (!rdata->cfile->invalidHandle || - !cifs_reopen_file(rdata->cfile, true)) + !(rc = cifs_reopen_file(rdata->cfile, true))) rc = server->ops->async_readv(rdata); error: if (rc) { @@ -3617,7 +3617,7 @@ static int cifs_readpages(struct file *file, struct address_space *mapping, } if (!rdata->cfile->invalidHandle || - !cifs_reopen_file(rdata->cfile, true)) + !(rc = cifs_reopen_file(rdata->cfile, true))) rc = server->ops->async_readv(rdata); if (rc) { add_credits_and_wake_if(server, rdata->credits, 0); From 67dbea2ce6873f8ba57988ba3e608e8bf61c347f Mon Sep 17 00:00:00 2001 From: Pavel Shilovsky Date: Wed, 12 Apr 2017 13:32:07 -0700 Subject: [PATCH 6/6] CIFS: Fix SMB3 mount without specifying a security mechanism Commit ef65aaede23f ("smb2: Enforce sec= mount option") changed the behavior of a mount command to enforce a specified security mechanism during mounting. On another hand according to the spec if SMB3 server doesn't respond with a security context it implies that it supports NTLMSSP. The current code doesn't keep it in mind and fails a mount for such servers if no security mechanism is specified. Fix this by indicating that a server supports NTLMSSP if a security context isn't returned during negotiate phase. This allows the code to use NTLMSSP by default for SMB3 mounts. Signed-off-by: Pavel Shilovsky Signed-off-by: Steve French --- fs/cifs/smb2pdu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c index 1bd5d3033fc8..02da648041fc 100644 --- a/fs/cifs/smb2pdu.c +++ b/fs/cifs/smb2pdu.c @@ -562,8 +562,10 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses) * but for time being this is our only auth choice so doesn't matter. * We just found a server which sets blob length to zero expecting raw. */ - if (blob_length == 0) + if (blob_length == 0) { cifs_dbg(FYI, "missing security blob on negprot\n"); + server->sec_ntlmssp = true; + } rc = cifs_enable_signing(server, ses->sign); if (rc)