[bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Jan Stancek]

Hi,

I'm chasing a memory corruption with 4.8-rc7 as I'm observing random Oopses
on ppc BE/LE systems (lpars, KVM guests). About 30% of issues is that
module list gets corrupted, and "cat /proc/modules" or "lsmod" triggers
an Oops, for example:

[   88.486041] Unable to handle kernel paging request for data at address 0x00000020
...
[   88.487658] NIP [c00000000020f820] m_show+0xa0/0x240
[   88.487689] LR [c00000000020f834] m_show+0xb4/0x240
[   88.487719] Call Trace:
[   88.487736] [c0000004b605bbb0] [c00000000020f834] m_show+0xb4/0x240 (unreliable)
[   88.487796] [c0000004b605bc50] [c00000000045e73c] seq_read+0x36c/0x520
[   88.487843] [c0000004b605bcf0] [c0000000004e1014] proc_reg_read+0x84/0x120
[   88.487889] [c0000004b605bd30] [c00000000040df88] vfs_read+0xf8/0x380
[   88.487934] [c0000004b605bde0] [c00000000040fd40] SyS_read+0x60/0x110
[   88.487981] [c0000004b605be30] [c000000000009590] system_call+0x38/0xec

0x20 offset is module_use->source, module_use is NULL because module.source_list
gets corrupted.

The source of corruption appears to originate from a 'ahash' test for p8_ghash:

cryptomgr_test
 alg_test
  alg_test_hash
   test_hash
    __test_hash
     ahash_partial_update
      shash_async_export
       memcpy

With some extra traces [1], I'm seeing that ahash_partial_update() allocates 56 bytes
for 'state', and then crypto_ahash_export() writes 76 bytes into it:

[    5.970887] __test_hash alg name p8_ghash, result: c000000004333ac0, key: c0000004b860a500, req: c0000004b860a380
[    5.970963] state: c000000004333f00, statesize: 56
[    5.970995] shash_default_export memcpy c000000004333f00 c0000004b860a3e0, len: 76

This seems to directly correspond with:
  p8_ghash_alg.descsize = sizeof(struct p8_ghash_desc_ctx) == 56
  shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) + crypto_shash_descsize(fallback) == 56 + 20
where 20 is presumably coming from "ghash_alg.descsize".

My gut feeling was that these 2 should match, but I'd love to hear
what crypto people think.

Thank you,
Jan

[1]
diff --git a/crypto/shash.c b/crypto/shash.c
index a051541..49fe182 100644
--- a/crypto/shash.c
+++ b/crypto/shash.c
@@ -188,6 +188,8 @@ EXPORT_SYMBOL_GPL(crypto_shash_digest);

 static int shash_default_export(struct shash_desc *desc, void *out)
 {
+       int len = crypto_shash_descsize(desc->tfm);
+       printk("shash_default_export memcpy %p %p, len: %d\n", out, shash_desc_ctx(desc), len);
        memcpy(out, shash_desc_ctx(desc), crypto_shash_descsize(desc->tfm));
        return 0;
 }
diff --git a/crypto/testmgr.c b/crypto/testmgr.c
index 5c9d5a5..2e54579 100644
--- a/crypto/testmgr.c
+++ b/crypto/testmgr.c
@@ -218,6 +218,8 @@ static int ahash_partial_update(struct ahash_request **preq,
                pr_err("alt: hash: Failed to alloc state for %s\n", algo);
                goto out_nostate;
        }
+       printk("state: %p, statesize: %d\n", state, statesize);
+
        ret = crypto_ahash_export(req, state);
        if (ret) {
                pr_err("alt: hash: Failed to export() for %s\n", algo);
@@ -288,6 +290,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
                       "%s\n", algo);
                goto out_noreq;
        }
+       printk("__test_hash alg name %s, result: %p, key: %p, req: %p\n", algo, result, key, req);
        ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
                                   tcrypt_complete, &tresult);

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 4144 bytes --]

Hi Jan,

Just out of curiosity, have you tried to use "76" on both values to
check if the problem still happens?

-- 
Regards,
Marcelo

