* [PATCH] cifs: fix missing null session check in mount
@ 2021-07-14 0:48 Steve French
2021-07-14 4:54 ` Shyam Prasad N
0 siblings, 1 reply; 5+ messages in thread
From: Steve French @ 2021-07-14 0:48 UTC (permalink / raw)
To: CIFS; +Cc: Paulo Alcantara
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
Although it is unlikely to be have ended up with a null
session pointer calling cifs_try_adding_channels in cifs_mount.
Coverity correctly notes that we are already checking for
it earlier (when we return from do_dfs_failover), so at
a minimum to clarify the code we should make sure we also
check for it when we exit the loop so we don't end up calling
cifs_try_adding_channels or mount_setup_tlink with a null
ses pointer.
Addresses-Coverity: 1505608 ("Derefernce after null check")
Reviewed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index db6c607269f5..463cae116c12 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb,
struct smb3_fs_context *ctx)
rc = -ELOOP;
} while (rc == -EREMOTE);
- if (rc || !tcon)
+ if (rc || !tcon || !ses)
goto error;
kfree(ref_path);
--
Thanks,
Steve
[-- Attachment #2: 0001-cifs-fix-missing-null-session-check-in-mount.patch --]
[-- Type: text/x-patch, Size: 1258 bytes --]
From 8b755ad38c72d2dc39fd6e9110d03aa132498571 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Tue, 13 Jul 2021 19:40:33 -0500
Subject: [PATCH] cifs: fix missing null session check in mount
Although it is unlikely to be have ended up with a null
session pointer calling cifs_try_adding_channels in cifs_mount.
Coverity correctly notes that we are already checking for
it earlier (when we return from do_dfs_failover), so at
a minimum to clarify the code we should make sure we also
check for it when we exit the loop so we don't end up calling
cifs_try_adding_channels or mount_setup_tlink with a null
ses pointer.
Addresses-Coverity: 1505608 ("Derefernce after null check")
Reviewed-by: Paulo Alcantara (SUSE) <pc@cjr.nz>
Signed-off-by: Steve French <stfrench@microsoft.com>
---
fs/cifs/connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index db6c607269f5..463cae116c12 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, struct smb3_fs_context *ctx)
rc = -ELOOP;
} while (rc == -EREMOTE);
- if (rc || !tcon)
+ if (rc || !tcon || !ses)
goto error;
kfree(ref_path);
--
2.30.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH] cifs: fix missing null session check in mount 2021-07-14 0:48 [PATCH] cifs: fix missing null session check in mount Steve French @ 2021-07-14 4:54 ` Shyam Prasad N 2021-07-14 5:02 ` Steve French 2021-07-14 5:29 ` ronnie sahlberg 0 siblings, 2 replies; 5+ messages in thread From: Shyam Prasad N @ 2021-07-14 4:54 UTC (permalink / raw) To: Steve French; +Cc: CIFS, Paulo Alcantara On Wed, Jul 14, 2021 at 6:19 AM Steve French <smfrench@gmail.com> wrote: > > Although it is unlikely to be have ended up with a null > session pointer calling cifs_try_adding_channels in cifs_mount. > Coverity correctly notes that we are already checking for > it earlier (when we return from do_dfs_failover), so at > a minimum to clarify the code we should make sure we also > check for it when we exit the loop so we don't end up calling > cifs_try_adding_channels or mount_setup_tlink with a null > ses pointer. > > Addresses-Coverity: 1505608 ("Derefernce after null check") > Reviewed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > Signed-off-by: Steve French <stfrench@microsoft.com> > --- > fs/cifs/connect.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > index db6c607269f5..463cae116c12 100644 > --- a/fs/cifs/connect.c > +++ b/fs/cifs/connect.c > @@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, > struct smb3_fs_context *ctx) > rc = -ELOOP; > } while (rc == -EREMOTE); > > - if (rc || !tcon) > + if (rc || !tcon || !ses) > goto error; > > kfree(ref_path); > > -- > Thanks, > > Steve Hi Paulo, Doesn't it make sense to check rc, tcon and ses values right after mount_get_conns call? rc = mount_get_conns(ctx, cifs_sb, &xid, &server, &ses, &tcon); <<<<<<<<<<<<<<<<<<< /* * If called with 'nodfs' mount option, then skip DFS resolving. Otherwise unconditionally * try to get an DFS referral (even cached) to determine whether it is an DFS mount. * * Skip prefix path to provide support for DFS referrals from w2k8 servers which don't seem * to respond with PATH_NOT_COVERED to requests that include the prefix. */ if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS) || dfs_cache_find(xid, ses, cifs_sb->local_nls, cifs_remap(cifs_sb), ctx->UNC + 1, NULL, NULL)) { if (rc) goto error; /* Check if it is fully accessible and then mount it */ rc = is_path_remote(cifs_sb, ctx, xid, server, tcon); if (!rc) goto out; if (rc != -EREMOTE) goto error; } Why don't we check for all rc values that we don't expect, and call dfs_cache_find only when it's an expected error? -- Regards, Shyam ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix missing null session check in mount 2021-07-14 4:54 ` Shyam Prasad N @ 2021-07-14 5:02 ` Steve French 2021-07-14 6:37 ` Shyam Prasad N 2021-07-14 5:29 ` ronnie sahlberg 1 sibling, 1 reply; 5+ messages in thread From: Steve French @ 2021-07-14 5:02 UTC (permalink / raw) To: Shyam Prasad N; +Cc: CIFS, Paulo Alcantara But isn't it the return from do_dfs_failover the issue ... we checked right before we called it on line 3561 ... but if it fails to populate ses ... then we break out of the loop - we could change it to "goto error" but the change in the patch is a little broader (wait until we exit the while loop) On Tue, Jul 13, 2021 at 11:54 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Wed, Jul 14, 2021 at 6:19 AM Steve French <smfrench@gmail.com> wrote: > > > > Although it is unlikely to be have ended up with a null > > session pointer calling cifs_try_adding_channels in cifs_mount. > > Coverity correctly notes that we are already checking for > > it earlier (when we return from do_dfs_failover), so at > > a minimum to clarify the code we should make sure we also > > check for it when we exit the loop so we don't end up calling > > cifs_try_adding_channels or mount_setup_tlink with a null > > ses pointer. > > > > Addresses-Coverity: 1505608 ("Derefernce after null check") > > Reviewed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > Signed-off-by: Steve French <stfrench@microsoft.com> > > --- > > fs/cifs/connect.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index db6c607269f5..463cae116c12 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, > > struct smb3_fs_context *ctx) > > rc = -ELOOP; > > } while (rc == -EREMOTE); > > > > - if (rc || !tcon) > > + if (rc || !tcon || !ses) > > goto error; > > > > kfree(ref_path); > > > > -- > > Thanks, > > > > Steve > > Hi Paulo, > > Doesn't it make sense to check rc, tcon and ses values right after > mount_get_conns call? > > rc = mount_get_conns(ctx, cifs_sb, &xid, &server, &ses, > &tcon); <<<<<<<<<<<<<<<<<<< > /* > * If called with 'nodfs' mount option, then skip DFS > resolving. Otherwise unconditionally > * try to get an DFS referral (even cached) to determine > whether it is an DFS mount. > * > * Skip prefix path to provide support for DFS referrals from > w2k8 servers which don't seem > * to respond with PATH_NOT_COVERED to requests that include the prefix. > */ > if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS) || > dfs_cache_find(xid, ses, cifs_sb->local_nls, > cifs_remap(cifs_sb), ctx->UNC + 1, NULL, > NULL)) { > if (rc) > goto error; > /* Check if it is fully accessible and then mount it */ > rc = is_path_remote(cifs_sb, ctx, xid, server, tcon); > if (!rc) > goto out; > if (rc != -EREMOTE) > goto error; > } > > Why don't we check for all rc values that we don't expect, and call > dfs_cache_find only when it's an expected error? > > -- > Regards, > Shyam -- Thanks, Steve ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix missing null session check in mount 2021-07-14 5:02 ` Steve French @ 2021-07-14 6:37 ` Shyam Prasad N 0 siblings, 0 replies; 5+ messages in thread From: Shyam Prasad N @ 2021-07-14 6:37 UTC (permalink / raw) To: Steve French; +Cc: CIFS, Paulo Alcantara On Wed, Jul 14, 2021 at 10:32 AM Steve French <smfrench@gmail.com> wrote: > > But isn't it the return from do_dfs_failover the issue ... we checked > right before we called it on line 3561 ... but if it fails to populate > ses ... then we break out of the loop - we could change it to "goto > error" but the change in the patch is a little broader (wait until we > exit the while loop) > Yes. That too. We need to check return value from mount_get_conns and do_dfs_failover. As Ronnie suggested, we need to make sure that when these functions return error, none of the pointers get allocated. > On Tue, Jul 13, 2021 at 11:54 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > > > On Wed, Jul 14, 2021 at 6:19 AM Steve French <smfrench@gmail.com> wrote: > > > > > > Although it is unlikely to be have ended up with a null > > > session pointer calling cifs_try_adding_channels in cifs_mount. > > > Coverity correctly notes that we are already checking for > > > it earlier (when we return from do_dfs_failover), so at > > > a minimum to clarify the code we should make sure we also > > > check for it when we exit the loop so we don't end up calling > > > cifs_try_adding_channels or mount_setup_tlink with a null > > > ses pointer. > > > > > > Addresses-Coverity: 1505608 ("Derefernce after null check") > > > Reviewed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > > Signed-off-by: Steve French <stfrench@microsoft.com> > > > --- > > > fs/cifs/connect.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > > index db6c607269f5..463cae116c12 100644 > > > --- a/fs/cifs/connect.c > > > +++ b/fs/cifs/connect.c > > > @@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, > > > struct smb3_fs_context *ctx) > > > rc = -ELOOP; > > > } while (rc == -EREMOTE); > > > > > > - if (rc || !tcon) > > > + if (rc || !tcon || !ses) > > > goto error; > > > > > > kfree(ref_path); > > > > > > -- > > > Thanks, > > > > > > Steve > > > > Hi Paulo, > > > > Doesn't it make sense to check rc, tcon and ses values right after > > mount_get_conns call? > > > > rc = mount_get_conns(ctx, cifs_sb, &xid, &server, &ses, > > &tcon); <<<<<<<<<<<<<<<<<<< > > /* > > * If called with 'nodfs' mount option, then skip DFS > > resolving. Otherwise unconditionally > > * try to get an DFS referral (even cached) to determine > > whether it is an DFS mount. > > * > > * Skip prefix path to provide support for DFS referrals from > > w2k8 servers which don't seem > > * to respond with PATH_NOT_COVERED to requests that include the prefix. > > */ > > if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS) || > > dfs_cache_find(xid, ses, cifs_sb->local_nls, > > cifs_remap(cifs_sb), ctx->UNC + 1, NULL, > > NULL)) { > > if (rc) > > goto error; > > /* Check if it is fully accessible and then mount it */ > > rc = is_path_remote(cifs_sb, ctx, xid, server, tcon); > > if (!rc) > > goto out; > > if (rc != -EREMOTE) > > goto error; > > } > > > > Why don't we check for all rc values that we don't expect, and call > > dfs_cache_find only when it's an expected error? > > > > -- > > Regards, > > Shyam > > > > -- > Thanks, > > Steve -- Regards, Shyam ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] cifs: fix missing null session check in mount 2021-07-14 4:54 ` Shyam Prasad N 2021-07-14 5:02 ` Steve French @ 2021-07-14 5:29 ` ronnie sahlberg 1 sibling, 0 replies; 5+ messages in thread From: ronnie sahlberg @ 2021-07-14 5:29 UTC (permalink / raw) To: Shyam Prasad N; +Cc: Steve French, CIFS, Paulo Alcantara On Wed, Jul 14, 2021 at 2:55 PM Shyam Prasad N <nspmangalore@gmail.com> wrote: > > On Wed, Jul 14, 2021 at 6:19 AM Steve French <smfrench@gmail.com> wrote: > > > > Although it is unlikely to be have ended up with a null > > session pointer calling cifs_try_adding_channels in cifs_mount. > > Coverity correctly notes that we are already checking for > > it earlier (when we return from do_dfs_failover), so at > > a minimum to clarify the code we should make sure we also > > check for it when we exit the loop so we don't end up calling > > cifs_try_adding_channels or mount_setup_tlink with a null > > ses pointer. > > > > Addresses-Coverity: 1505608 ("Derefernce after null check") > > Reviewed-off-by: Paulo Alcantara (SUSE) <pc@cjr.nz> > > Signed-off-by: Steve French <stfrench@microsoft.com> > > --- > > fs/cifs/connect.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index db6c607269f5..463cae116c12 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -3577,7 +3577,7 @@ int cifs_mount(struct cifs_sb_info *cifs_sb, > > struct smb3_fs_context *ctx) > > rc = -ELOOP; > > } while (rc == -EREMOTE); > > > > - if (rc || !tcon) > > + if (rc || !tcon || !ses) > > goto error; > > > > kfree(ref_path); > > > > -- > > Thanks, > > > > Steve > > Hi Paulo, > > Doesn't it make sense to check rc, tcon and ses values right after > mount_get_conns call? Or check rc, but to make mount_get_conns an easy to use api, maybe make sure and specify that IF it returns error then the output arguments are guaranteed to be NULL. > > rc = mount_get_conns(ctx, cifs_sb, &xid, &server, &ses, > &tcon); <<<<<<<<<<<<<<<<<<< > /* > * If called with 'nodfs' mount option, then skip DFS > resolving. Otherwise unconditionally > * try to get an DFS referral (even cached) to determine > whether it is an DFS mount. > * > * Skip prefix path to provide support for DFS referrals from > w2k8 servers which don't seem > * to respond with PATH_NOT_COVERED to requests that include the prefix. > */ > if ((cifs_sb->mnt_cifs_flags & CIFS_MOUNT_NO_DFS) || > dfs_cache_find(xid, ses, cifs_sb->local_nls, > cifs_remap(cifs_sb), ctx->UNC + 1, NULL, > NULL)) { > if (rc) > goto error; > /* Check if it is fully accessible and then mount it */ > rc = is_path_remote(cifs_sb, ctx, xid, server, tcon); > if (!rc) > goto out; > if (rc != -EREMOTE) > goto error; > } > > Why don't we check for all rc values that we don't expect, and call > dfs_cache_find only when it's an expected error? > > -- > Regards, > Shyam ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-07-14 6:38 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-14 0:48 [PATCH] cifs: fix missing null session check in mount Steve French 2021-07-14 4:54 ` Shyam Prasad N 2021-07-14 5:02 ` Steve French 2021-07-14 6:37 ` Shyam Prasad N 2021-07-14 5:29 ` ronnie sahlberg
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).