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

[Mickaël Salaün]

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!