On Fri, Sep 23, 2016 at 08:22:27PM -0400, Jan Stancek wrote:
> Hi,
> 
> I'm chasing a memory corruption with 4.8-rc7 as I'm observing random Oopses
> on ppc BE/LE systems (lpars, KVM guests). About 30% of issues is that
> module list gets corrupted, and "cat /proc/modules" or "lsmod" triggers
> an Oops, for example:
> 
> [   88.486041] Unable to handle kernel paging request for data at address 0x00000020
> ...
> [   88.487658] NIP [c00000000020f820] m_show+0xa0/0x240
> [   88.487689] LR [c00000000020f834] m_show+0xb4/0x240
> [   88.487719] Call Trace:
> [   88.487736] [c0000004b605bbb0] [c00000000020f834] m_show+0xb4/0x240 (unreliable)
> [   88.487796] [c0000004b605bc50] [c00000000045e73c] seq_read+0x36c/0x520
> [   88.487843] [c0000004b605bcf0] [c0000000004e1014] proc_reg_read+0x84/0x120
> [   88.487889] [c0000004b605bd30] [c00000000040df88] vfs_read+0xf8/0x380
> [   88.487934] [c0000004b605bde0] [c00000000040fd40] SyS_read+0x60/0x110
> [   88.487981] [c0000004b605be30] [c000000000009590] system_call+0x38/0xec
> 
> 0x20 offset is module_use->source, module_use is NULL because module.source_list
> gets corrupted.
> 
> The source of corruption appears to originate from a 'ahash' test for p8_ghash:
> 
> cryptomgr_test
>  alg_test
>   alg_test_hash
>    test_hash
>     __test_hash
>      ahash_partial_update
>       shash_async_export
>        memcpy
> 
> With some extra traces [1], I'm seeing that ahash_partial_update() allocates 56 bytes
> for 'state', and then crypto_ahash_export() writes 76 bytes into it:
> 
> [    5.970887] __test_hash alg name p8_ghash, result: c000000004333ac0, key: c0000004b860a500, req: c0000004b860a380
> [    5.970963] state: c000000004333f00, statesize: 56
> [    5.970995] shash_default_export memcpy c000000004333f00 c0000004b860a3e0, len: 76
> 
> This seems to directly correspond with:
>   p8_ghash_alg.descsize = sizeof(struct p8_ghash_desc_ctx) == 56
>   shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) + crypto_shash_descsize(fallback) == 56 + 20
> where 20 is presumably coming from "ghash_alg.descsize".
> 
> My gut feeling was that these 2 should match, but I'd love to hear
> what crypto people think.
> 
> Thank you,
> Jan
> 
> [1]
> diff --git a/crypto/shash.c b/crypto/shash.c
> index a051541..49fe182 100644
> --- a/crypto/shash.c
> +++ b/crypto/shash.c
> @@ -188,6 +188,8 @@ EXPORT_SYMBOL_GPL(crypto_shash_digest);
> 
>  static int shash_default_export(struct shash_desc *desc, void *out)
>  {
> +       int len = crypto_shash_descsize(desc->tfm);
> +       printk("shash_default_export memcpy %p %p, len: %d\n", out, shash_desc_ctx(desc), len);
>         memcpy(out, shash_desc_ctx(desc), crypto_shash_descsize(desc->tfm));
>         return 0;
>  }
> diff --git a/crypto/testmgr.c b/crypto/testmgr.c
> index 5c9d5a5..2e54579 100644
> --- a/crypto/testmgr.c
> +++ b/crypto/testmgr.c
> @@ -218,6 +218,8 @@ static int ahash_partial_update(struct ahash_request **preq,
>                 pr_err("alt: hash: Failed to alloc state for %s\n", algo);
>                 goto out_nostate;
>         }
> +       printk("state: %p, statesize: %d\n", state, statesize);
> +
>         ret = crypto_ahash_export(req, state);
>         if (ret) {
>                 pr_err("alt: hash: Failed to export() for %s\n", algo);
> @@ -288,6 +290,7 @@ static int __test_hash(struct crypto_ahash *tfm, struct hash_testvec *template,
>                        "%s\n", algo);
>                 goto out_noreq;
>         }
> +       printk("__test_hash alg name %s, result: %p, key: %p, req: %p\n", algo, result, key, req);
>         ahash_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
>                                    tcrypt_complete, &tresult);
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Jan Stancek]

