Re: [PATCH v2 1/3] landlock: Fix handling of disconnected directories

["Mickaël Salaün" <mic@digikod.net>]

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).

It should be tested with
layout4_disconnected_leafs.s1d41_s1d42_disconnected

Thanks for the review!