linux-kernel - re: cifs: Avoid doing network I/O while holding cache lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 16 Jan 2020 19:10:07 +0000
From:   Colin Ian King <colin.king@...onical.com>
To:     "Paulo Alcantara (SUSE)" <pc@....nz>,
        Steve French <sfrench@...ba.org>, linux-cifs@...r.kernel.org,
        samba-technical@...ts.samba.org
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: re: cifs: Avoid doing network I/O while holding cache lock

Hi,

static analysis with Coverity has detected an issue with the following
commit:

commit 03535b72873ba74e80e6938b5f772cf5b07ca76b
Author: Paulo Alcantara (SUSE) <pc@....nz>
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