----- Original Message -----
> From: "Marcelo Cerri" <marcelo.cerri@canonical.com>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: "rui y wang" <rui.y.wang@intel.com>, herbert@gondor.apana.org.au, mhcerri@linux.vnet.ibm.com,
> leosilva@linux.vnet.ibm.com, pfsmorigo@linux.vnet.ibm.com, linux-crypto@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
> Sent: Monday, 26 September, 2016 4:15:10 PM
> Subject: Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7
> 
> Hi Jan,
> 
> Just out of curiosity, have you tried to use "76" on both values to
> check if the problem still happens?

I did, I haven't seen any panics with such patch:

@@ -211,7 +212,7 @@ struct shash_alg p8_ghash_alg = {
        .update = p8_ghash_update,
        .final = p8_ghash_final,
        .setkey = p8_ghash_setkey,
-       .descsize = sizeof(struct p8_ghash_desc_ctx),
+       .descsize = sizeof(struct p8_ghash_desc_ctx) + 20,
        .base = {
                 .cra_name = "ghash",
                 .cra_driver_name = "p8_ghash",

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Fri, Sep 23, 2016 at 08:22:27PM -0400, Jan Stancek wrote:
>
> This seems to directly correspond with:
>   p8_ghash_alg.descsize = sizeof(struct p8_ghash_desc_ctx) == 56
>   shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) + crypto_shash_descsize(fallback) == 56 + 20
> where 20 is presumably coming from "ghash_alg.descsize".
> 
> My gut feeling was that these 2 should match, but I'd love to hear
> what crypto people think.

Indeed.  The vmx driver is broken.  It is allocating a fallback
but is not providing any space for the state of the fallback.

Unfortunately our interface doesn't really provide a way to provide
the state size dynamically.  So what I'd suggest is to fix the
fallback to the generic ghash implementation and export its state
size like we do for md5/sha.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 1430 bytes --]

Herbert,

Wouldn't be enough to provide a pair of import/export functions as the
padlock-sha driver does?

-- 
Regards,
Marcelo

On Mon, Sep 26, 2016 at 10:59:34PM +0800, Herbert Xu wrote:
> On Fri, Sep 23, 2016 at 08:22:27PM -0400, Jan Stancek wrote:
> >
> > This seems to directly correspond with:
> >   p8_ghash_alg.descsize = sizeof(struct p8_ghash_desc_ctx) == 56
> >   shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx) + crypto_shash_descsize(fallback) == 56 + 20
> > where 20 is presumably coming from "ghash_alg.descsize".
> > 
> > My gut feeling was that these 2 should match, but I'd love to hear
> > what crypto people think.
> 
> Indeed.  The vmx driver is broken.  It is allocating a fallback
> but is not providing any space for the state of the fallback.
> 
> Unfortunately our interface doesn't really provide a way to provide
> the state size dynamically.  So what I'd suggest is to fix the
> fallback to the generic ghash implementation and export its state
> size like we do for md5/sha.
> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> --
> To unsubscribe from this list: send the line "unsubscribe linux-crypto" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Mon, Sep 26, 2016 at 02:43:17PM -0300, Marcelo Cerri wrote:
> 
> Wouldn't be enough to provide a pair of import/export functions as the
> padlock-sha driver does?

I don't think that will help as ultimately you need to call the
export function on the fallback and that's what requires the extra
memory.  In fact very operation involving the fallback will need
that extra memory too.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Jan Stancek]

----- Original Message -----
> From: "Herbert Xu" <herbert@gondor.apana.org.au>
> To: "Marcelo Cerri" <marcelo.cerri@canonical.com>
> Cc: "Jan Stancek" <jstancek@redhat.com>, "rui y wang" <rui.y.wang@intel.com>, mhcerri@linux.vnet.ibm.com,
> leosilva@linux.vnet.ibm.com, pfsmorigo@linux.vnet.ibm.com, linux-crypto@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
> Sent: Tuesday, 27 September, 2016 5:08:26 AM
> Subject: Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7
> 
> On Mon, Sep 26, 2016 at 02:43:17PM -0300, Marcelo Cerri wrote:
> > 
> > Wouldn't be enough to provide a pair of import/export functions as the
> > padlock-sha driver does?
> 
> I don't think that will help as ultimately you need to call the
> export function on the fallback and that's what requires the extra
> memory.  In fact very operation involving the fallback will need
> that extra memory too.

