| Message ID | CAH2r5mtHpjjVVKFYKEkAZHG5U=-_umHwMhF2KDJjDSNgoa=Fxw@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cifs: fix missing null session check in mount | expand |
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?
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
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
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
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
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(-) kfree(ref_path);