linux-kernel - Re: [PATCH v2] arm64: mm: fix pmd

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Apr 2022 11:19:41 +0100
From:   Will Deacon <will@...nel.org>
To:     Muchun Song <songmuchun@...edance.com>
Cc:     catalin.marinas@....com, akpm@...ux-foundation.org,
        anshuman.khandual@....com, steven.price@....com,
        lengxujun2007@....com, arnd@...db.de, smuchun@...il.com,
        duanxiongchun@...edance.com, quic_qiancai@...cinc.com,
        aneesh.kumar@...ux.ibm.com, linux-arm-kernel@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] arm64: mm: fix pmd_leaf()

On Mon, Apr 11, 2022 at 08:26:53PM +0800, Muchun Song wrote:
> The pmd_leaf() is used to test a leaf mapped PMD, however, it misses
> the PROT_NONE mapped PMD on arm64.  Fix it.  A real world issue [1]
> caused by this was reported by Qian Cai.
> 
> Link: https://patchwork.kernel.org/comment/24798260/ [1]
> Fixes: 8aa82df3c123 ("arm64: mm: add p?d_leaf() definitions")
> Reported-by: Qian Cai <quic_qiancai@...cinc.com>
> Signed-off-by: Muchun Song <songmuchun@...edance.com>
> ---
> v2:
> - Replace pmd_present() with pmd_val() since we expect pmd_leaf() works
>   well on non-present pmd case.
> 
>  arch/arm64/include/asm/pgtable.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/pgtable.h b/arch/arm64/include/asm/pgtable.h
> index ad9b221963d4..00cdd2d895d3 100644
> --- a/arch/arm64/include/asm/pgtable.h
> +++ b/arch/arm64/include/asm/pgtable.h
> @@ -551,7 +551,7 @@ extern pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
>  				 PMD_TYPE_TABLE)
>  #define pmd_sect(pmd)		((pmd_val(pmd) & PMD_TYPE_MASK) == \
>  				 PMD_TYPE_SECT)
> -#define pmd_leaf(pmd)		pmd_sect(pmd)
> +#define pmd_leaf(pmd)		(pmd_val(pmd) && !(pmd_val(pmd) & PMD_TABLE_BIT))
>  #define pmd_bad(pmd)		(!pmd_table(pmd))

I'm still trying to get my head around the desired semantics here.

If we want to fix the original report, then we need to take PROT_NONE
entries into account. The easiest way to do that is, as you originally
suggested, by using pmd_present():

#define pmd_leaf(pmd)	(pmd_present(pmd) && !pmd_table(pmd))

But now you seem to be saying that !pmd_present() entries should also be
considered as pmd_leaf() -- is there a real need for that?

If so, then I think this simply becomes:

#define pmd_leaf(pmd)	(!pmd_table(pmd))

which is, amusingly, identical to pmd_bad().

The documentation/comment that Steven referred to also desperately needs
clarifying as it currently states:

  "Only meaningful when called on a valid entry."

whatever that means.

Finally, if this has implications beyond PROT_NONE (as I think you're
suggesting in your v2) then pud_leaf() probably needs similar treatment.
And we can remove pmd_sect() altogether if we no longer need it.

Will