[PATCH] CIFS: Make ops->close return void

[PATCH] CIFS: Make ops->close return void

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/cifsglob.h |    3 ++-
 fs/cifs/file.c     |    5 ++---
 fs/cifs/smb1ops.c  |    4 ++--
 fs/cifs/smb2ops.c  |    4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index a39e5b7..f6f4063 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -316,7 +316,8 @@ struct smb_version_operations {
 	/* set fid protocol-specific info */
 	void (*set_fid)(struct cifsFileInfo *, struct cifs_fid *, __u32);
 	/* close a file */
-	int (*close)(const unsigned int, struct cifs_tcon *, struct cifs_fid *);
+	void (*close)(const unsigned int, struct cifs_tcon *,
+		      struct cifs_fid *);
 	/* 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 075f7cf..7d7bbdc 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -355,12 +355,11 @@ void cifsFileInfo_put(struct cifsFileInfo *cifs_file)
 	if (!tcon->need_reconnect && !cifs_file->invalidHandle) {
 		struct TCP_Server_Info *server = tcon->ses->server;
 		unsigned int xid;
-		int rc = -ENOSYS;
 
 		xid = get_xid();
 		if (server->ops->close)
-			rc = server->ops->close(xid, tcon, &cifs_file->fid);
-		free_xid(xid);
+			server->ops->close(xid, tcon, &cifs_file->fid);
+		_free_xid(xid);
 	}
 
 	cifs_del_pending_open(&open);
diff --git a/fs/cifs/smb1ops.c b/fs/cifs/smb1ops.c
index 5fb0fe5..42dccbb 100644
--- a/fs/cifs/smb1ops.c
+++ b/fs/cifs/smb1ops.c
@@ -720,11 +720,11 @@ cifs_set_fid(struct cifsFileInfo *cfile, struct cifs_fid *fid, __u32 oplock)
 	cinode->can_cache_brlcks = cinode->clientCanCacheAll;
 }
 
-static int
+static void
 cifs_close_file(const unsigned int xid, struct cifs_tcon *tcon,
 		struct cifs_fid *fid)
 {
-	return CIFSSMBClose(xid, tcon, fid->netfid);
+	CIFSSMBClose(xid, tcon, fid->netfid);
 }
 
 static int
diff --git a/fs/cifs/smb2ops.c b/fs/cifs/smb2ops.c
index 2183bb3..1570cbe 100644
--- a/fs/cifs/smb2ops.c
+++ b/fs/cifs/smb2ops.c
@@ -374,11 +374,11 @@ smb2_set_fid(struct cifsFileInfo *cfile, struct cifs_fid *fid, __u32 oplock)
 	cinode->can_cache_brlcks = cinode->clientCanCacheAll;
 }
 
-static int
+static void
 smb2_close_file(const unsigned int xid, struct cifs_tcon *tcon,
 		struct cifs_fid *fid)
 {
-	return SMB2_close(xid, tcon, fid->persistent_fid, fid->volatile_fid);
+	SMB2_close(xid, tcon, fid->persistent_fid, fid->volatile_fid);
 }
 
 static int
-- 
1.7.5.4

[PATCH] CIFS: Fix possible freed pointer dereference in CIFS_SessSetup

[Pavel Shilovsky]

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/sess.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
index 382c06d..76809f4 100644
--- a/fs/cifs/sess.c
+++ b/fs/cifs/sess.c
@@ -876,7 +876,8 @@ ssetup_ntlmssp_authenticate:
 	pSMB = (SESSION_SETUP_ANDX *)iov[0].iov_base;
 	smb_buf = (struct smb_hdr *)iov[0].iov_base;
 
-	if ((type == RawNTLMSSP) && (smb_buf->Status.CifsError ==
+	if ((type == RawNTLMSSP) && (resp_buf_type != CIFS_NO_BUFFER) &&
+	    (smb_buf->Status.CifsError ==
 			cpu_to_le32(NT_STATUS_MORE_PROCESSING_REQUIRED))) {
 		if (phase != NtLmNegotiate) {
 			cERROR(1, "Unexpected more processing error");
-- 
1.7.5.4

[PATCH] CIFS: Fix possible freed pointer dereference in SMB2_sess_setup

[Pavel Shilovsky]

and remove redundant (rsp == NULL) checks after SendReceive2.

Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
---
 fs/cifs/smb2pdu.c |   35 +++--------------------------------
 1 files changed, 3 insertions(+), 32 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index a7db95f..5ad88b4b 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -409,11 +409,6 @@ SMB2_negotiate(const unsigned int xid, struct cifs_ses *ses)
 	if (rc != 0)
 		goto neg_exit;
 
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto neg_exit;
-	}
-
 	cFYI(1, "mode 0x%x", rsp->SecurityMode);
 
 	if (rsp->DialectRevision == smb2protocols[SMB21_PROT].name)
@@ -637,13 +632,14 @@ ssetup_ntlmssp_authenticate:
 
 	kfree(security_blob);
 	rsp = (struct smb2_sess_setup_rsp *)iov[0].iov_base;
-	if (rsp->hdr.Status == STATUS_MORE_PROCESSING_REQUIRED) {
+	if (resp_buftype != CIFS_NO_BUFFER &&
+	    rsp->hdr.Status == STATUS_MORE_PROCESSING_REQUIRED) {
 		if (phase != NtLmNegotiate) {
 			cERROR(1, "Unexpected more processing error");
 			goto ssetup_exit;
 		}
 		if (offsetof(struct smb2_sess_setup_rsp, Buffer) - 4 !=
-			le16_to_cpu(rsp->SecurityBufferOffset)) {
+				le16_to_cpu(rsp->SecurityBufferOffset)) {
 			cERROR(1, "Invalid security buffer offset %d",
 				  le16_to_cpu(rsp->SecurityBufferOffset));
 			rc = -EIO;
@@ -669,11 +665,6 @@ ssetup_ntlmssp_authenticate:
 	if (rc != 0)
 		goto ssetup_exit;
 
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto ssetup_exit;
-	}
-
 	ses->session_flags = le16_to_cpu(rsp->SessionFlags);
 ssetup_exit:
 	free_rsp_buf(resp_buftype, rsp);
@@ -793,11 +784,6 @@ SMB2_tcon(const unsigned int xid, struct cifs_ses *ses, const char *tree,
 		goto tcon_error_exit;
 	}
 
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto tcon_exit;
-	}
-
 	if (tcon == NULL) {
 		ses->ipc_tid = rsp->hdr.TreeId;
 		goto tcon_exit;
@@ -1046,10 +1032,6 @@ SMB2_open(const unsigned int xid, struct cifs_tcon *tcon, __le16 *path,
 		goto creat_exit;
 	}
 
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto creat_exit;
-	}
 	*persistent_fid = rsp->PersistentFileId;
 	*volatile_fid = rsp->VolatileFileId;
 
@@ -1111,11 +1093,6 @@ SMB2_close(const unsigned int xid, struct cifs_tcon *tcon,
 		goto close_exit;
 	}
 
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto close_exit;
-	}
-
 	/* BB FIXME - decode close response, update inode for caching */
 
 close_exit:
@@ -1950,12 +1927,6 @@ send_set_info(const unsigned int xid, struct cifs_tcon *tcon,
 		cifs_stats_fail_inc(tcon, SMB2_SET_INFO_HE);
 		goto out;
 	}
-
-	if (rsp == NULL) {
-		rc = -EIO;
-		goto out;
-	}
-
 out:
 	free_rsp_buf(resp_buftype, rsp);
 	kfree(iov);
-- 
1.7.5.4

Re: [PATCH] CIFS: Fix possible freed pointer dereference in CIFS_SessSetup

[Steve French]

merged into cifs-2.6.git (along with the other two in this series)

On Tue, Sep 25, 2012 at 2:00 AM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> Signed-off-by: Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org>
> ---
>  fs/cifs/sess.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/cifs/sess.c b/fs/cifs/sess.c
> index 382c06d..76809f4 100644
> --- a/fs/cifs/sess.c
> +++ b/fs/cifs/sess.c
> @@ -876,7 +876,8 @@ ssetup_ntlmssp_authenticate:
>         pSMB = (SESSION_SETUP_ANDX *)iov[0].iov_base;
>         smb_buf = (struct smb_hdr *)iov[0].iov_base;
>
> -       if ((type == RawNTLMSSP) && (smb_buf->Status.CifsError ==
> +       if ((type == RawNTLMSSP) && (resp_buf_type != CIFS_NO_BUFFER) &&
> +           (smb_buf->Status.CifsError ==
>                         cpu_to_le32(NT_STATUS_MORE_PROCESSING_REQUIRED))) {
>                 if (phase != NtLmNegotiate) {
>                         cERROR(1, "Unexpected more processing error");



-- 
Thanks,

Steve

Re: [PATCH] CIFS: Fix possible freed pointer dereference in CIFS_SessSetup

[Pavel Shilovsky]

2012/9/27 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> merged into cifs-2.6.git (along with the other two in this

What do you think about Cc'ing it to stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org as well?

-- 
Best regards,
Pavel Shilovsky.

Re: [PATCH] CIFS: Fix possible freed pointer dereference in CIFS_SessSetup

[Steve French]

On Thu, Sep 27, 2012 at 3:17 AM, Pavel Shilovsky <piastry-7qunaywFIewox3rIn2DAYQ@public.gmane.org> wrote:
> 2012/9/27 Steve French <smfrench-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
>> merged into cifs-2.6.git (along with the other two in this
>
> What do you think about Cc'ing it to stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org as well?

I don't think it is necessary but if you think a problem is
reproducible let me know.  In the case where

           resp_buf_type == CIFS_NO_BUFFER

then we have iov pointing to the original request and therefore

    smb_buf->Status.CifsError ==
                        cpu_to_le32(NT_STATUS_MORE_PROCESSING_REQUIRED

will never be true  Your code (adding the check for resp_buf_type)
makes it cleaner and better, but  I don't see how we can end up
oopsing or falling into this section of code accidentally with the
previous version.
-- 
Thanks,

Steve