linux-kernel - Re: [PATCH v2 2/2] x86/pti: check the return value of pti_user_pagetable_walk

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]

Date:   Tue, 17 Jul 2018 11:03:39 -0700
From:   Dave Hansen <dave.hansen@...ux.intel.com>
To:     Jiang Biao <jiang.biao2@....com.cn>, tglx@...utronix.de,
        mingo@...hat.com
Cc:     luto@...nel.org, hpa@...or.com, x86@...nel.org, albcamus@...il.com,
        linux-kernel@...r.kernel.org, zhong.weidong@....com.cn
Subject: Re: [PATCH v2 2/2] x86/pti: check the return value of
 pti_user_pagetable_walk_pmd

On 07/17/2018 12:23 AM, Jiang Biao wrote:
> Check the return value of pti_user_pagetable_walk_pmd() to avoid
> NULL pointer dereference. And add warning for fail allocation.

For both of these:

Acked-by: Dave Hansen <dave.hansen@...el.com>

It's minor, but if you redo these, I'd appreciate a slightly different
form.  Instead of:

> @@ -239,8 +239,10 @@ static pmd_t *pti_user_pagetable_walk_pmd(unsigned long address)
>  static __init pte_t *pti_user_pagetable_walk_pte(unsigned long address)
>  {
>  	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
> -	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
>  	pte_t *pte;
> +	pmd_t *pmd = pti_user_pagetable_walk_pmd(address);
> +	if (!pmd)
> +		return NULL;

I'd much rather see separation of code -- especially _important_ code
like an allocation -- from local variable definitions.  Like this:

	gfp_t gfp = (GFP_KERNEL | __GFP_NOTRACK | __GFP_ZERO);
	pmd_t *pmd;
  	pte_t *pte;

	pmd = pti_user_pagetable_walk_pmd(address);
	if (!pmd)
		return NULL;

That clearly separtes the variables from the _code_ and also nicely
pairs the action with the check for that action being successful.