On Tue, Jul 15, 2025 at 08:52:24PM +0200, Mickaël Salaün wrote:
> On Mon, Jul 14, 2025 at 01:39:12PM +0100, Tingmao Wang wrote:
> > 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?
>
> Yes, it's tricky, I'll add more comments.
>
> > 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.
>
> That's right. There are potential race conditions after each
> !path_connected() checks, but AFAICT it doesn't matter at that point
> because the access state for the current dentry is valid. This dentry
> could be renamed after this check, but we always check with another
> !path_connected() or mnt_root after that. This means that we could have
> partial access rights while a path is being renamed, but they should all
> be consistent at time of checks, right?
>
> >
> > 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.
>
> The layer_masks_child* accesses are only used to check if the moved file
> (or directory) would not get more access rights on the destination
> (excluding those directly moved with the child). Once we know the move
> would be safe, we check if the move is allowed according to the parent
> source and the parent destination (but the child access rights are
> ignored).
I misunderstood some parts of your comment, there should be no race
condition, good catch! It should be fixed in third patch series.
>
> It should be tested with
> layout4_disconnected_leafs.s1d41_s1d42_disconnected
>
> Thanks for the review!
