Re: [PATCH] Use kzfree in crypto API context initialization and key/iv handling
From: Larry H. resea...@subreption.com
Date: Sat, 30 May 2009 19:57:20 -0700
[PATCH] Use kzfree in crypto API context initialization and key/iv handling
Thanks for not CC:ing the crypto list, and also not CC:'ing the
crypto maintainer.
Your submissions leave a lot to be desired, on every level.
This patch replaces the kfree() calls within the crypto API (algorithms,
key setup and handling, etc) with kzfree(), to enforce sanitization of
the allocated memory.
This prevents such information from persisting on memory and eventually
leak to other kernel users or during coldboot attacks.
This patch replaces kfree() for context (algorithm meta-data) structures
too. Those are initialized or released once, and remain in use during the
lifetime of the cipher/algorithm instance, therefore no performance impact
exists for those specific changes.
This patch doesn't affect fastpaths.
Signed-off-by: Larry Highsmith resea...@subreption.com
---
crypto/ablkcipher.c |3 +--
crypto/aead.c |7 +++
crypto/ahash.c |3 +--
crypto/algapi.c |4 ++--
crypto/algboss.c|8
crypto/api.c| 13 +
crypto/authenc.c|4 ++--
crypto/blkcipher.c | 13 +++--
crypto/cbc.c|2 +-
crypto/ccm.c|8
crypto/cipher.c |3 +--
crypto/cryptd.c |4 ++--
crypto/ctr.c|2 +-
crypto/cts.c|2 +-
crypto/deflate.c|4 ++--
crypto/ecb.c|2 +-
crypto/gcm.c| 10 +-
crypto/gf128mul.c |4 ++--
crypto/hash.c |3 +--
crypto/hmac.c |2 +-
crypto/lrw.c|2 +-
crypto/pcbc.c |2 +-
crypto/rng.c|2 +-
crypto/seqiv.c |4 ++--
crypto/shash.c |3 +--
crypto/xcbc.c |2 +-
crypto/xts.c|2 +-
27 files changed, 55 insertions(+), 63 deletions(-)
Index: linux-2.6/crypto/ablkcipher.c
===
--- linux-2.6.orig/crypto/ablkcipher.c
+++ linux-2.6/crypto/ablkcipher.c
@@ -42,8 +42,7 @@ static int setkey_unaligned(struct crypt
alignbuffer = (u8 *)ALIGN((unsigned long)buffer, alignmask + 1);
memcpy(alignbuffer, key, keylen);
ret = cipher-setkey(tfm, alignbuffer, keylen);
- memset(alignbuffer, 0, keylen);
- kfree(buffer);
+ kzfree(buffer);
return ret;
}
Index: linux-2.6/crypto/aead.c
===
--- linux-2.6.orig/crypto/aead.c
+++ linux-2.6/crypto/aead.c
@@ -40,8 +40,7 @@ static int setkey_unaligned(struct crypt
alignbuffer = (u8 *)ALIGN((unsigned long)buffer, alignmask + 1);
memcpy(alignbuffer, key, keylen);
ret = aead-setkey(tfm, alignbuffer, keylen);
- memset(alignbuffer, 0, keylen);
- kfree(buffer);
+ kzfree(buffer);
return ret;
}
@@ -298,7 +297,7 @@ out:
err_drop_alg:
crypto_drop_aead(spawn);
err_free_inst:
- kfree(inst);
+ kzfree(inst);
inst = ERR_PTR(err);
goto out;
}
@@ -307,7 +306,7 @@ EXPORT_SYMBOL_GPL(aead_geniv_alloc);
void aead_geniv_free(struct crypto_instance *inst)
{
crypto_drop_aead(crypto_instance_ctx(inst));
- kfree(inst);
+ kzfree(inst);
}
EXPORT_SYMBOL_GPL(aead_geniv_free);
Index: linux-2.6/crypto/ahash.c
===
--- linux-2.6.orig/crypto/ahash.c
+++ linux-2.6/crypto/ahash.c
@@ -145,8 +145,7 @@ static int ahash_setkey_unaligned(struct
alignbuffer = (u8 *)ALIGN((unsigned long)buffer, alignmask + 1);
memcpy(alignbuffer, key, keylen);
ret = ahash-setkey(tfm, alignbuffer, keylen);
- memset(alignbuffer, 0, keylen);
- kfree(buffer);
+ kzfree(buffer);
return ret;
}
Index: linux-2.6/crypto/algapi.c
===
--- linux-2.6.orig/crypto/algapi.c
+++ linux-2.6/crypto/algapi.c
@@ -185,7 +185,7 @@ out:
return larval;
free_larval:
- kfree(larval);
+ kzfree(larval);
err:
larval = ERR_PTR(ret);
goto out;
@@ -657,7 +657,7 @@ struct crypto_instance *crypto_alloc_ins
return inst;
err_free_inst:
- kfree(inst);
+ kzfree(inst);
return ERR_PTR(err);
}
EXPORT_SYMBOL_GPL(crypto_alloc_instance);
Index: linux-2.6/crypto/algboss.c
===
--- linux-2.6.orig/crypto/algboss.c
+++ linux-2.6/crypto/algboss.c
@@ -81,7 +81,7 @@ static int cryptomgr_probe(void *data)
goto err;
out:
- kfree(param);
+ kzfree(param);
module_put_and_exit(0);
err:
@@ -193,7 +193,7 @@ static int cryptomgr_schedule_probe(stru
return NOTIFY_STOP;
err_free_param:
- kfree(param);
+ kzfree(param);
err_put_module:
Re: [patch 5/5] Apply the PG_sensitive flag to the CryptoAPI subsystem
Also, there's no discussion about long-lived threads keeping sensitive information in there kernel stack indefinitely. kernel stack clearing isn't hard to do, just do it on every syscall exit and in the infinite loop for kernel threads. Actually that is probably not as important. In most cases you would be leaking data between syscalls made by the same thread. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 5/5] Apply the PG_sensitive flag to the CryptoAPI subsystem
On 30 May 2009 at 20:05, Ingo Molnar wrote: I think there's a rather significant omission here: there's no discussion about on-kernel-stack information leaking out. If a thread that does a crypto call happens to leave sensitive on-stack data (this can happen easily as stack variables are not cleared on return), or if a future variant or modification of a crypto algorithm leaves such information around - then there's nothing that keeps that data from potentially leaking out. This is not academic and it happens all the time: only look at various crash dumps on lkml. For example this crash shows such leaked information: [ 96.138788] [810ab62e] perf_counter_exit_task+0x10e/0x3f3 [ 96.145464] [8104cf46] do_exit+0x2e7/0x722 [ 96.150837] [810630cf] ? up_read+0x9/0xb [ 96.156036] [8151cc0b] ? do_page_fault+0x27d/0x2a5 [ 96.162141] [8104d3f4] do_group_exit+0x73/0xa0 [ 96.167860] [8104d433] sys_exit_group+0x12/0x16 [ 96.173665] [8100bb2b] system_call_fastpath+0x16/0x1b The '8151cc0b' 64-bit word is actually a leftover from a previous system context. ( And this is at the bottom of the stack that gets cleared all the time - the top of the kernel stack is a lot more more persistent in practice and crypto calls tend to have a healthy stack footprint. ) So IMO the GFP_SENSITIVE facility (beyond being misnomer - it should be something like GFP_NON_PERSISTENT instead) actually results in _worse_ security in the end: because people (and organizations) 'think' that their keys are safe against information leaks via this space, while they are not. The kernel stack can be freed, be reused by something else partially and then written out to disk (say as part of hibernation) where it's recoverable from the disk image. So this whole facility probably only makes sense if all kernel stacks that handle sensitive data are zeroed on free. But i havent seen any kernel thread stack clearing functionality in this patch-set - is it an intentional omission? (or have i missed some aspect of the patch-set) i think you missed the fact that the page flag based approach had been abandoned already in favour of unconditional page sanitizing on free (modulo a kernel boot option). the other approach of doing the sanitizing on a smaller allocation base (kfree, etc) is orthogonal to this one since they address the lifetime problem at different levels (i'm just making it clear since you brought up a freed kernel stack ending up in a hibernation image and leaving data there, that obviously won't happen as the freed kernel stack pages will be sanitized on free). now as for kernel stacks. first of all, the original idea of sanitization was meant to address userland secrets staying around for too long, little if any of that is long-lived on kernel stacks. kernel data lifetime got affected by virtue of doing the sanitization at the lowest possible level of the page allocator (which was in turn favoured over the page flag and strict 'userland data only' sanitization due to its simplicity, a few lines of code literally). so consider that as a fortunate sideeffect. with that said, there's certainly room for evolution, both in addressing more kind of data (it's not only the kernel stack you mention but also the userland stack whose unused pages can be taken back for example) and/or reducing lifetime further. i personally never bothered with any of that because the original request/goal was already addressed. Also, there's no discussion about long-lived threads keeping sensitive information in there kernel stack indefinitely. kernel stack clearing isn't hard to do, just do it on every syscall exit and in the infinite loop for kernel threads. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HMAC regression
You must getting an sg entry that crosses a page boundary, rather than two sg entries that both stay within a page. Yes. These things are very rare, and usually occurs as a result of SLAB debugging causing kmalloc to return memory that crosses page boundaries. Indeed, SLAB_DEBUG was enabled in my config. Disabling it resolves this issue. Can you see if this patch fixes the problem? Yes, it fixes HMAC calculation with enabled SLAB debugging. Thanks for your help! -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Crypto Update for 2.6.30
Hi Linus:
This push fixes a regression that triggers with SLAB debugging on,
where the new ahash code fails to handle sg entries that cross page
boundaries which are generated by kmalloc.
Please pull from
git://git.kernel.org/pub/scm/linux/kernel/git/herbert/crypto-2.6.git
or
master.kernel.org:/pub/scm/linux/kernel/git/herbert/crypto-2.6.git
Herbert Xu (1):
crypto: hash - Fix handling of sg entry that crosses page boundary
crypto/ahash.c |7 ---
1 files changed, 4 insertions(+), 3 deletions(-)
Thanks,
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: HMAC regression
On Sun, May 31, 2009 at 03:01:19PM +0200, Martin Willi wrote:
Yes, it fixes HMAC calculation with enabled SLAB debugging.
Thanks for confirming. I'll push the fix through.
--
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmVHI~} herb...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Use kzfree in crypto API context initialization and key/iv handling
David Miller wrote: From: Larry H. resea...@subreption.com Date: Sat, 30 May 2009 19:57:20 -0700 [PATCH] Use kzfree in crypto API context initialization and key/iv handling Thanks for not CC:ing the crypto list, and also not CC:'ing the crypto maintainer. Your submissions leave a lot to be desired, on every level. That's a pretty roundabout way of saying I have no technical objections :) -- All rights reversed. -- To unsubscribe from this list: send the line unsubscribe linux-crypto in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: linux-image-2.6.24-1-686: airo hang when loading because of aes
matthieu castet castet.matth...@free.fr wrote: airo driver hang with 2.6.24-6 on a PIII. It seems it it because it need aes crypto. It will first try to load padlock-aes, but it fails to load Then it load geode_aes which load, and airo hang (airo seems to use geode_aes). [1] [...] geode_aes is a PCI driver and should be automatically loaded if the PCI device it handles is present. However, since it also declares the alias aes, when the crypto system attempts to load the aes module it may load this module and then wait for up to a minute for it to initialise and register the aes algorithm. geode_aes will never do this, since if there was a suitable device present it would already have been loaded. (The padlock modules don't seem to have the same problem because their module initialisation functions immediately return failure if the CPU doesn't support.) I believe geode_aes can be fixed by either (1) removing the MODULE_ALIAS declaration or (2) making the module initialisation function fail if the device is not present. The latter behaviour is generally wrong for PCI drivers, but this device presumably cannot be hotplugged. Ben. -- Ben Hutchings Logic doesn't apply to the real world. - Marvin Minsky signature.asc Description: This is a digitally signed message part
