Date:   Sat, 05 Dec 2020 20:48:15 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Corentin Labbe <clabbe.montjoie@...il.com>
Cc:     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>,
        Julia Lawall <julia.lawall@...6.fr>
Subject: Re: crypto: sun4i-ss: error with kmap

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.

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);
 

Powered by blists - more mailing lists