Hi,
I'm getting a lot of unaligned access messages each time I
do "modprobe [-r] <module>" on sparc:
Kernel unaligned access at TPC[6ad9b4] pkcs7_verify+0x1ec/0x5e0
Kernel unaligned access at TPC[6a5484] crypto_shash_finup+0xc/0x5c
Kernel unaligned access at TPC[6a5390] crypto_shash_update+0xc/0x54
Kernel unaligned access at TPC[10150308] sha1_sparc64_update+0x14/0x5c [sha1_sparc64]
Kernel unaligned access at TPC[101501ac] __sha1_sparc64_update+0xc/0x98 [sha1_sparc64]
Looks like these are being caused by an unaligned desc at
desc = digest + digest_size;
Doing this:
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -46,7 +46,8 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
- sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
+ sinfo->sig.digest_size = digest_size =
+ ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
ret = -ENOMEM;
digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
makes the unaliagned message go away, but I dont know if
sinfo->sig.digest_size needs to be set to the (unaligned) raw
value of crypto_shash_digestsize() itself, and how to verify that
this doesnt break something else in crypto
--Sowmini
On Fri, Oct 02, 2015 at 02:00:14PM +0000, Sowmini Varadhan wrote:
>
> I'm getting a lot of unaligned access messages each time I
> do "modprobe [-r] <module>" on sparc:
>
> Kernel unaligned access at TPC[6ad9b4] pkcs7_verify+0x1ec/0x5e0
> Kernel unaligned access at TPC[6a5484] crypto_shash_finup+0xc/0x5c
> Kernel unaligned access at TPC[6a5390] crypto_shash_update+0xc/0x54
> Kernel unaligned access at TPC[10150308] sha1_sparc64_update+0x14/0x5c [sha1_sparc64]
> Kernel unaligned access at TPC[101501ac] __sha1_sparc64_update+0xc/0x98 [sha1_sparc64]
>
> Looks like these are being caused by an unaligned desc at
>
> desc = digest + digest_size;
>
> Doing this:
>
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -46,7 +46,8 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
> return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
>
> desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> - sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> + sinfo->sig.digest_size = digest_size =
> + ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
>
> ret = -ENOMEM;
> digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
>
> makes the unaliagned message go away, but I dont know if
> sinfo->sig.digest_size needs to be set to the (unaligned) raw
> value of crypto_shash_digestsize() itself, and how to verify that
> this doesnt break something else in crypto
What hash algorithm were you using?
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On (10/08/15 21:15), Herbert Xu wrote:
> > desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > - sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> > + sinfo->sig.digest_size = digest_size =
> > + ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
:
> What hash algorithm were you using?
Algorithm is sha1. From printk, crypto_shash_descsize(tfm) comes out
to 0x60, digest_size to 0x14. Stack trace (for each modprobe [-r]) is
pkcs7_verify+0x1d0/0x5e0
system_verify_data+0x54/0xb4
mod_verify_sig+0xa0/0xc4
load_module+0x48/0x16a0
SyS_init_module+0x114/0x128
linux_sparc_syscall+0x34/0x44
--Sowmini
On Thu, Oct 08, 2015 at 10:43:43AM -0400, Sowmini Varadhan wrote:
> On (10/08/15 21:15), Herbert Xu wrote:
> > > desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > > - sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> > > + sinfo->sig.digest_size = digest_size =
> > > + ALIGN(crypto_shash_digestsize(tfm), sizeof (*desc));
> :
> > What hash algorithm were you using?
>
> Algorithm is sha1. From printk, crypto_shash_descsize(tfm) comes out
> to 0x60, digest_size to 0x14. Stack trace (for each modprobe [-r]) is
>
> pkcs7_verify+0x1d0/0x5e0
> system_verify_data+0x54/0xb4
> mod_verify_sig+0xa0/0xc4
> load_module+0x48/0x16a0
> SyS_init_module+0x114/0x128
> linux_sparc_syscall+0x34/0x44
Thanks. We have two bugs here. First of all pkcs7_verify definitely
shouldn't place the structure after the digest without aligning the
pointer. So something like your patch is needed (but please use
alignof instead of sizeof). Also don't put in digest_size but
instead align the pointer like
desc = PTR_ALIGN(digest + digest_size, ...)
The sparc sha algorithms themselves need to declare the alignment
that they require. Currently they claim to be able to handle any
alignment which appears to not be the case.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On (10/12/15 21:32), Herbert Xu wrote:
> Thanks. We have two bugs here. First of all pkcs7_verify definitely
> shouldn't place the structure after the digest without aligning the
> pointer. So something like your patch is needed (but please use
> alignof instead of sizeof). Also don't put in digest_size but
> instead align the pointer like
>
> desc = PTR_ALIGN(digest + digest_size, ...)
That patch might not be rock-solid by itself though. I was seeing
some panics/crashes when I was running with that patch, so I backed
it off temporarily - should sinfo->sig.digest_size be set to the aligned
value?
> The sparc sha algorithms themselves need to declare the alignment
> that they require. Currently they claim to be able to handle any
> alignment which appears to not be the case.
How would one do that correctly? I'm not a crypto expert, but I can
help test the patch..
--Sowmini
From: Herbert Xu <[email protected]>
Date: Mon, 12 Oct 2015 21:32:09 +0800
> The sparc sha algorithms themselves need to declare the alignment
> that they require. Currently they claim to be able to handle any
> alignment which appears to not be the case.
The sparc SHA assembler can handle arbitrary alignment.
On (10/12/15 21:32), Herbert Xu wrote:
>
> .. pkcs7_verify definitely
> shouldn't place the structure after the digest without aligning the
> pointer. So something like your patch is needed (but please use
> alignof instead of sizeof). Also don't put in digest_size but
> instead align the pointer like
>
> desc = PTR_ALIGN(digest + digest_size, ...)
I tried the patch below for 24+ hours, and it ran without mishaps.
But given that the panics I saw earlier (page faults typically
triggered by openswan's pluto) occurred unpredictably, it would be
useful to have an expert-review of this code.
Thanks,
--Sowmini
diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
index d20c0b4..958ac01 100644
--- a/crypto/asymmetric_keys/pkcs7_verify.c
+++ b/crypto/asymmetric_keys/pkcs7_verify.c
@@ -46,14 +46,15 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
- sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
-
+ sinfo->sig.digest_size = crypto_shash_digestsize(tfm);
+ digest_size = crypto_shash_digestsize(tfm) +
+ ALIGN(crypto_shash_digestsize(tfm), __alignof__(*desc));
ret = -ENOMEM;
digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
if (!digest)
goto error_no_desc;
- desc = digest + digest_size;
+ desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc));
desc->tfm = tfm;
desc->flags = CRYPTO_TFM_REQ_MAY_SLEEP;
On Tue, Oct 13, 2015 at 09:29:28AM -0400, Sowmini Varadhan wrote:
>
> diff --git a/crypto/asymmetric_keys/pkcs7_verify.c b/crypto/asymmetric_keys/pkcs7_verify.c
> index d20c0b4..958ac01 100644
> --- a/crypto/asymmetric_keys/pkcs7_verify.c
> +++ b/crypto/asymmetric_keys/pkcs7_verify.c
> @@ -46,14 +46,15 @@ static int pkcs7_digest(struct pkcs7_message *pkcs7,
> return (PTR_ERR(tfm) == -ENOENT) ? -ENOPKG : PTR_ERR(tfm);
>
> desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> - sinfo->sig.digest_size = digest_size = crypto_shash_digestsize(tfm);
> -
> + sinfo->sig.digest_size = crypto_shash_digestsize(tfm);
> + digest_size = crypto_shash_digestsize(tfm) +
> + ALIGN(crypto_shash_digestsize(tfm), __alignof__(*desc));
You should leave digest_size as is but round it up when you use
it in kzalloc. Otherwise you are essentially rounding it up
twice which happens to work but looks rather odd. It may also
lead to bugs later on if somebody uses digest_size as the digest
size.
> ret = -ENOMEM;
> digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
So I would replace this with
digest = kzalloc(ALIGN(digest_size, desc_align) + desc_size, GFP_KERNEL);
> if (!digest)
> goto error_no_desc;
>
> - desc = digest + digest_size;
> + desc = PTR_ALIGN(digest + digest_size, __alignof__(*desc));
This looks good.
BTW when you submit this please add a sign-off as specified by
Documentation/SubmittingPatches.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Mon, Oct 12, 2015 at 07:06:34AM -0700, David Miller wrote:
> From: Herbert Xu <[email protected]>
> Date: Mon, 12 Oct 2015 21:32:09 +0800
>
> > The sparc sha algorithms themselves need to declare the alignment
> > that they require. Currently they claim to be able to handle any
> > alignment which appears to not be the case.
>
> The sparc SHA assembler can handle arbitrary alignment.
Right. The warnings that originated in sha1_sparc64 are probably
just the result of an unaligned desc structure caused by the bug
in pkcs7_verify.
Thanks for the confirmation.
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt