Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning
On Wed, 1 Apr 2015 00:00:57 -0500
Steve French wrote:
> On Tue, Mar 31, 2015 at 7:46 PM, Jeff Layton wrote:
> > On Fri, 27 Mar 2015 00:28:01 -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
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning
On Tue, Mar 31, 2015 at 7:46 PM, Jeff Layton wrote:
> On Fri, 27 Mar 2015 00:28:01 -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
>
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning
On Fri, 27 Mar 2015 00:28:01 -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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning
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
ACKed-by: Sachin Prabhu
> ---
> 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;
> }
> }
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 3/4] [SMB3] Fix dereference before null check warning
Looks correct.
Acked-by: Shirish Pargaonkar
On Fri, Mar 27, 2015 at 12:28 AM, 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
> ---
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH 3/4] [SMB3] Fix dereference before null check warning
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
---
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-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
