linux-kernel - Re: [PATCH] powerpc/mm: add sanity check to avoid null pointer dereference

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: <48e444b9-ccdd-2aac-4cc6-3d9b75e90005@csgroup.eu>
Date:   Thu, 8 Sep 2022 06:21:03 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Defang Bo <bodefang@....com>,
        "mpe@...erman.id.au" <mpe@...erman.id.au>,
        "benh@...nel.crashing.org" <benh@...nel.crashing.org>,
        "paulus@...ba.org" <paulus@...ba.org>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "geert@...ux-m68k.org" <geert@...ux-m68k.org>,
        "rppt@...nel.org" <rppt@...nel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] powerpc/mm: add sanity check to avoid null pointer
 dereference



Le 28/12/2020 à 04:10, Defang Bo a écrit :
> Similar to commit<0dc294f717d4>("powerpc/mm: bail out early when flushing TLB page"),
> there should be a check for 'mm' to prevent Null pointer dereference
> in case of 'mm' argument was legitimately passed.

I don't understand what you are trying to do here.

In commit 0dc294f717d4, a call with a NULL mm is expected. The commit 
just reorganises the code in order to bail out earlier and more explicitely.

In your patch, you are adding WARN_ON, which means that being called 
with a NULL mm is unexpected. Why do you think it is necessary to do it 
? Have you encountered any problem ?

Thanks
Christophe

> 
> Signed-off-by: Defang Bo <bodefang@....com>
> ---
>   arch/powerpc/mm/nohash/tlb.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/mm/nohash/tlb.c b/arch/powerpc/mm/nohash/tlb.c
> index 5872f69..1d89335 100644
> --- a/arch/powerpc/mm/nohash/tlb.c
> +++ b/arch/powerpc/mm/nohash/tlb.c
> @@ -192,6 +192,9 @@ void local_flush_tlb_mm(struct mm_struct *mm)
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
>   	pid = mm->context.id;
>   	if (pid != MMU_NO_CONTEXT)
> @@ -205,8 +208,11 @@ void __local_flush_tlb_page(struct mm_struct *mm, unsigned long vmaddr,
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
> -	pid = mm ? mm->context.id : 0;
> +	pid = mm->context.id;
>   	if (pid != MMU_NO_CONTEXT)
>   		_tlbil_va(vmaddr, pid, tsize, ind);
>   	preempt_enable();
> @@ -268,6 +274,9 @@ void flush_tlb_mm(struct mm_struct *mm)
>   {
>   	unsigned int pid;
>   
> +	if (WARN_ON(!mm))
> +		return;
> +
>   	preempt_disable();
>   	pid = mm->context.id;
>   	if (unlikely(pid == MMU_NO_CONTEXT))