| Message ID | 1655600242.1561022.1474676547316.JavaMail.zimbra@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Herbert Xu |
| Headers | show |
Hi Jan, Just out of curiosity, have you tried to use "76" on both values to check if the problem still happens?
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,
Herbert, Wouldn't be enough to provide a pair of import/export functions as the padlock-sha driver does?
----- 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", -- 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
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,
----- 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 > -- 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
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
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,
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
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] the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html