So, if we extended p8_ghash_desc_ctx to accommodate fallback_desc's ctx
and then provided statesize/import/export, would that be acceptable?

struct p8_ghash_desc_ctx {
        ...
        struct shash_desc fallback_desc;
+       char fallback_ctx[sizeof(struct ghash_desc_ctx)];


Also, does that mean that padlock_sha has similar problem?
It does not seem to reserve any space for fallback __ctx and it calls
init()/update()/export() with padlock_sha_desc's fallback:

struct padlock_sha_desc {
        struct shash_desc fallback;
};

static struct shash_alg sha1_alg = {
        .descsize       =       sizeof(struct padlock_sha_desc),

Regards,
Jan

> 
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

Hi,

On Tue, Sep 27, 2016 at 05:01:03AM -0400, Jan Stancek wrote:
> So, if we extended p8_ghash_desc_ctx to accommodate fallback_desc's ctx
> and then provided statesize/import/export, would that be acceptable?
> 
> struct p8_ghash_desc_ctx {
>         ...
>         struct shash_desc fallback_desc;
> +       char fallback_ctx[sizeof(struct ghash_desc_ctx)];
> 

I think so. That's the solution mentioned by Herbert. The only drawback
is that we will need to fix "ghash-generic" as the fallback
implementation in order to know beforehand its descsize.

However I would keep the p8_ghash_desc_ctx the way it is and I would
sum sizeof(struct ghash_desc_ctx) to the algorithm descsize instead.

Let me put a quick patch together to test this.

> 
> Also, does that mean that padlock_sha has similar problem?
> It does not seem to reserve any space for fallback __ctx and it calls
> init()/update()/export() with padlock_sha_desc's fallback:
> 
> struct padlock_sha_desc {
>         struct shash_desc fallback;
> };
> 
> static struct shash_alg sha1_alg = {
>         .descsize       =       sizeof(struct padlock_sha_desc),
> 

Yeah. It still seems to me that padlock-sha has the same problem. Maybe
we are missing something...

--
Regards,
Marcelo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 3002 bytes --]

Jan, 

Can you check if the problem occurs with this patch?

---
 drivers/crypto/vmx/ghash.c | 28 +++++++++++++++++-----------
 drivers/crypto/vmx/vmx.c   |  9 +++++++++
 2 files changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/crypto/vmx/ghash.c b/drivers/crypto/vmx/ghash.c
index 6c999cb0..033aba1 100644
--- a/drivers/crypto/vmx/ghash.c
+++ b/drivers/crypto/vmx/ghash.c
@@ -36,6 +36,8 @@
 #define GHASH_DIGEST_SIZE (16)
 #define GHASH_KEY_LEN (16)
 
+#define GHASH_FALLBACK_ALG "ghash-generic"
+
 void gcm_init_p8(u128 htable[16], const u64 Xi[2]);
 void gcm_gmult_p8(u64 Xi[2], const u128 htable[16]);
 void gcm_ghash_p8(u64 Xi[2], const u128 htable[16],
@@ -53,18 +55,26 @@ struct p8_ghash_desc_ctx {
 	struct shash_desc fallback_desc;
 };
 
+int p8_ghash_fallback_descsize(void)
+{
+	int descsize;
+	struct crypto_shash *fallback;
+	fallback = crypto_alloc_shash(GHASH_FALLBACK_ALG, 0,
+				      CRYPTO_ALG_NEED_FALLBACK);
+	if (IS_ERR(fallback)) {
+		return PTR_ERR(fallback);
+	}
+	descsize = crypto_shash_descsize(fallback);
+	crypto_free_shash(fallback);
+	return descsize;
+}
+
 static int p8_ghash_init_tfm(struct crypto_tfm *tfm)
 {
-	const char *alg;
+	const char *alg = GHASH_FALLBACK_ALG;
 	struct crypto_shash *fallback;
-	struct crypto_shash *shash_tfm = __crypto_shash_cast(tfm);
 	struct p8_ghash_ctx *ctx = crypto_tfm_ctx(tfm);
 
-	if (!(alg = crypto_tfm_alg_name(tfm))) {
-		printk(KERN_ERR "Failed to get algorithm name.\n");
-		return -ENOENT;
-	}
-
 	fallback = crypto_alloc_shash(alg, 0, CRYPTO_ALG_NEED_FALLBACK);
 	if (IS_ERR(fallback)) {
 		printk(KERN_ERR
@@ -79,10 +89,6 @@ static int p8_ghash_init_tfm(struct crypto_tfm *tfm)
 			       crypto_shash_get_flags((struct crypto_shash
 						       *) tfm));
 	ctx->fallback = fallback;
-
-	shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx)
-	    + crypto_shash_descsize(fallback);
-
 	return 0;
 }
 
diff --git a/drivers/crypto/vmx/vmx.c b/drivers/crypto/vmx/vmx.c
index 31a98dc..8a51149 100644
--- a/drivers/crypto/vmx/vmx.c
+++ b/drivers/crypto/vmx/vmx.c
@@ -28,6 +28,8 @@
 #include <asm/cputable.h>
 #include <crypto/internal/hash.h>
 
+int p8_ghash_fallback_descsize(void);
+
 extern struct shash_alg p8_ghash_alg;
 extern struct crypto_alg p8_aes_alg;
 extern struct crypto_alg p8_aes_cbc_alg;
@@ -45,6 +47,7 @@ int __init p8_init(void)
 {
 	int ret = 0;
 	struct crypto_alg **alg_it;
+	int ghash_descsize;
 
 	for (alg_it = algs; *alg_it; alg_it++) {
 		ret = crypto_register_alg(*alg_it);
@@ -59,6 +62,12 @@ int __init p8_init(void)
 	if (ret)
 		return ret;
 
+	ghash_descsize = p8_ghash_fallback_descsize();
+	if (ghash_descsize < 0) {
+		printk(KERN_ERR "Cannot get descsize for p8_ghash fallback\n");
+		return ghash_descsize;
+	}
+	p8_ghash_alg.descsize += ghash_descsize;
 	ret = crypto_register_shash(&p8_ghash_alg);
 	if (ret) {
 		for (alg_it = algs; *alg_it; alg_it++)
-- 
2.7.4


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote:
> 
> Can you check if the problem occurs with this patch?

In light of the fact that padlock-sha is the correct example
to follow, you only need to add one line to the init_tfm fucntion
to update the descsize based on that of the fallback.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Jan Stancek]

----- Original Message -----
> From: "Herbert Xu" <herbert@gondor.apana.org.au>
> To: "Marcelo Cerri" <marcelo.cerri@canonical.com>
> Cc: "Jan Stancek" <jstancek@redhat.com>, "rui y wang" <rui.y.wang@intel.com>, mhcerri@linux.vnet.ibm.com,
> leosilva@linux.vnet.ibm.com, pfsmorigo@linux.vnet.ibm.com, linux-crypto@vger.kernel.org,
> linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
> Sent: Wednesday, 28 September, 2016 4:45:49 AM
> Subject: Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7
> 
> On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote:
> > 
> > Can you check if the problem occurs with this patch?
> 
> In light of the fact that padlock-sha is the correct example
> to follow, you only need to add one line to the init_tfm fucntion
> to update the descsize based on that of the fallback.

Thanks for clearing up how this works in padlock-sha, but
we are not exactly in same situation with p8_ghash.

p8_ghash_init_tfm() already updates descsize. Problem in original report
is that without custom export/import/statesize p8_ghash_alg.statesize
gets initialized by shash_prepare_alg() to alg->descsize:

crash> p p8_ghash_alg.statesize
$1 = 56

testmgr allocates space for export based on crypto_shash_statesize(),
but shash_default_export() writes based on crypto_shash_descsize():
[    8.297902] state: c0000004b873aa80, statesize: 56
[    8.297932] shash_default_export memcpy c0000004b873aa80 c0000004b8607da0, len: 76

so I think we need either:
1) make sure p8_ghash_alg.descsize is correct before we register shash,
   this is what Marcelo's last patch is doing
2) provide custom export/import/statesize for p8_ghash_alg

Regards,
Jan

> 
> Thanks,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Wed, Sep 28, 2016 at 03:40:51AM -0400, Jan Stancek wrote:
> 
> Thanks for clearing up how this works in padlock-sha, but
> we are not exactly in same situation with p8_ghash.
> 
> p8_ghash_init_tfm() already updates descsize. Problem in original report
> is that without custom export/import/statesize p8_ghash_alg.statesize
> gets initialized by shash_prepare_alg() to alg->descsize:

Right.

> so I think we need either:
> 1) make sure p8_ghash_alg.descsize is correct before we register shash,
>    this is what Marcelo's last patch is doing

This approach doesn't work because there is no guarantee that
you'll get the same fallback the next time you allocate a tfm.
So relying on the descsize being constant can only work if all
implementations of the fallback use the same desc struct.

> 2) provide custom export/import/statesize for p8_ghash_alg

This works for padlock-sha because every implementation of SHA
uses the same state data structure from sha.h.  If we can make
all implementations of ghash agree on the exported state then
we can use the same approach.

Otherwise we can go back to allocating just ghash-generic and
also move its data structure into an exported header file.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 1944 bytes --]

Hi Hebert,

On Wed, Sep 28, 2016 at 08:29:35PM +0800, Herbert Xu wrote:
> On Wed, Sep 28, 2016 at 03:40:51AM -0400, Jan Stancek wrote:
> > 
> > Thanks for clearing up how this works in padlock-sha, but
> > we are not exactly in same situation with p8_ghash.
> > 
> > p8_ghash_init_tfm() already updates descsize. Problem in original report
> > is that without custom export/import/statesize p8_ghash_alg.statesize
> > gets initialized by shash_prepare_alg() to alg->descsize:
> 
> Right.
> 
> > so I think we need either:
> > 1) make sure p8_ghash_alg.descsize is correct before we register shash,
> >    this is what Marcelo's last patch is doing
> 
> This approach doesn't work because there is no guarantee that
> you'll get the same fallback the next time you allocate a tfm.
> So relying on the descsize being constant can only work if all
> implementations of the fallback use the same desc struct.

