[PATCH 0/4] Fix various coverity warnings in fs/cifs

[PATCH 0/4] Fix various coverity warnings in fs/cifs

[Steve French]

Four small fixes to address warnings coverity analyzer displays
for fs/cifs.

Steve French (4):
  [SMB3] Fix warning on uninitialized buftype
  [CIFS] Don't ignore errors on encrypting password in SMBTcon
  [SMB3] Fix dereference before null check warning
  [SMB3] Fix coverity warning

 fs/cifs/connect.c  |  6 ++++++
 fs/cifs/smb2misc.c |  2 +-
 fs/cifs/smb2pdu.c  | 15 +++++++++------
 3 files changed, 16 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

[Steve French]

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 <smfrench@gmail.com>
---
 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 3417340..1b906de 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;
-- 
1.9.1

[PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

[Steve French]

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 <smfrench@gmail.com>
---
 fs/cifs/connect.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 4cb8450..cdb1aaf 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) {
-- 
1.9.1

[PATCH 3/4] [SMB3] Fix dereference before null check warning

[Steve French]

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 <smfrench@gmail.com>
---
 fs/cifs/smb2pdu.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 1b906de..78b329f 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;
 		}
 	}
-- 
1.9.1

[PATCH 4/4] [SMB3] Fix coverity warning

[Steve French]

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 <smfrench@gmail.com>
---
 fs/cifs/smb2misc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
index 689f035..22dfdf1 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;
 }
-- 
1.9.1

Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

[Shirish Pargaonkar]

Looks correct.

Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

On Fri, Mar 27, 2015 at 12:27 AM, Steve French <smfrench@gmail.com> wrote:
> 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 <smfrench@gmail.com>
> ---
>  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 3417340..1b906de 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;
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

[Shirish Pargaonkar]

Looks correct, although we could just goto a label at the end of this
function which
does the same.

Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>


On Fri, Mar 27, 2015 at 12:28 AM, Steve French <smfrench@gmail.com> wrote:
> 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 <smfrench@gmail.com>
> ---
>  fs/cifs/connect.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4cb8450..cdb1aaf 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) {
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

[Shirish Pargaonkar]

Looks correct.

Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

On Fri, Mar 27, 2015 at 12:28 AM, Steve French <smfrench@gmail.com> wrote:
> 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 <smfrench@gmail.com>
> ---
>  fs/cifs/smb2pdu.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1b906de..78b329f 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;
>                 }
>         }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] [SMB3] Fix coverity warning

[Shirish Pargaonkar]

Looks correct.

Acked-by: Shirish Pargaonkar <shirishpargaonkar@gmail.com>

On Fri, Mar 27, 2015 at 12:28 AM, Steve French <smfrench@gmail.com> wrote:
> 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 <smfrench@gmail.com>
> ---
>  fs/cifs/smb2misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 689f035..22dfdf1 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;
>  }
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

[Sachin Prabhu]

On Fri, 2015-03-27 at 00:27 -0500, Steve French wrote:
> 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 <smfrench@gmail.com>

ACKed-by: Sachin Prabhu <sprabhu@redhat.com>

> ---
>  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 3417340..1b906de 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;

Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

[Sachin Prabhu]

On Fri, 2015-03-27 at 00:28 -0500, Steve French wrote:
> 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 <smfrench@gmail.com>

ACKed-by: Sachin Prabhu <sprabhu@redhat.com>

> ---
>  fs/cifs/connect.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4cb8450..cdb1aaf 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) {

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

[Sachin Prabhu]

On Fri, 2015-03-27 at 00:28 -0500, Steve French wrote:
> 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 <smfrench@gmail.com>

ACKed-by: Sachin Prabhu <sprabhu@redhat.com>

> ---
>  fs/cifs/smb2pdu.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1b906de..78b329f 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;
>  		}
>  	}

Re: [PATCH 4/4] [SMB3] Fix coverity warning

[Sachin Prabhu]

On Fri, 2015-03-27 at 00:28 -0500, Steve French wrote:
> 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 <smfrench@gmail.com>

Acked-by: Sachin Prabhu <sprabhu@redhat.com>

> ---
>  fs/cifs/smb2misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 689f035..22dfdf1 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;
>  }

Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype

[Jeff Layton]

On Fri, 27 Mar 2015 00:27:59 -0500
Steve French <smfrench@gmail.com> wrote:

> 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 <smfrench@gmail.com>
> ---
>  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 3417340..1b906de 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;

Reviewed-by: Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon

[Jeff Layton]

On Fri, 27 Mar 2015 00:28:00 -0500
Steve French <smfrench@gmail.com> wrote:

