Re: cifs: Avoid doing network I/O while holding cache lock

[Steve French]

Added the one line patch from Paulo to address what Colin reported and
added a "Reported-by" for Colin mentioning Coverity.  merged into
cifs-2.6.git for-next

On Thu, Jan 16, 2020 at 1:10 PM Colin Ian King <colin.king@xxxxxxxxxxxxx> wrote:
>
> Hi,
>
> static analysis with Coverity has detected an issue with the following
> commit:
>
> commit 03535b72873ba74e80e6938b5f772cf5b07ca76b
> Author: Paulo Alcantara (SUSE) <pc@xxxxxx>
> Date:   Wed Dec 4 17:38:03 2019 -0300
>
>     cifs: Avoid doing network I/O while holding cache lock
>
>
>
> This commit removed a memset on the vol object, causing the issue as
> reported below by Coverity:
>
> 1342static struct cifs_ses *find_root_ses(struct vol_info *vi,
> 1343                                      struct cifs_tcon *tcon,
> 1344                                      const char *path)
> 1345{
> 1346        char *rpath;
> 1347        int rc;
> 1348        struct cache_entry *ce;
> 1349        struct dfs_info3_param ref = {0};
> 1350        char *mdata = NULL, *devname = NULL;
> 1351        struct TCP_Server_Info *server;
> 1352        struct cifs_ses *ses;
>
>     1. var_decl: Declaring variable vol without initializer.
> 1353        struct smb_vol vol;
> 1354
> 1355        rpath = get_dfs_root(path);
>
>     2. Condition IS_ERR(rpath), taking false branch.
> 1356        if (IS_ERR(rpath))
> 1357                return ERR_CAST(rpath);
> 1358
> 1359        down_read(&htable_rw_lock);
> 1360
> 1361        ce = lookup_cache_entry(rpath, NULL);
>
>     3. Condition IS_ERR(ce), taking true branch.
> 1362        if (IS_ERR(ce)) {
> 1363                up_read(&htable_rw_lock);
> 1364                ses = ERR_CAST(ce);
>
>     4. Jumping to label out.
> 1365                goto out;
> 1366        }
> 1367
> 1368        rc = setup_referral(path, ce, &ref, get_tgt_name(ce));
> 1369        if (rc) {
> 1370                up_read(&htable_rw_lock);
> 1371                ses = ERR_PTR(rc);
> 1372                goto out;
> 1373        }
> 1374
> 1375        up_read(&htable_rw_lock);
> 1376
> 1377        mdata = cifs_compose_mount_options(vi->mntdata, rpath, &ref,
> 1378                                           &devname);
> 1379        free_dfs_info_param(&ref);
> 1380
> 1381        if (IS_ERR(mdata)) {
> 1382                ses = ERR_CAST(mdata);
> 1383                mdata = NULL;
> 1384                goto out;
> 1385        }
> 1386
> 1387        rc = cifs_setup_volume_info(&vol, mdata, devname, false);
> 1388        kfree(devname);
> 1389
> 1390        if (rc) {
> 1391                ses = ERR_PTR(rc);
> 1392                goto out;
> 1393        }
> 1394
> 1395        server = get_tcp_server(&vol);
> 1396        if (!server) {
> 1397                ses = ERR_PTR(-EHOSTDOWN);
> 1398                goto out;
> 1399        }
> 1400
> 1401        ses = cifs_get_smb_ses(server, &vol);
> 1402
> 1403out:
>
>     5. uninit_use_in_call: Using uninitialized value vol.username when
> calling cifs_cleanup_volume_info_contents.
>
>     6. uninit_use_in_call: Using uninitialized value vol.password when
> calling cifs_cleanup_volume_info_contents.
>
>     7. uninit_use_in_call: Using uninitialized value vol.domainname when
> calling cifs_cleanup_volume_info_contents.
>
>     8. uninit_use_in_call: Using uninitialized value vol.UNC when
> calling cifs_cleanup_volume_info_contents.
>
>     9. uninit_use_in_call: Using uninitialized value vol.iocharset when
> calling cifs_cleanup_volume_info_contents.
>
>  Uninitialized pointer read
>    10. uninit_use_in_call: Using uninitialized value vol.prepath when
> calling cifs_cleanup_volume_info_contents.
>
> 1404        cifs_cleanup_volume_info_contents(&vol);
> 1405        kfree(mdata);
> 1406        kfree(rpath);
>
> The vol object is memset as a result of calling cifs_setup_volume_info()
> but this is too late for the earlier jumps to the error cleanup path at
> label out.  I did think about putting this back but it adds an
> unnecessary memset, a better fix may be return immediately or to exit
> via the kfree(mdata) at the end of the function.
>
> Not sure what is best, so I'm flagging this up as an issue that needs
> fixing.
>
> Colin



-- 
Thanks,

Steve