The patch forces ghash-generic as the fallback. And I don't think that
is a big problem if we decide to go by this path.
> 
> > 2) provide custom export/import/statesize for p8_ghash_alg
> 
> This works for padlock-sha because every implementation of SHA
> uses the same state data structure from sha.h.  If we can make
> all implementations of ghash agree on the exported state then
> we can use the same approach.

That would be nice because it would allow p8_ghash to keep using a
dynamic fallback, but I'm not that is viable. What do you think?

> 
> Otherwise we can go back to allocating just ghash-generic and
> also move its data structure into an exported header file.
> 

That would make the fix much more simple and it wouldn't require to get
the fallback descsize at runtime.

> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-- 
Regards,
Marcelo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Wed, Sep 28, 2016 at 09:38:41AM -0300, Marcelo Cerri wrote:
> 
> The patch forces ghash-generic as the fallback. And I don't think that
> is a big problem if we decide to go by this path.

Right it should work but could break for example if we ever decide
to change the exported state structure for ghash and someone unloads
the ghash-generic module and reloads a new one.

> That would be nice because it would allow p8_ghash to keep using a
> dynamic fallback, but I'm not that is viable. What do you think?

We did it for SHA because it was desirable to have multiple
fallbacks, i.e., a generic C version plus an assembly-optimised
version.

Not sure whether the same motiviation exists for GHASH.

> > Otherwise we can go back to allocating just ghash-generic and
> > also move its data structure into an exported header file.
> > 
> 
> That would make the fix much more simple and it wouldn't require to get
> the fallback descsize at runtime.

This is the easiest fix so let's go with this now.  If we ever
care enough to have multiple fallbacks for GHASH we can always
revisit this.  The exported format is not exposed to user-space
so it can always be changed.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

On Wed, Sep 28, 2016 at 08:44:52PM +0800, Herbert Xu wrote:
> On Wed, Sep 28, 2016 at 09:38:41AM -0300, Marcelo Cerri wrote:
> > 
> > The patch forces ghash-generic as the fallback. And I don't think that
> > is a big problem if we decide to go by this path.
> 
> Right it should work but could break for example if we ever decide
> to change the exported state structure for ghash and someone unloads
> the ghash-generic module and reloads a new one.
> 

Great! If we check the descsize every time a fallback tfm is allocated
that should be enough to prevent bigger problems such as memory
corruptions.

> > That would be nice because it would allow p8_ghash to keep using a
> > dynamic fallback, but I'm not that is viable. What do you think?
> 
> We did it for SHA because it was desirable to have multiple
> fallbacks, i.e., a generic C version plus an assembly-optimised
> version.
> 
> Not sure whether the same motiviation exists for GHASH.
> 
> > > Otherwise we can go back to allocating just ghash-generic and
> > > also move its data structure into an exported header file.
> > > 
> > 
> > That would make the fix much more simple and it wouldn't require to get
> > the fallback descsize at runtime.
> 
> This is the easiest fix so let's go with this now.  If we ever
> care enough to have multiple fallbacks for GHASH we can always
> revisit this.  The exported format is not exposed to user-space
> so it can always be changed.

