unaligned access in pkcs7_verify

unaligned access in pkcs7_verify

[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

Re: unaligned access in pkcs7_verify

[Herbert Xu]

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

Re: unaligned access in pkcs7_verify

[Sowmini Varadhan]

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

Re: unaligned access in pkcs7_verify

[Herbert Xu]

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

Re: unaligned access in pkcs7_verify

[Sowmini Varadhan]

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

Re: unaligned access in pkcs7_verify

[David Miller]

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.

Re: unaligned access in pkcs7_verify

[Herbert Xu]

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

Re: unaligned access in pkcs7_verify

[Sowmini Varadhan]

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;
 

Re: unaligned access in pkcs7_verify

[Herbert Xu]

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