Re: [PATCH 3/5] mm: abstract get_arg_page() stack expansion and mmap read lock

[Kees Cook]

On Tue, Dec 03, 2024 at 06:05:10PM +0000, Lorenzo Stoakes wrote:
> Right now fs/exec.c invokes expand_downwards(), an otherwise internal
> implementation detail of the VMA logic in order to ensure that an arg page
> can be obtained by get_user_pages_remote().

I think this is already in -next, but yeah, this looks good. It does mix
some logic changes, but it's pretty minor.

> -	if (write && pos < vma->vm_start) {
> -		mmap_write_lock(mm);
> -		ret = expand_downwards(vma, pos);
> -		if (unlikely(ret < 0)) {
> -			mmap_write_unlock(mm);
> -			return NULL;
> -		}
> -		mmap_write_downgrade(mm);
> -	} else
> -		mmap_read_lock(mm);
> +	if (!mmap_read_lock_maybe_expand(mm, vma, pos, write))
> +		return NULL;
>  
> [...]
> +	if (!write || addr >= new_vma->vm_start) {
> +		mmap_read_lock(mm);
> +		return true;
> +	}
> +
> +	if (!(new_vma->vm_flags & VM_GROWSDOWN))
> +		return false;

The VM_GROWSDOWN check is moved around a bit. It seems okay, though.

> +
> +	mmap_write_lock(mm);
> +	if (expand_downwards(new_vma, addr)) {
> +		mmap_write_unlock(mm);
> +		return false;
> +	}
> +
> +	mmap_write_downgrade(mm);
> +	return true;

I actually find this much more readable now since it follows the more
common "bail out early" coding style.

Acked-by: Kees Cook <kees@xxxxxxxxxx>

Thanks!

-Kees

-- 
Kees Cook