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