Can I move ghash_desc_ctx to a header file under include/crypto/? Or do
you do you prefer to do that?

Maybe include/crypto/internal/hash.h or a new header file
include/crypto/internal/ghash.h ?

> 
> Cheers,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Wed, Sep 28, 2016 at 09:55:58AM -0300, Marcelo Cerri wrote:
> 
> Great! If we check the descsize every time a fallback tfm is allocated
> that should be enough to prevent bigger problems such as memory
> corruptions.

Yes a check wouldn't hurt.
 
> Can I move ghash_desc_ctx to a header file under include/crypto/? Or do
> you do you prefer to do that?

No you can go ahead and do the move.

> Maybe include/crypto/internal/hash.h or a new header file
> include/crypto/internal/ghash.h ?

Let's put it in include/crypto/ghash.h.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Marcelo Cerri]

[-- Attachment #1: Type: text/plain, Size: 1931 bytes --]

Hi Herbert,

On Wed, Sep 28, 2016 at 10:45:49AM +0800, Herbert Xu wrote:
> On Tue, Sep 27, 2016 at 04:46:44PM -0300, Marcelo Cerri wrote:
> >
> > Can you check if the problem occurs with this patch?
>
> In light of the fact that padlock-sha is the correct example
> to follow, you only need to add one line to the init_tfm fucntion
> to update the descsize based on that of the fallback.
>

Not sure if I'm missing something here but p8_ghash already does that:

 56 static int p8_ghash_init_tfm(struct crypto_tfm *tfm)
 57 {
 ...
 83         shash_tfm->descsize = sizeof(struct p8_ghash_desc_ctx)
 84             + crypto_shash_descsize(fallback);
 85
 86         return 0;
 87 }

I think the problem here is that crypto_ahash_statesize uses the
statesize value (that is initialized with the descsize value from
shash_alg) instead of using the value from the tfm that was updated.

For padlock_sha1, it uses the value from alg->statesize since it defines
import and export functions. It doesn't make much difference if it uses
the value from descsize or statesize here, what really matter is that
it uses the value defined in struct shash_alg and not from the tfm.

The big difference between p8_ghash and padlock_sha1 is that
padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which
is the descsize value used by sha1_generic. This probably works but
it's also wrong because the padlock_sha1 driver does not ensures that
sha1_generic is always used.

So, one solution is to hardcode ghash-generic as the fallback algorithm
and update the descsize direct in its shash_alg structure. There's only
one problem with that. ghash-generic desc type (struct ghash_desc_ctx)
is not available for vmx_crypto at compile type, the type is defined
directly in crypto/ghash-generic.c. That's the reason I added a function
to get the fallback desc size at runtime in the patch I wrote as a prove
of concept.

--
Regards,
Marcelo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Wed, Sep 28, 2016 at 09:28:44AM -0300, Marcelo Cerri wrote:
>
> The big difference between p8_ghash and padlock_sha1 is that
> padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which
> is the descsize value used by sha1_generic. This probably works but
> it's also wrong because the padlock_sha1 driver does not ensures that
> sha1_generic is always used.

It should work because all our SHA implementations use the same
export format.  This is not necessarily the case for GHASH though.

> So, one solution is to hardcode ghash-generic as the fallback algorithm
> and update the descsize direct in its shash_alg structure. There's only
> one problem with that. ghash-generic desc type (struct ghash_desc_ctx)
> is not available for vmx_crypto at compile type, the type is defined
> directly in crypto/ghash-generic.c. That's the reason I added a function
> to get the fallback desc size at runtime in the patch I wrote as a prove
> of concept.

The problem with your patch is that there is no guarantee that
you will get the same algorithm every time you allocate a fallback.
Someone may have loaded a new module for example.

So I think the safe approach is to stick with ghash-generic and
expose its state data structure in a header file.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Paulo Flabiano Smorigo]

