Re: fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)

From: Luís Henriques
Date: Fri Sep 29 2023 - 05:06:19 EST

Next message: syzbot: "[syzbot] Monthly dccp report (Sep 2023)"
Previous message: Mike Rapoport: "Re: [PATCH] memblock: don't run loop in memblock_add_range() twice"
In reply to: Dan Carpenter: "fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Dan,

Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxx/config)
> compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
> reproduce: (https://download.01.org/0day-ci/archive/20230928/202309282202.xZxGdvS3-lkp@xxxxxxxxx/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@xxxxxxxxx>
> | Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> | Closes: https://lore.kernel.org/r/202309282202.xZxGdvS3-lkp@xxxxxxxxx/
>
> 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 message: syzbot: "[syzbot] Monthly dccp report (Sep 2023)"
Previous message: Mike Rapoport: "Re: [PATCH] memblock: don't run loop in memblock_add_range() twice"
In reply to: Dan Carpenter: "fs/ceph/crypto.c:465 ceph_fname_to_usr() warn: variable dereferenced before IS_ERR check 'dir' (see line 403)"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]