* [PATCH 0/4] Fix various coverity warnings in fs/cifs
@ 2015-03-27 5:27 Steve French
2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French
` (3 more replies)
0 siblings, 4 replies; 19+ messages in thread
From: Steve French @ 2015-03-27 5:27 UTC (permalink / raw)
To: linux-cifs, linux-kernel; +Cc: smfrench
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype 2015-03-27 5:27 [PATCH 0/4] Fix various coverity warnings in fs/cifs Steve French @ 2015-03-27 5:27 ` Steve French 2015-03-28 14:19 ` Shirish Pargaonkar ` (2 more replies) 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French ` (2 subsequent siblings) 3 siblings, 3 replies; 19+ messages in thread From: Steve French @ 2015-03-27 5:27 UTC (permalink / raw) To: linux-cifs, linux-kernel; +Cc: smfrench 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French @ 2015-03-28 14:19 ` Shirish Pargaonkar 2015-03-30 9:36 ` Sachin Prabhu 2015-04-01 0:35 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Shirish Pargaonkar @ 2015-03-28 14:19 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, LKML 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French 2015-03-28 14:19 ` Shirish Pargaonkar @ 2015-03-30 9:36 ` Sachin Prabhu 2015-04-01 0:35 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Sachin Prabhu @ 2015-03-30 9:36 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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; ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French 2015-03-28 14:19 ` Shirish Pargaonkar 2015-03-30 9:36 ` Sachin Prabhu @ 2015-04-01 0:35 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Jeff Layton @ 2015-04-01 0:35 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon 2015-03-27 5:27 [PATCH 0/4] Fix various coverity warnings in fs/cifs Steve French 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French @ 2015-03-27 5:28 ` Steve French 2015-03-28 14:26 ` Shirish Pargaonkar ` (2 more replies) 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French 3 siblings, 3 replies; 19+ messages in thread From: Steve French @ 2015-03-27 5:28 UTC (permalink / raw) To: linux-cifs, linux-kernel; +Cc: smfrench 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French @ 2015-03-28 14:26 ` Shirish Pargaonkar 2015-03-30 9:44 ` Sachin Prabhu 2015-04-01 0:37 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Shirish Pargaonkar @ 2015-03-28 14:26 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, LKML 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French 2015-03-28 14:26 ` Shirish Pargaonkar @ 2015-03-30 9:44 ` Sachin Prabhu 2015-04-01 0:37 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Sachin Prabhu @ 2015-03-30 9:44 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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) { ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French 2015-03-28 14:26 ` Shirish Pargaonkar 2015-03-30 9:44 ` Sachin Prabhu @ 2015-04-01 0:37 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Jeff Layton @ 2015-04-01 0:37 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-03-27 5:27 [PATCH 0/4] Fix various coverity warnings in fs/cifs Steve French 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French @ 2015-03-27 5:28 ` Steve French 2015-03-28 14:27 ` Shirish Pargaonkar ` (2 more replies) 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French 3 siblings, 3 replies; 19+ messages in thread From: Steve French @ 2015-03-27 5:28 UTC (permalink / raw) To: linux-cifs, linux-kernel; +Cc: smfrench 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French @ 2015-03-28 14:27 ` Shirish Pargaonkar 2015-03-30 9:50 ` Sachin Prabhu 2015-04-01 0:46 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Shirish Pargaonkar @ 2015-03-28 14:27 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, LKML 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French 2015-03-28 14:27 ` Shirish Pargaonkar @ 2015-03-30 9:50 ` Sachin Prabhu 2015-04-01 0:46 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Sachin Prabhu @ 2015-03-30 9:50 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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; > } > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French 2015-03-28 14:27 ` Shirish Pargaonkar 2015-03-30 9:50 ` Sachin Prabhu @ 2015-04-01 0:46 ` Jeff Layton 2015-04-01 5:00 ` Steve French 2 siblings, 1 reply; 19+ messages in thread From: Jeff Layton @ 2015-04-01 0:46 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-04-01 0:46 ` Jeff Layton @ 2015-04-01 5:00 ` Steve French 2015-04-01 11:07 ` Jeff Layton 0 siblings, 1 reply; 19+ messages in thread From: Steve French @ 2015-04-01 5:00 UTC (permalink / raw) To: Jeff Layton; +Cc: linux-cifs, LKML 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning 2015-04-01 5:00 ` Steve French @ 2015-04-01 11:07 ` Jeff Layton 0 siblings, 0 replies; 19+ messages in thread From: Jeff Layton @ 2015-04-01 11:07 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, LKML 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/4] [SMB3] Fix coverity warning 2015-03-27 5:27 [PATCH 0/4] Fix various coverity warnings in fs/cifs Steve French ` (2 preceding siblings ...) 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French @ 2015-03-27 5:28 ` Steve French 2015-03-28 15:05 ` Shirish Pargaonkar ` (2 more replies) 3 siblings, 3 replies; 19+ messages in thread From: Steve French @ 2015-03-27 5:28 UTC (permalink / raw) To: linux-cifs, linux-kernel; +Cc: smfrench 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] [SMB3] Fix coverity warning 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French @ 2015-03-28 15:05 ` Shirish Pargaonkar 2015-03-30 9:51 ` Sachin Prabhu 2015-04-01 0:38 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Shirish Pargaonkar @ 2015-03-28 15:05 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, LKML 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] [SMB3] Fix coverity warning 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French 2015-03-28 15:05 ` Shirish Pargaonkar @ 2015-03-30 9:51 ` Sachin Prabhu 2015-04-01 0:38 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Sachin Prabhu @ 2015-03-30 9:51 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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; > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/4] [SMB3] Fix coverity warning 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French 2015-03-28 15:05 ` Shirish Pargaonkar 2015-03-30 9:51 ` Sachin Prabhu @ 2015-04-01 0:38 ` Jeff Layton 2 siblings, 0 replies; 19+ messages in thread From: Jeff Layton @ 2015-04-01 0:38 UTC (permalink / raw) To: Steve French; +Cc: linux-cifs, linux-kernel 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> ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2015-04-01 11:07 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-03-27 5:27 [PATCH 0/4] Fix various coverity warnings in fs/cifs Steve French 2015-03-27 5:27 ` [PATCH 1/4] [SMB3] Fix warning on uninitialized buftype Steve French 2015-03-28 14:19 ` Shirish Pargaonkar 2015-03-30 9:36 ` Sachin Prabhu 2015-04-01 0:35 ` Jeff Layton 2015-03-27 5:28 ` [PATCH 2/4] [CIFS] Don't ignore errors on encrypting password in SMBTcon Steve French 2015-03-28 14:26 ` Shirish Pargaonkar 2015-03-30 9:44 ` Sachin Prabhu 2015-04-01 0:37 ` Jeff Layton 2015-03-27 5:28 ` [PATCH 3/4] [SMB3] Fix dereference before null check warning Steve French 2015-03-28 14:27 ` Shirish Pargaonkar 2015-03-30 9:50 ` Sachin Prabhu 2015-04-01 0:46 ` Jeff Layton 2015-04-01 5:00 ` Steve French 2015-04-01 11:07 ` Jeff Layton 2015-03-27 5:28 ` [PATCH 4/4] [SMB3] Fix coverity warning Steve French 2015-03-28 15:05 ` Shirish Pargaonkar 2015-03-30 9:51 ` Sachin Prabhu 2015-04-01 0:38 ` Jeff Layton
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).