Wed, Sep 28, 2016 at 08:33:18PM +0800, Herbert Xu wrote:
> On Wed, Sep 28, 2016 at 09:28:44AM -0300, Marcelo Cerri wrote:
> >
> > The big difference between p8_ghash and padlock_sha1 is that
> > padlock_sha1 defines alg->statesize as sizeof(struct sha1_state), which
> > is the descsize value used by sha1_generic. This probably works but
> > it's also wrong because the padlock_sha1 driver does not ensures that
> > sha1_generic is always used.
> 
> It should work because all our SHA implementations use the same
> export format.  This is not necessarily the case for GHASH though.
> 
> > So, one solution is to hardcode ghash-generic as the fallback algorithm
> > and update the descsize direct in its shash_alg structure. There's only
> > one problem with that. ghash-generic desc type (struct ghash_desc_ctx)
> > is not available for vmx_crypto at compile type, the type is defined
> > directly in crypto/ghash-generic.c. That's the reason I added a function
> > to get the fallback desc size at runtime in the patch I wrote as a prove
> > of concept.
> 
> The problem with your patch is that there is no guarantee that
> you will get the same algorithm every time you allocate a fallback.
> Someone may have loaded a new module for example.
> 
> So I think the safe approach is to stick with ghash-generic and
> expose its state data structure in a header file.

Since generic is the only posible fallback for ppc, I think that's a
good solution. :)

> 
> Thanks,
> -- 
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
> 

-- 
Paulo Flabiano Smorigo
IBM Linux Technology Center

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Jan Stancek]

> Jan,
> 
> Can you check if the problem occurs with this patch?

No issues in over-night test with this patch.

> --- a/drivers/crypto/vmx/vmx.c
> +++ b/drivers/crypto/vmx/vmx.c
> @@ -28,6 +28,8 @@
>  #include <asm/cputable.h>
>  #include <crypto/internal/hash.h>
>  
> +int p8_ghash_fallback_descsize(void);
> +
>  extern struct shash_alg p8_ghash_alg;
>  extern struct crypto_alg p8_aes_alg;
>  extern struct crypto_alg p8_aes_cbc_alg;
> @@ -45,6 +47,7 @@ int __init p8_init(void)
>  {
>  	int ret = 0;
>  	struct crypto_alg **alg_it;
> +	int ghash_descsize;
>  
>  	for (alg_it = algs; *alg_it; alg_it++) {
>  		ret = crypto_register_alg(*alg_it);
> @@ -59,6 +62,12 @@ int __init p8_init(void)
>  	if (ret)
>  		return ret;
>  
> +	ghash_descsize = p8_ghash_fallback_descsize();
> +	if (ghash_descsize < 0) {
> +		printk(KERN_ERR "Cannot get descsize for p8_ghash fallback\n");
> +		return ghash_descsize;
> +	}
> +	p8_ghash_alg.descsize += ghash_descsize;
>  	ret = crypto_register_shash(&p8_ghash_alg);
>  	if (ret) {
>  		for (alg_it = algs; *alg_it; alg_it++)

I'd suggest to move this inside vmx/ghash.c to a new function, so all p8_ghash
related code is at single place. Then p8_init() would just call the new
function:
-    ret = crypto_register_shash(&p8_ghash_alg);
+    ret = register_p8_ghash();

Regards,
Jan

Re: [bug] crypto/vmx/p8_ghash memory corruption in 4.8-rc7

[Herbert Xu]

On Tue, Sep 27, 2016 at 05:01:03AM -0400, Jan Stancek wrote:
> 
> Also, does that mean that padlock_sha has similar problem?
> It does not seem to reserve any space for fallback __ctx and it calls
> init()/update()/export() with padlock_sha_desc's fallback:
> 
> struct padlock_sha_desc {
>         struct shash_desc fallback;
> };
> 
> static struct shash_alg sha1_alg = {
>         .descsize       =       sizeof(struct padlock_sha_desc),

Actually I was wrong when I said that the API couldn't handle
a dynamic fallback.  It can and padlock-sha does the right thing
by updating descsize in the cra_init function.

So this is what vmx should do too.

Thanks,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt