linux-kernel - Re: crypto: sun4i-ss: error with kmap

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]

Message-ID: <alpine.DEB.2.22.394.2012052115430.2754@hadrien>
Date:   Sat, 5 Dec 2020 21:16:04 +0100 (CET)
From:   Julia Lawall <julia.lawall@...ia.fr>
To:     Thomas Gleixner <tglx@...utronix.de>
cc:     Corentin Labbe <clabbe.montjoie@...il.com>,
        herbert@...dor.apana.org.au, mripard@...nel.org, wens@...e.org,
        linux-arm-kernel@...ts.infradead.org, linux-crypto@...r.kernel.org,
        linux-kernel@...r.kernel.org, Jens Axboe <axboe@...nel.dk>,
        linux-mm@...ck.org, Andrew Morton <akpm@...uxfoundation.org>
Subject: Re: crypto: sun4i-ss: error with kmap



On Sat, 5 Dec 2020, Thomas Gleixner wrote:

> Corentin,
>
> On Sat, Dec 05 2020 at 19:43, Corentin Labbe wrote:
> > On Fri, Dec 04, 2020 at 09:58:21PM +0100, Thomas Gleixner wrote:
> >> Can you please replace the debug patch with the one below and try again?
> >> That stops the trace right on the condition.
> >
> > Hello, the result could be found at http://kernel.montjoie.ovh/130739.log
>
> Thanks for providing this. This is clearly showing where stuff goes
> wrong. It starts here at 729.550001. I removed the uninteresting parts:
>
> 0d..2 147103293us : __kmap_local_page_prot <-sg_miter_next
> 0d..3 147103308us :__kmap_local_pfn_prot: kmap_local_pfn: 1 ffefd000
>
> 0d..3 147103311us : __kmap_local_page_prot <-sg_miter_next
> 0d..4 147103325us : __kmap_local_pfn_prot: kmap_local_pfn: 3 ffefb000
>
> 0d..3 147103429us : kunmap_local_indexed <-sg_miter_stop
> 0d..4 147103433us : kunmap_local_indexed: kunmap_local: 3 ffefd000
>
> So this maps two pages and unmaps the first one. That's all called from
> sun4i_ss_opti_poll() and the bug is clearly visible there:
>
> 	sg_miter_next(&mi);
> 	sg_miter_next(&mo);
>
> release_ss:
> 	sg_miter_stop(&mi);
> 	sg_miter_stop(&mo);
>
> Written by yourself :) Same issue in sun4i_ss_cipher_poll()
>
> Fix below.
>
> Julia, it might be worth to have a coccinelle check for that. It's the
> only place which got it wrong, but this goes unnoticed when code is
> e.g. only fully tested on 64bit or as in this case never tested with
> full debugging enabled. The whole kmap_atomic and kmap_local (new in
> next) family and all users like the sg_miter stuff are affected by this.

OK, thanks for the suggestion.  I will look into it.

julia

>
> Thanks,
>
>         tglx
> ---
> Subject: crypto: sun4i-ss - Fix sg_miter_stop() ordering
> From: Thomas Gleixner <tglx@...utronix.de>
> Date: Sat, 05 Dec 2020 20:17:28 +0100
>
> sun4i_ss_opti_poll() and sun4i_ss_cipher_poll() do:
>
>         sg_miter_next(&mi);
>         sg_miter_next(&mo);
> 	...
>         sg_miter_stop(&mi);
>         sg_miter_stop(&mo);
>
> which is the wrong order because sg_miter_next() maps a page with
> kmap_atomic() and sg_miter_stop() unmaps it. kmap_atomic() uses a stack
> internaly which requires that the nested map is unmapped first. As this
> uses the wrong order it triggers the warning in kunmap_local_indexed()
> which checks the to be unmapped address and subsequently crashes.
>
> This went unnoticed for 5 years because the ARM kmap_atomic()
> implementation had the warning conditional on CONFIG_DEBUG_HIGHMEM which
> was obviously never enabled when testing that driver.
>
> Flip the order to cure it.
>
> Reported-by: Corentin Labbe <clabbe.montjoie@...il.com>
> Fixes: 6298e948215f ("crypto: sunxi-ss - Add Allwinner Security System crypto accelerator")
> Signed-off-by: Thomas Gleixner <tglx@...utronix.de>
> Cc: stable@...r.kernel.org
> ---
>  drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> --- a/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> +++ b/drivers/crypto/allwinner/sun4i-ss/sun4i-ss-cipher.c
> @@ -109,8 +109,8 @@ static int noinline_for_stack sun4i_ss_o
>  	}
>
>  release_ss:
> -	sg_miter_stop(&mi);
>  	sg_miter_stop(&mo);
> +	sg_miter_stop(&mi);
>  	writel(0, ss->base + SS_CTL);
>  	spin_unlock_irqrestore(&ss->slock, flags);
>  	return err;
> @@ -333,8 +333,8 @@ static int sun4i_ss_cipher_poll(struct s
>  	}
>
>  release_ss:
> -	sg_miter_stop(&mi);
>  	sg_miter_stop(&mo);
> +	sg_miter_stop(&mi);
>  	writel(0, ss->base + SS_CTL);
>  	spin_unlock_irqrestore(&ss->slock, flags);
>
>
>
>