Linux-CIFS Archive on lore.kernel.org
 help / color / Atom feed
* re: cifs: Avoid doing network I/O while holding cache lock
@ 2020-01-16 19:10 Colin Ian King
  2020-01-16 22:06 ` Steve French
  0 siblings, 1 reply; 2+ messages in thread
From: Colin Ian King @ 2020-01-16 19:10 UTC (permalink / raw)
  To: Paulo Alcantara (SUSE), Steve French, linux-cifs, samba-technical
  Cc: linux-kernel

Hi,

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

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

^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: cifs: Avoid doing network I/O while holding cache lock
  2020-01-16 19:10 cifs: Avoid doing network I/O while holding cache lock Colin Ian King
@ 2020-01-16 22:06 ` Steve French
  0 siblings, 0 replies; 2+ messages in thread
From: Steve French @ 2020-01-16 22:06 UTC (permalink / raw)
  To: Colin Ian King
  Cc: Paulo Alcantara (SUSE),
	Steve French, CIFS, samba-technical, linux-kernel

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@canonical.com> wrote:
>
> Hi,
>
> static analysis with Coverity has detected an issue with the following
> commit:
>
> commit 03535b72873ba74e80e6938b5f772cf5b07ca76b
> Author: Paulo Alcantara (SUSE) <pc@cjr.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



-- 
Thanks,

Steve

^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, back to index

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-16 19:10 cifs: Avoid doing network I/O while holding cache lock Colin Ian King
2020-01-16 22:06 ` Steve French

Linux-CIFS Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-cifs/0 linux-cifs/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-cifs linux-cifs/ https://lore.kernel.org/linux-cifs \
		linux-cifs@vger.kernel.org
	public-inbox-index linux-cifs

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-cifs


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git