* unaligned access in pkcs7_verify
@ 2015-10-02 14:00 Sowmini Varadhan
2015-10-08 13:15 ` Herbert Xu
0 siblings, 1 reply; 9+ messages in thread
From: Sowmini Varadhan @ 2015-10-02 14:00 UTC (permalink / raw)
To: dhowells, linux-crypto; +Cc: sowmini.varadhan
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-02 14:00 unaligned access in pkcs7_verify Sowmini Varadhan
@ 2015-10-08 13:15 ` Herbert Xu
2015-10-08 14:43 ` Sowmini Varadhan
0 siblings, 1 reply; 9+ messages in thread
From: Herbert Xu @ 2015-10-08 13:15 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: dhowells, linux-crypto
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-08 13:15 ` Herbert Xu
@ 2015-10-08 14:43 ` Sowmini Varadhan
2015-10-12 13:32 ` Herbert Xu
0 siblings, 1 reply; 9+ messages in thread
From: Sowmini Varadhan @ 2015-10-08 14:43 UTC (permalink / raw)
To: Herbert Xu; +Cc: dhowells, linux-crypto
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-08 14:43 ` Sowmini Varadhan
@ 2015-10-12 13:32 ` Herbert Xu
2015-10-12 13:46 ` Sowmini Varadhan
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Herbert Xu @ 2015-10-12 13:32 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: dhowells, linux-crypto, David S. Miller
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-12 13:32 ` Herbert Xu
@ 2015-10-12 13:46 ` Sowmini Varadhan
2015-10-12 14:06 ` David Miller
2015-10-13 13:29 ` Sowmini Varadhan
2 siblings, 0 replies; 9+ messages in thread
From: Sowmini Varadhan @ 2015-10-12 13:46 UTC (permalink / raw)
To: Herbert Xu; +Cc: dhowells, linux-crypto, David S. Miller
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-12 13:32 ` Herbert Xu
2015-10-12 13:46 ` Sowmini Varadhan
@ 2015-10-12 14:06 ` David Miller
2015-10-13 13:39 ` Herbert Xu
2015-10-13 13:29 ` Sowmini Varadhan
2 siblings, 1 reply; 9+ messages in thread
From: David Miller @ 2015-10-12 14:06 UTC (permalink / raw)
To: herbert; +Cc: sowmini.varadhan, dhowells, linux-crypto
From: Herbert Xu <herbert@gondor.apana.org.au>
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-12 13:32 ` Herbert Xu
2015-10-12 13:46 ` Sowmini Varadhan
2015-10-12 14:06 ` David Miller
@ 2015-10-13 13:29 ` Sowmini Varadhan
2015-10-13 13:36 ` Herbert Xu
2 siblings, 1 reply; 9+ messages in thread
From: Sowmini Varadhan @ 2015-10-13 13:29 UTC (permalink / raw)
To: Herbert Xu; +Cc: dhowells, linux-crypto, David S. Miller
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;
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-13 13:29 ` Sowmini Varadhan
@ 2015-10-13 13:36 ` Herbert Xu
0 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2015-10-13 13:36 UTC (permalink / raw)
To: Sowmini Varadhan; +Cc: dhowells, linux-crypto, David S. Miller
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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: unaligned access in pkcs7_verify
2015-10-12 14:06 ` David Miller
@ 2015-10-13 13:39 ` Herbert Xu
0 siblings, 0 replies; 9+ messages in thread
From: Herbert Xu @ 2015-10-13 13:39 UTC (permalink / raw)
To: David Miller; +Cc: sowmini.varadhan, dhowells, linux-crypto
On Mon, Oct 12, 2015 at 07:06:34AM -0700, David Miller wrote:
> From: Herbert Xu <herbert@gondor.apana.org.au>
> 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 <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-10-13 13:39 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-10-02 14:00 unaligned access in pkcs7_verify Sowmini Varadhan
2015-10-08 13:15 ` Herbert Xu
2015-10-08 14:43 ` Sowmini Varadhan
2015-10-12 13:32 ` Herbert Xu
2015-10-12 13:46 ` Sowmini Varadhan
2015-10-12 14:06 ` David Miller
2015-10-13 13:39 ` Herbert Xu
2015-10-13 13:29 ` Sowmini Varadhan
2015-10-13 13:36 ` Herbert Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.