From: "Luís Henriques" <lhenriques@suse.de>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: oe-kbuild@lists.linux.dev, lkp@intel.com,
oe-kbuild-all@lists.linux.dev, linux-kernel@vger.kernel.org,
Ilya Dryomov <idryomov@gmail.com>,
Jeff Layton <jlayton@kernel.org>, Xiubo Li <xiubli@redhat.com>,
Milind Changire <mchangir@redhat.com>
Subject: Re: fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)
Date: Fri, 29 Sep 2023 10:06:09 +0100 [thread overview]
Message-ID: <87ttrdz6v2.fsf@suse.de> (raw)
In-Reply-To: <b5a0acdb-9c25-46f3-aa44-ac57da8efeee@kadam.mountain> (Dan Carpenter's message of "Thu, 28 Sep 2023 18:12:27 +0300")
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
>
next prev parent reply other threads:[~2023-09-29 9:06 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
-- strict thread matches above, loose matches on Subject: below --
2023-09-28 14:54 kernel test robot
2023-09-09 4:36 kernel test robot
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87ttrdz6v2.fsf@suse.de \
--to=lhenriques@suse.de \
--cc=dan.carpenter@linaro.org \
--cc=idryomov@gmail.com \
--cc=jlayton@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=lkp@intel.com \
--cc=mchangir@redhat.com \
--cc=oe-kbuild-all@lists.linux.dev \
--cc=oe-kbuild@lists.linux.dev \
--cc=xiubli@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.