* Re: [PATCH v1 06/11] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER
2022-02-22 3:16 ` [PATCH v1 06/11] landlock: Add support for file reparenting with LANDLOCK_ACCESS_FS_REFER kernel test robot
@ 2022-02-22 10:18 ` Mickaël Salaün
0 siblings, 0 replies; 2+ messages in thread
From: Mickaël Salaün @ 2022-02-22 10:18 UTC (permalink / raw)
To: kernel test robot, James Morris, Serge E . Hallyn
Cc: llvm, kbuild-all, Al Viro, Jann Horn, Kees Cook,
Konstantin Meskhidze, Paul Moore, Shuah Khan, linux-doc,
linux-fsdevel, linux-kernel, linux-security-module
This error is because clang does not behave like GCC:
check_access_path_dual() should be marked as __always_inline, or I
should change from BUILD_BUG_ON() to WARN_ON_ONCE() if needed. I'll fix
that in the next series.
On 22/02/2022 04:16, kernel test robot wrote:
> Hi "Mickaël,
>
> I love your patch! Yet something to improve:
>
> [auto build test ERROR on cfb92440ee71adcc2105b0890bb01ac3cddb8507]
>
> url: https://github.com/0day-ci/linux/commits/Micka-l-Sala-n/Landlock-file-linking-and-renaming-support/20220222-051842
> base: cfb92440ee71adcc2105b0890bb01ac3cddb8507
> config: hexagon-randconfig-r002-20220221 (https://download.01.org/0day-ci/archive/20220222/202202221149.qLO9DEqo-lkp@intel.com/config)
> compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project d271fc04d5b97b12e6b797c6067d3c96a8d7470e)
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/0day-ci/linux/commit/c68b879f54d6262963d435a18cedbc238b7faeaf
> git remote add linux-review https://github.com/0day-ci/linux
> git fetch --no-tags linux-review Micka-l-Sala-n/Landlock-file-linking-and-renaming-support/20220222-051842
> git checkout c68b879f54d6262963d435a18cedbc238b7faeaf
> # save the config file to linux build tree
> mkdir build_dir
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
>>> security/landlock/fs.c:463:2: error: call to __compiletime_assert_228 declared with 'error' attribute: BUILD_BUG_ON failed: !layer_masks_dst_parent
> BUILD_BUG_ON(!layer_masks_dst_parent);
> ^
> include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
> BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> ^
> include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^
> include/linux/compiler_types.h:346:2: note: expanded from macro 'compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> ^
> include/linux/compiler_types.h:334:2: note: expanded from macro '_compiletime_assert'
> __compiletime_assert(condition, msg, prefix, suffix)
> ^
> include/linux/compiler_types.h:327:4: note: expanded from macro '__compiletime_assert'
> prefix ## suffix(); \
> ^
> <scratch space>:170:1: note: expanded from here
> __compiletime_assert_228
> ^
>>> security/landlock/fs.c:670:2: error: call to __compiletime_assert_229 declared with 'error' attribute: BUILD_BUG_ON failed: !layer_masks_dom
> BUILD_BUG_ON(!layer_masks_dom);
> ^
> include/linux/build_bug.h:50:2: note: expanded from macro 'BUILD_BUG_ON'
> BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> ^
> include/linux/build_bug.h:39:37: note: expanded from macro 'BUILD_BUG_ON_MSG'
> #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> ^
> include/linux/compiler_types.h:346:2: note: expanded from macro 'compiletime_assert'
> _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> ^
> include/linux/compiler_types.h:334:2: note: expanded from macro '_compiletime_assert'
> __compiletime_assert(condition, msg, prefix, suffix)
> ^
> include/linux/compiler_types.h:327:4: note: expanded from macro '__compiletime_assert'
> prefix ## suffix(); \
> ^
> <scratch space>:174:1: note: expanded from here
> __compiletime_assert_229
> ^
> 2 errors generated.
>
>
> vim +/error +463 security/landlock/fs.c
>
> 401
> 402 /**
> 403 * check_access_path_dual - Check a source and a destination accesses
> 404 *
> 405 * @domain: Domain to check against.
> 406 * @path: File hierarchy to walk through.
> 407 * @child_is_directory: Must be set to true if the (original) leaf is a
> 408 * directory, false otherwise.
> 409 * @access_request_dst_parent: Accesses to check, once @layer_masks_dst_parent
> 410 * is equal to @layer_masks_src_parent (if any).
> 411 * @layer_masks_dst_parent: Pointer to a matrix of layer masks per access
> 412 * masks, identifying the layers that forbid a specific access. Bits from
> 413 * this matrix can be unset according to the @path walk. An empty matrix
> 414 * means that @domain allows all possible Landlock accesses (i.e. not only
> 415 * those identified by @access_request_dst_parent). This matrix can
> 416 * initially refer to domain layer masks and, when the accesses for the
> 417 * destination and source are the same, to request layer masks.
> 418 * @access_request_src_parent: Similar to @access_request_dst_parent but for an
> 419 * initial source path request. Only taken into account if
> 420 * @layer_masks_src_parent is not NULL.
> 421 * @layer_masks_src_parent: Similar to @layer_masks_dst_parent but for an
> 422 * initial source path walk. This can be NULL if only dealing with a
> 423 * destination access request (i.e. not a rename nor a link action).
> 424 * @layer_masks_child: Similar to @layer_masks_src_parent but only for the
> 425 * linked or renamed inode (without hierarchy). This is only used if
> 426 * @layer_masks_src_parent is not NULL.
> 427 *
> 428 * This helper first checks that the destination has a superset of restrictions
> 429 * compared to the source (if any) for a common path. It then checks that the
> 430 * collected accesses and the remaining ones are enough to allow the request.
> 431 *
> 432 * Returns:
> 433 * - 0 if the access request is granted;
> 434 * - -EACCES if it is denied because of access right other than
> 435 * LANDLOCK_ACCESS_FS_REFER;
> 436 * - -EXDEV if the renaming or linking would be a privileged escalation
> 437 * (according to each layered policies), or if LANDLOCK_ACCESS_FS_REFER is
> 438 * not allowed by the source or the destination.
> 439 */
> 440 static int check_access_path_dual(const struct landlock_ruleset *const domain,
> 441 const struct path *const path,
> 442 bool child_is_directory,
> 443 const access_mask_t access_request_dst_parent,
> 444 layer_mask_t (*const
> 445 layer_masks_dst_parent)[LANDLOCK_NUM_ACCESS_FS],
> 446 const access_mask_t access_request_src_parent,
> 447 layer_mask_t (*layer_masks_src_parent)[LANDLOCK_NUM_ACCESS_FS],
> 448 layer_mask_t (*layer_masks_child)[LANDLOCK_NUM_ACCESS_FS])
> 449 {
> 450 bool allowed_dst_parent = false, allowed_src_parent = false, is_dom_check;
> 451 struct path walker_path;
> 452 access_mask_t access_masked_dst_parent, access_masked_src_parent;
> 453
> 454 if (!access_request_dst_parent && !access_request_src_parent)
> 455 return 0;
> 456 if (WARN_ON_ONCE(!domain || !path))
> 457 return 0;
> 458 if (is_nouser_or_private(path->dentry))
> 459 return 0;
> 460 if (WARN_ON_ONCE(domain->num_layers < 1))
> 461 return -EACCES;
> 462
> > 463 BUILD_BUG_ON(!layer_masks_dst_parent);
> 464 if (layer_masks_src_parent) {
> 465 if (WARN_ON_ONCE(!layer_masks_child))
> 466 return -EACCES;
> 467 access_masked_dst_parent = access_masked_src_parent =
> 468 get_handled_accesses(domain);
> 469 is_dom_check = true;
> 470 } else {
> 471 if (WARN_ON_ONCE(layer_masks_child))
> 472 return -EACCES;
> 473 access_masked_dst_parent = access_request_dst_parent;
> 474 access_masked_src_parent = access_request_src_parent;
> 475 is_dom_check = false;
> 476 }
> 477
> 478 walker_path = *path;
> 479 path_get(&walker_path);
> 480 /*
> 481 * We need to walk through all the hierarchy to not miss any relevant
> 482 * restriction.
> 483 */
> 484 while (true) {
> 485 struct dentry *parent_dentry;
> 486 const struct landlock_rule *rule;
> 487
> 488 /*
> 489 * If at least all accesses allowed on the destination are
> 490 * already allowed on the source, respectively if there is at
> 491 * least as much as restrictions on the destination than on the
> 492 * source, then we can safely refer files from the source to
> 493 * the destination without risking a privilege escalation.
> 494 * This is crucial for standalone multilayered security
> 495 * policies. Furthermore, this helps avoid policy writers to
> 496 * shoot themselves in the foot.
> 497 */
> 498 if (is_dom_check && is_superset(child_is_directory,
> 499 layer_masks_dst_parent,
> 500 layer_masks_src_parent,
> 501 layer_masks_child)) {
> 502 allowed_dst_parent =
> 503 scope_to_request(access_request_dst_parent,
> 504 layer_masks_dst_parent);
> 505 allowed_src_parent =
> 506 scope_to_request(access_request_src_parent,
> 507 layer_masks_src_parent);
> 508
> 509 /* Stops when all accesses are granted. */
> 510 if (allowed_dst_parent && allowed_src_parent)
> 511 break;
> 512
> 513 /*
> 514 * Downgrades checks from domain handled accesses to
> 515 * requested accesses.
> 516 */
> 517 is_dom_check = false;
> 518 access_masked_dst_parent = access_request_dst_parent;
> 519 access_masked_src_parent = access_request_src_parent;
> 520 }
> 521
> 522 rule = find_rule(domain, walker_path.dentry);
> 523 allowed_dst_parent = unmask_layers(rule, access_masked_dst_parent,
> 524 layer_masks_dst_parent);
> 525 allowed_src_parent = unmask_layers(rule, access_masked_src_parent,
> 526 layer_masks_src_parent);
> 527
> 528 /* Stops when a rule from each layer grants access. */
> 529 if (allowed_dst_parent && allowed_src_parent)
> 530 break;
> 531
> 532 jump_up:
> 533 if (walker_path.dentry == walker_path.mnt->mnt_root) {
> 534 if (follow_up(&walker_path)) {
> 535 /* Ignores hidden mount points. */
> 536 goto jump_up;
> 537 } else {
> 538 /*
> 539 * Stops at the real root. Denies access
> 540 * because not all layers have granted access.
> 541 */
> 542 allowed_dst_parent = false;
> 543 break;
> 544 }
> 545 }
> 546 if (unlikely(IS_ROOT(walker_path.dentry))) {
> 547 /*
> 548 * Stops at disconnected root directories. Only allows
> 549 * access to internal filesystems (e.g. nsfs, which is
> 550 * reachable through /proc/<pid>/ns/<namespace>).
> 551 */
> 552 allowed_dst_parent = !!(walker_path.mnt->mnt_flags &
> 553 MNT_INTERNAL);
> 554 break;
> 555 }
> 556 parent_dentry = dget_parent(walker_path.dentry);
> 557 dput(walker_path.dentry);
> 558 walker_path.dentry = parent_dentry;
> 559 }
> 560 path_put(&walker_path);
> 561
> 562 if (allowed_dst_parent && allowed_src_parent)
> 563 return 0;
> 564
> 565 /*
> 566 * Unfortunately, we cannot prioritize EACCES over EXDEV for all
> 567 * RENAME_EXCHANGE cases because it depends on the source and
> 568 * destination order. This could be changed with a new
> 569 * security_path_rename hook implementation.
> 570 */
> 571 if (likely(is_eacces(layer_masks_dst_parent, access_request_dst_parent)
> 572 || is_eacces(layer_masks_src_parent,
> 573 access_request_src_parent)))
> 574 return -EACCES;
> 575
> 576 /*
> 577 * Gracefully forbids reparenting if the destination directory
> 578 * hierarchy is not a superset of restrictions of the source directory
> 579 * hierarchy, or if LANDLOCK_ACCESS_FS_REFER is not allowed by the
> 580 * source or the destination.
> 581 */
> 582 return -EXDEV;
> 583 }
> 584
>
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
^ permalink raw reply [flat|nested] 2+ messages in thread