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
