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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <202412131701.81693C36C@keescook>
Date: Fri, 13 Dec 2024 17:05:24 -0800
From: Kees Cook <kees@...nel.org>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: Andrew Morton <akpm@...ux-foundation.org>,
	"Liam R . Howlett" <Liam.Howlett@...cle.com>,
	Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
	Eric Biederman <ebiederm@...ssion.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
	linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/5] mm: abstract get_arg_page() stack expansion and mmap
 read lock

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@...nel.org>

Thanks!

-Kees

-- 
Kees Cook