> 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 <smfrench@gmail.com>
> ---
>  fs/cifs/connect.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 4cb8450..cdb1aaf 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) {

Acked-by:
-- 
Jeff Layton <jlayton@samba.org>

Re: [PATCH 4/4] [SMB3] Fix coverity warning

[Jeff Layton]

On Fri, 27 Mar 2015 00:28:02 -0500
Steve French <smfrench@gmail.com> wrote:

> 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 <smfrench@gmail.com>
> ---
>  fs/cifs/smb2misc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/cifs/smb2misc.c b/fs/cifs/smb2misc.c
> index 689f035..22dfdf1 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;
>  }

Acked-by: Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

[Jeff Layton]

On Fri, 27 Mar 2015 00:28:01 -0500
Steve French <smfrench@gmail.com> wrote:

> 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 <smfrench@gmail.com>

I don't get it. Under what circumstances would the tcon ever be NULL
here? If there aren't any then this is just confusing. It would be
better to just remove the bogus checks for a NULL tcon instead.

> ---
>  fs/cifs/smb2pdu.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> index 1b906de..78b329f 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;
>  		}
>  	}


-- 
Jeff Layton <jlayton@poochiereds.net>

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

[Steve French]

On Tue, Mar 31, 2015 at 7:46 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> On Fri, 27 Mar 2015 00:28:01 -0500
> Steve French <smfrench@gmail.com> wrote:
>
>> 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 <smfrench@gmail.com>
>
> I don't get it. Under what circumstances would the tcon ever be NULL
> here? If there aren't any then this is just confusing. It would be
> better to just remove the bogus checks for a NULL tcon instead.

I don't think it really matters much one way or another but I agree it
would be a bug to pass in a null tcon to SMB2_ioctl.

On the other hand ... if there are any paths where tcon might be null
(other than SessionSetup and NegProt and TCon itself) due to bug,
SMB2/SMB3 ioctl/fsctl would be the one since there are various strange
operations (such as security related calls such as validate negotiate
for example) that either call it now or will need to call it as
additional SMB2/SMB3 ioctls are added.  I didn't see any harm in
checking for null tcon, although clearly passing in a null tcon would
be a bug - this is one code path where I don't mind checking since
there are some counterintuitive things which SMB2 ioctl/fsctl protocol
operations do.


>> ---
>>  fs/cifs/smb2pdu.c | 13 ++++++++-----
>>  1 file changed, 8 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
>> index 1b906de..78b329f 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;
>>               }
>>       }
>
>



-- 
Thanks,

Steve

Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning

[Jeff Layton]

On Wed, 1 Apr 2015 00:00:57 -0500
Steve French <smfrench@gmail.com> wrote:

> On Tue, Mar 31, 2015 at 7:46 PM, Jeff Layton <jlayton@poochiereds.net> wrote:
> > On Fri, 27 Mar 2015 00:28:01 -0500
> > Steve French <smfrench@gmail.com> wrote:
> >
> >> 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 <smfrench@gmail.com>
> >
> > I don't get it. Under what circumstances would the tcon ever be NULL
> > here? If there aren't any then this is just confusing. It would be
> > better to just remove the bogus checks for a NULL tcon instead.
> 
> I don't think it really matters much one way or another but I agree it
> would be a bug to pass in a null tcon to SMB2_ioctl.
> 
> On the other hand ... if there are any paths where tcon might be null
> (other than SessionSetup and NegProt and TCon itself) due to bug,
> SMB2/SMB3 ioctl/fsctl would be the one since there are various strange
> operations (such as security related calls such as validate negotiate
> for example) that either call it now or will need to call it as
> additional SMB2/SMB3 ioctls are added.  I didn't see any harm in
> checking for null tcon, although clearly passing in a null tcon would
> be a bug - this is one code path where I don't mind checking since
> there are some counterintuitive things which SMB2 ioctl/fsctl protocol
> operations do.
> 

I guess my thinking was that an ioctl syscall requires that you have an
open file, and that requires a fully-established tcon with cifs.ko. So
I don't quite understand how you could get into SMB2_ioctl and not have
a valid tcon.

Bogus NULL pointer checks like this may seem harmless, but they tend to
lead toward the papering over of real bugs. My suggestion would be to
get rid of any pretense that passing in a NULL tcon is OK here.

> 
> >> ---
> >>  fs/cifs/smb2pdu.c | 13 ++++++++-----
> >>  1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
> >> index 1b906de..78b329f 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;
> >>               }
> >>       }
> >
> >
> 
> 
> 


-- 
Jeff Layton <jlayton@samba.org>