On 7/11/25 20:19, Mickaël Salaün wrote:
> [...]
> @@ -800,6 +802,8 @@ static bool is_access_to_paths_allowed(
> access_masked_parent1 = access_masked_parent2 =
> landlock_union_access_masks(domain).fs;
> is_dom_check = true;
> + memcpy(&_layer_masks_parent2_bkp, layer_masks_parent2,
> + sizeof(_layer_masks_parent2_bkp));
> } else {
> if (WARN_ON_ONCE(dentry_child1 || dentry_child2))
> return false;
> @@ -807,6 +811,8 @@ static bool is_access_to_paths_allowed(
> access_masked_parent1 = access_request_parent1;
> access_masked_parent2 = access_request_parent2;
> is_dom_check = false;
> + memcpy(&_layer_masks_parent1_bkp, layer_masks_parent1,
> + sizeof(_layer_masks_parent1_bkp));
Is this memcpy meant to be in this else branch? If parent2 is set, we
will leave _layer_masks_parent1_bkp uninitialized right?
> }
>
> if (unlikely(dentry_child1)) {
> @@ -858,6 +864,14 @@ static bool is_access_to_paths_allowed(
> child1_is_directory, layer_masks_parent2,
> layer_masks_child2,
> child2_is_directory))) {
> + /*
> + * Rewinds walk for disconnected directories before any other state
> + * change.
> + */
> + if (unlikely(!path_connected(walker_path.mnt,
> + walker_path.dentry)))
> + goto reset_to_mount_root;
> +
> /*
> * Now, downgrades the remaining checks from domain
> * handled accesses to requested accesses.
I think reasoning about how the domain check interacts with
reset_to_mount_root was very tricky, and I wonder if you could add some
more comments explaining the various cases? For example, one fact which
took me a while to realize is that for renames, this function will never
see the bottom-most child being disconnected with its mount, since we
start walking from the mountpoint, and so it is really only handling the
case of the mountpoint itself being disconnected.
Also, it was not very clear to me whether it would always be correct to
reset to the backed up layer mask, if the backup was taken when we were
still in domain check mode (and thus have the domain handled access bits
set, not just the requested ones), but we then exit domain check mode, and
before reaching the next mountpoint we suddenly found out the current path
is disconnected, and thus resetting to the backup (but without going back
into domain check mode, since we don't reset that).
Because of the !path_connected check within the if(is_dom_check ...)
branch itself, the above situation would only happen in some race
condition tho.
I also wonder if there's another potential issue (although I've not tested
it), where if the file being renamed itself is disconnected from its
mountpoint, when we get to is_access_to_paths_allowed, the passed in
layer_masks_parent1 would be empty (which is correct), but when we do the
no_more_access check, we're still using layer_masks_child{1,2} which has
access bits set according to rules attached directly to the child. I think
technically if the child is disconnected from its mount, we're supposed to
ignore any access rules it has on itself as well? And so this
no_more_access check would be a bit inconsistent, I think.
