* fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
@ 2023-09-28 15:12 Dan Carpenter
2023-09-29 9:06 ` Luís Henriques
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-09-28 15:12 UTC (permalink / raw)
To: oe-kbuild, Luís Henriques
Cc: lkp, oe-kbuild-all, linux-kernel, Ilya Dryomov, Jeff Layton,
Xiubo Li, Milind Changire
tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
head: 633b47cb009d09dc8f4ba9cdb3a0ca138809c7c7
commit: dd66df0053ef84add5e684df517aa9b498342381 ceph: add support for encrypted snapshot names
config: x86_64-randconfig-161-20230928 (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202309282202.xZxGdvS3-lkp@intel.com/
smatch warnings:
fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
vim +/dir +465 fs/ceph/crypto.c
457117f077c674 Jeff Layton 2021-03-26 380 int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
457117f077c674 Jeff Layton 2021-03-26 381 struct fscrypt_str *oname, bool *is_nokey)
457117f077c674 Jeff Layton 2021-03-26 382 {
dd66df0053ef84 Luís Henriques 2022-08-25 383 struct inode *dir = fname->dir;
457117f077c674 Jeff Layton 2021-03-26 384 struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
457117f077c674 Jeff Layton 2021-03-26 385 struct fscrypt_str iname;
dd66df0053ef84 Luís Henriques 2022-08-25 386 char *name = fname->name;
dd66df0053ef84 Luís Henriques 2022-08-25 387 int name_len = fname->name_len;
dd66df0053ef84 Luís Henriques 2022-08-25 388 int ret;
457117f077c674 Jeff Layton 2021-03-26 389
457117f077c674 Jeff Layton 2021-03-26 390 /* Sanity check that the resulting name will fit in the buffer */
457117f077c674 Jeff Layton 2021-03-26 391 if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
457117f077c674 Jeff Layton 2021-03-26 392 return -EIO;
457117f077c674 Jeff Layton 2021-03-26 393
dd66df0053ef84 Luís Henriques 2022-08-25 394 /* Handle the special case of snapshot names that start with '_' */
dd66df0053ef84 Luís Henriques 2022-08-25 395 if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
dd66df0053ef84 Luís Henriques 2022-08-25 396 (name[0] == '_')) {
dd66df0053ef84 Luís Henriques 2022-08-25 397 dir = parse_longname(dir, name, &name_len);
dd66df0053ef84 Luís Henriques 2022-08-25 398 if (IS_ERR(dir))
dd66df0053ef84 Luís Henriques 2022-08-25 399 return PTR_ERR(dir);
If dir is an error pointer, then we return directly.
dd66df0053ef84 Luís Henriques 2022-08-25 400 name++; /* skip initial '_' */
dd66df0053ef84 Luís Henriques 2022-08-25 401 }
dd66df0053ef84 Luís Henriques 2022-08-25 402
dd66df0053ef84 Luís Henriques 2022-08-25 @403 if (!IS_ENCRYPTED(dir)) {
dd66df0053ef84 Luís Henriques 2022-08-25 404 oname->name = fname->name;
dd66df0053ef84 Luís Henriques 2022-08-25 405 oname->len = fname->name_len;
dd66df0053ef84 Luís Henriques 2022-08-25 406 ret = 0;
dd66df0053ef84 Luís Henriques 2022-08-25 407 goto out_inode;
dd66df0053ef84 Luís Henriques 2022-08-25 408 }
dd66df0053ef84 Luís Henriques 2022-08-25 409
dd66df0053ef84 Luís Henriques 2022-08-25 410 ret = ceph_fscrypt_prepare_readdir(dir);
dd66df0053ef84 Luís Henriques 2022-08-25 411 if (ret)
dd66df0053ef84 Luís Henriques 2022-08-25 412 goto out_inode;
457117f077c674 Jeff Layton 2021-03-26 413
457117f077c674 Jeff Layton 2021-03-26 414 /*
457117f077c674 Jeff Layton 2021-03-26 415 * Use the raw dentry name as sent by the MDS instead of
457117f077c674 Jeff Layton 2021-03-26 416 * generating a nokey name via fscrypt.
457117f077c674 Jeff Layton 2021-03-26 417 */
dd66df0053ef84 Luís Henriques 2022-08-25 418 if (!fscrypt_has_encryption_key(dir)) {
af9ffa6df7e337 Xiubo Li 2022-03-14 419 if (fname->no_copy)
af9ffa6df7e337 Xiubo Li 2022-03-14 420 oname->name = fname->name;
af9ffa6df7e337 Xiubo Li 2022-03-14 421 else
457117f077c674 Jeff Layton 2021-03-26 422 memcpy(oname->name, fname->name, fname->name_len);
457117f077c674 Jeff Layton 2021-03-26 423 oname->len = fname->name_len;
457117f077c674 Jeff Layton 2021-03-26 424 if (is_nokey)
457117f077c674 Jeff Layton 2021-03-26 425 *is_nokey = true;
dd66df0053ef84 Luís Henriques 2022-08-25 426 ret = 0;
dd66df0053ef84 Luís Henriques 2022-08-25 427 goto out_inode;
457117f077c674 Jeff Layton 2021-03-26 428 }
457117f077c674 Jeff Layton 2021-03-26 429
457117f077c674 Jeff Layton 2021-03-26 430 if (fname->ctext_len == 0) {
457117f077c674 Jeff Layton 2021-03-26 431 int declen;
457117f077c674 Jeff Layton 2021-03-26 432
457117f077c674 Jeff Layton 2021-03-26 433 if (!tname) {
457117f077c674 Jeff Layton 2021-03-26 434 ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
457117f077c674 Jeff Layton 2021-03-26 435 if (ret)
dd66df0053ef84 Luís Henriques 2022-08-25 436 goto out_inode;
457117f077c674 Jeff Layton 2021-03-26 437 tname = &_tname;
457117f077c674 Jeff Layton 2021-03-26 438 }
457117f077c674 Jeff Layton 2021-03-26 439
dd66df0053ef84 Luís Henriques 2022-08-25 440 declen = ceph_base64_decode(name, name_len, tname->name);
457117f077c674 Jeff Layton 2021-03-26 441 if (declen <= 0) {
457117f077c674 Jeff Layton 2021-03-26 442 ret = -EIO;
457117f077c674 Jeff Layton 2021-03-26 443 goto out;
457117f077c674 Jeff Layton 2021-03-26 444 }
457117f077c674 Jeff Layton 2021-03-26 445 iname.name = tname->name;
457117f077c674 Jeff Layton 2021-03-26 446 iname.len = declen;
457117f077c674 Jeff Layton 2021-03-26 447 } else {
457117f077c674 Jeff Layton 2021-03-26 448 iname.name = fname->ctext;
457117f077c674 Jeff Layton 2021-03-26 449 iname.len = fname->ctext_len;
457117f077c674 Jeff Layton 2021-03-26 450 }
457117f077c674 Jeff Layton 2021-03-26 451
dd66df0053ef84 Luís Henriques 2022-08-25 452 ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
dd66df0053ef84 Luís Henriques 2022-08-25 453 if (!ret && (dir != fname->dir)) {
dd66df0053ef84 Luís Henriques 2022-08-25 454 char tmp_buf[CEPH_BASE64_CHARS(NAME_MAX)];
dd66df0053ef84 Luís Henriques 2022-08-25 455
dd66df0053ef84 Luís Henriques 2022-08-25 456 name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
dd66df0053ef84 Luís Henriques 2022-08-25 457 oname->len, oname->name, dir->i_ino);
dd66df0053ef84 Luís Henriques 2022-08-25 458 memcpy(oname->name, tmp_buf, name_len);
dd66df0053ef84 Luís Henriques 2022-08-25 459 oname->len = name_len;
dd66df0053ef84 Luís Henriques 2022-08-25 460 }
dd66df0053ef84 Luís Henriques 2022-08-25 461
457117f077c674 Jeff Layton 2021-03-26 462 out:
457117f077c674 Jeff Layton 2021-03-26 463 fscrypt_fname_free_buffer(&_tname);
dd66df0053ef84 Luís Henriques 2022-08-25 464 out_inode:
dd66df0053ef84 Luís Henriques 2022-08-25 @465 if ((dir != fname->dir) && !IS_ERR(dir)) {
^^^^^^^^^^^^
Checking a second time, is harmless but annoys static analysis. I think
if you have the cross function database then this warning is not
triggered because Smatch tries to not warn about unnecessary checks so
long as we are sure they are harmless. Without the cross function
database we know that "dir" isn't an error pointer but it might still
be an invalid pointer. I guess I could make this more strict to only
count dereferencing which are potentially error pointer dereferences
instead of just potentially invalid. With the cross function database
we know that parse_longname() either returns valid or error pointers.
dd66df0053ef84 Luís Henriques 2022-08-25 466 if ((dir->i_state & I_NEW))
dd66df0053ef84 Luís Henriques 2022-08-25 467 discard_new_inode(dir);
dd66df0053ef84 Luís Henriques 2022-08-25 468 else
dd66df0053ef84 Luís Henriques 2022-08-25 469 iput(dir);
dd66df0053ef84 Luís Henriques 2022-08-25 470 }
457117f077c674 Jeff Layton 2021-03-26 471 return ret;
457117f077c674 Jeff Layton 2021-03-26 472 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
2023-09-28 15:12 fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403) Dan Carpenter
@ 2023-09-29 9:06 ` Luís Henriques
0 siblings, 0 replies; 2+ messages in thread
From: Luís Henriques @ 2023-09-29 9:06 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, lkp, oe-kbuild-all, linux-kernel, Ilya Dryomov,
Jeff Layton, Xiubo Li, Milind Changire
Hi Dan,
Dan Carpenter <dan.carpenter@linaro.org> writes:
> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head: 633b47cb009d09dc8f4ba9cdb3a0ca138809c7c7
> commit: dd66df0053ef84add5e684df517aa9b498342381 ceph: add support for encrypted snapshot names
> config: x86_64-randconfig-161-20230928 (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@intel.com/reproduce)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202309282202.xZxGdvS3-lkp@intel.com/
>
> smatch warnings:
> fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
I agree that this check here is indeed useless and the IS_ERR() here
could/should be dropped. In fact, function ceph_encode_encrypted_dname()
has a similar structure and is not repeating this error check in the
clean-up code.
I'll send out a clean-up fix for this in a second (although I also don't
think there's a real bug here). Thank you for your report.
Cheers,
--
Luís
> vim +/dir +465 fs/ceph/crypto.c
>
> 457117f077c674 Jeff Layton 2021-03-26 380 int ceph_fname_to_usr(const struct ceph_fname *fname, struct fscrypt_str *tname,
> 457117f077c674 Jeff Layton 2021-03-26 381 struct fscrypt_str *oname, bool *is_nokey)
> 457117f077c674 Jeff Layton 2021-03-26 382 {
> dd66df0053ef84 Luís Henriques 2022-08-25 383 struct inode *dir = fname->dir;
> 457117f077c674 Jeff Layton 2021-03-26 384 struct fscrypt_str _tname = FSTR_INIT(NULL, 0);
> 457117f077c674 Jeff Layton 2021-03-26 385 struct fscrypt_str iname;
> dd66df0053ef84 Luís Henriques 2022-08-25 386 char *name = fname->name;
> dd66df0053ef84 Luís Henriques 2022-08-25 387 int name_len = fname->name_len;
> dd66df0053ef84 Luís Henriques 2022-08-25 388 int ret;
> 457117f077c674 Jeff Layton 2021-03-26 389
> 457117f077c674 Jeff Layton 2021-03-26 390 /* Sanity check that the resulting name will fit in the buffer */
> 457117f077c674 Jeff Layton 2021-03-26 391 if (fname->name_len > NAME_MAX || fname->ctext_len > NAME_MAX)
> 457117f077c674 Jeff Layton 2021-03-26 392 return -EIO;
> 457117f077c674 Jeff Layton 2021-03-26 393
> dd66df0053ef84 Luís Henriques 2022-08-25 394 /* Handle the special case of snapshot names that start with '_' */
> dd66df0053ef84 Luís Henriques 2022-08-25 395 if ((ceph_snap(dir) == CEPH_SNAPDIR) && (name_len > 0) &&
> dd66df0053ef84 Luís Henriques 2022-08-25 396 (name[0] == '_')) {
> dd66df0053ef84 Luís Henriques 2022-08-25 397 dir = parse_longname(dir, name, &name_len);
> dd66df0053ef84 Luís Henriques 2022-08-25 398 if (IS_ERR(dir))
> dd66df0053ef84 Luís Henriques 2022-08-25 399 return PTR_ERR(dir);
>
> If dir is an error pointer, then we return directly.
>
> dd66df0053ef84 Luís Henriques 2022-08-25 400 name++; /* skip initial '_' */
> dd66df0053ef84 Luís Henriques 2022-08-25 401 }
> dd66df0053ef84 Luís Henriques 2022-08-25 402
> dd66df0053ef84 Luís Henriques 2022-08-25 @403 if (!IS_ENCRYPTED(dir)) {
> dd66df0053ef84 Luís Henriques 2022-08-25 404 oname->name = fname->name;
> dd66df0053ef84 Luís Henriques 2022-08-25 405 oname->len = fname->name_len;
> dd66df0053ef84 Luís Henriques 2022-08-25 406 ret = 0;
> dd66df0053ef84 Luís Henriques 2022-08-25 407 goto out_inode;
> dd66df0053ef84 Luís Henriques 2022-08-25 408 }
> dd66df0053ef84 Luís Henriques 2022-08-25 409
> dd66df0053ef84 Luís Henriques 2022-08-25 410 ret = ceph_fscrypt_prepare_readdir(dir);
> dd66df0053ef84 Luís Henriques 2022-08-25 411 if (ret)
> dd66df0053ef84 Luís Henriques 2022-08-25 412 goto out_inode;
> 457117f077c674 Jeff Layton 2021-03-26 413
> 457117f077c674 Jeff Layton 2021-03-26 414 /*
> 457117f077c674 Jeff Layton 2021-03-26 415 * Use the raw dentry name as sent by the MDS instead of
> 457117f077c674 Jeff Layton 2021-03-26 416 * generating a nokey name via fscrypt.
> 457117f077c674 Jeff Layton 2021-03-26 417 */
> dd66df0053ef84 Luís Henriques 2022-08-25 418 if (!fscrypt_has_encryption_key(dir)) {
> af9ffa6df7e337 Xiubo Li 2022-03-14 419 if (fname->no_copy)
> af9ffa6df7e337 Xiubo Li 2022-03-14 420 oname->name = fname->name;
> af9ffa6df7e337 Xiubo Li 2022-03-14 421 else
> 457117f077c674 Jeff Layton 2021-03-26 422 memcpy(oname->name, fname->name, fname->name_len);
> 457117f077c674 Jeff Layton 2021-03-26 423 oname->len = fname->name_len;
> 457117f077c674 Jeff Layton 2021-03-26 424 if (is_nokey)
> 457117f077c674 Jeff Layton 2021-03-26 425 *is_nokey = true;
> dd66df0053ef84 Luís Henriques 2022-08-25 426 ret = 0;
> dd66df0053ef84 Luís Henriques 2022-08-25 427 goto out_inode;
> 457117f077c674 Jeff Layton 2021-03-26 428 }
> 457117f077c674 Jeff Layton 2021-03-26 429
> 457117f077c674 Jeff Layton 2021-03-26 430 if (fname->ctext_len == 0) {
> 457117f077c674 Jeff Layton 2021-03-26 431 int declen;
> 457117f077c674 Jeff Layton 2021-03-26 432
> 457117f077c674 Jeff Layton 2021-03-26 433 if (!tname) {
> 457117f077c674 Jeff Layton 2021-03-26 434 ret = fscrypt_fname_alloc_buffer(NAME_MAX, &_tname);
> 457117f077c674 Jeff Layton 2021-03-26 435 if (ret)
> dd66df0053ef84 Luís Henriques 2022-08-25 436 goto out_inode;
> 457117f077c674 Jeff Layton 2021-03-26 437 tname = &_tname;
> 457117f077c674 Jeff Layton 2021-03-26 438 }
> 457117f077c674 Jeff Layton 2021-03-26 439
> dd66df0053ef84 Luís Henriques 2022-08-25 440 declen = ceph_base64_decode(name, name_len, tname->name);
> 457117f077c674 Jeff Layton 2021-03-26 441 if (declen <= 0) {
> 457117f077c674 Jeff Layton 2021-03-26 442 ret = -EIO;
> 457117f077c674 Jeff Layton 2021-03-26 443 goto out;
> 457117f077c674 Jeff Layton 2021-03-26 444 }
> 457117f077c674 Jeff Layton 2021-03-26 445 iname.name = tname->name;
> 457117f077c674 Jeff Layton 2021-03-26 446 iname.len = declen;
> 457117f077c674 Jeff Layton 2021-03-26 447 } else {
> 457117f077c674 Jeff Layton 2021-03-26 448 iname.name = fname->ctext;
> 457117f077c674 Jeff Layton 2021-03-26 449 iname.len = fname->ctext_len;
> 457117f077c674 Jeff Layton 2021-03-26 450 }
> 457117f077c674 Jeff Layton 2021-03-26 451
> dd66df0053ef84 Luís Henriques 2022-08-25 452 ret = fscrypt_fname_disk_to_usr(dir, 0, 0, &iname, oname);
> dd66df0053ef84 Luís Henriques 2022-08-25 453 if (!ret && (dir != fname->dir)) {
> dd66df0053ef84 Luís Henriques 2022-08-25 454 char tmp_buf[CEPH_BASE64_CHARS(NAME_MAX)];
> dd66df0053ef84 Luís Henriques 2022-08-25 455
> dd66df0053ef84 Luís Henriques 2022-08-25 456 name_len = snprintf(tmp_buf, sizeof(tmp_buf), "_%.*s_%ld",
> dd66df0053ef84 Luís Henriques 2022-08-25 457 oname->len, oname->name, dir->i_ino);
> dd66df0053ef84 Luís Henriques 2022-08-25 458 memcpy(oname->name, tmp_buf, name_len);
> dd66df0053ef84 Luís Henriques 2022-08-25 459 oname->len = name_len;
> dd66df0053ef84 Luís Henriques 2022-08-25 460 }
> dd66df0053ef84 Luís Henriques 2022-08-25 461
> 457117f077c674 Jeff Layton 2021-03-26 462 out:
> 457117f077c674 Jeff Layton 2021-03-26 463 fscrypt_fname_free_buffer(&_tname);
> dd66df0053ef84 Luís Henriques 2022-08-25 464 out_inode:
> dd66df0053ef84 Luís Henriques 2022-08-25 @465 if ((dir != fname->dir) && !IS_ERR(dir)) {
> ^^^^^^^^^^^^
> Checking a second time, is harmless but annoys static analysis. I think
> if you have the cross function database then this warning is not
> triggered because Smatch tries to not warn about unnecessary checks so
> long as we are sure they are harmless. Without the cross function
> database we know that "dir" isn't an error pointer but it might still
> be an invalid pointer. I guess I could make this more strict to only
> count dereferencing which are potentially error pointer dereferences
> instead of just potentially invalid. With the cross function database
> we know that parse_longname() either returns valid or error pointers.
>
> dd66df0053ef84 Luís Henriques 2022-08-25 466 if ((dir->i_state & I_NEW))
> dd66df0053ef84 Luís Henriques 2022-08-25 467 discard_new_inode(dir);
> dd66df0053ef84 Luís Henriques 2022-08-25 468 else
> dd66df0053ef84 Luís Henriques 2022-08-25 469 iput(dir);
> dd66df0053ef84 Luís Henriques 2022-08-25 470 }
> 457117f077c674 Jeff Layton 2021-03-26 471 return ret;
> 457117f077c674 Jeff Layton 2021-03-26 472 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-09-29 9:06 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-28 15:12 fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403) Dan Carpenter
2023-09-29 9:06 ` Luís Henriques
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox