[patch 0/7] kzfree() v2

[patch 0/7] kzfree() v2

[Johannes Weiner]

This series introduces kzfree() and converts callsites which do
memset() + kzfree() explicitely.

The caller intention is to ensure that security-sensitive data are
cleared from slab objects before they are passed back to the
allocator.

This also removes the last modular ksize() user (crypto/api.c) again
by converting it to kzfree() which figures out the length of the
memory region to zero internally.

I left out drivers/w1/w1{,_int}.c and dropped the conversion of
drivers/atm/mpoa_caches.c in this iteration as I think they don't
strictly need the zeroeing and the memsetting should probably be
removed [ added Chas Williams and Evgeniy Polyakov to Cc ].

v2:
  - EXPORT_SYMBOL(kzfree), thanks linker
  - remove superfluous NULL checks, thanks Pekka
  - mention `security' in the description

	Hannes

 arch/s390/crypto/prng.c             |    3 +--
 crypto/api.c                        |    5 +----
 drivers/md/dm-crypt.c               |    6 ++----
 drivers/s390/crypto/zcrypt_pcixcc.c |    3 +--
 drivers/usb/host/hwa-hc.c           |    3 +--
 drivers/usb/wusbcore/cbaf.c         |    3 +--
 fs/cifs/connect.c                   |    6 +-----
 fs/cifs/misc.c                      |   10 ++--------
 fs/ecryptfs/keystore.c              |    3 +--
 fs/ecryptfs/messaging.c             |    3 +--
 include/linux/slab.h                |    1 +
 mm/util.c                           |   20 ++++++++++++++++++++
 12 files changed, 33 insertions(+), 33 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 1/7] slab: introduce kzfree()

[Johannes Weiner]

[-- Attachment #1: slab-introduce-kzfree.patch --]
[-- Type: text/plain, Size: 2015 bytes --]

kzfree() is a wrapper for kfree() that additionally zeroes the
underlying memory before releasing it to the slab allocator.

Currently there is code which memset()s the memory region of an object
before releasing it back to the slab allocator to make sure
security-sensitive data are really zeroed out after use.

These callsites can then just use kzfree() which saves some code,
makes users greppable and allows for a stupid destructor that isn't
necessarily aware of the actual object size.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Matt Mackall <mpm@selenic.com>
Cc: Christoph Lameter <cl@linux-foundation.org>
Cc: Nick Piggin <npiggin@suse.de>
---
 include/linux/slab.h |    1 +
 mm/util.c            |   20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -127,6 +127,7 @@ int kmem_ptr_validate(struct kmem_cache 
 void * __must_check __krealloc(const void *, size_t, gfp_t);
 void * __must_check krealloc(const void *, size_t, gfp_t);
 void kfree(const void *);
+void kzfree(const void *);
 size_t ksize(const void *);
 
 /*
--- a/mm/util.c
+++ b/mm/util.c
@@ -129,6 +129,26 @@ void *krealloc(const void *p, size_t new
 }
 EXPORT_SYMBOL(krealloc);
 
+/**
+ * kzfree - like kfree but zero memory
+ * @p: object to free memory of
+ *
+ * The memory of the object @p points to is zeroed before freed.
+ * If @p is %NULL, kzfree() does nothing.
+ */
+void kzfree(const void *p)
+{
+	size_t ks;
+	void *mem = (void *)p;
+
+	if (unlikely(ZERO_OR_NULL_PTR(mem)))
+		return;
+	ks = ksize(mem);
+	memset(mem, 0, ks);
+	kfree(mem);
+}
+EXPORT_SYMBOL(kzfree);
+
 /*
  * strndup_user - duplicate an existing string from user space
  * @s: The string to duplicate


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 2/7] crypto: use kzfree()

[Johannes Weiner]

[-- Attachment #1: crypto-use-kzfree.patch --]
[-- Type: text/plain, Size: 985 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
---
 crypto/api.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

--- a/crypto/api.c
+++ b/crypto/api.c
@@ -569,20 +569,17 @@ EXPORT_SYMBOL_GPL(crypto_alloc_tfm);
 void crypto_destroy_tfm(void *mem, struct crypto_tfm *tfm)
 {
 	struct crypto_alg *alg;
-	int size;
 
 	if (unlikely(!mem))
 		return;
 
 	alg = tfm->__crt_alg;
-	size = ksize(mem);
 
 	if (!tfm->exit && alg->cra_exit)
 		alg->cra_exit(tfm);
 	crypto_exit_ops(tfm);
 	crypto_mod_put(alg);
-	memset(mem, 0, size);
-	kfree(mem);
+	kzfree(mem);
 }
 EXPORT_SYMBOL_GPL(crypto_destroy_tfm);
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 3/7] s390: use kzfree()

[Johannes Weiner]

[-- Attachment #1: s390-use-kzfree.patch --]
[-- Type: text/plain, Size: 1224 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
---
 arch/s390/crypto/prng.c             |    3 +--
 drivers/s390/crypto/zcrypt_pcixcc.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/arch/s390/crypto/prng.c
+++ b/arch/s390/crypto/prng.c
@@ -201,8 +201,7 @@ out_free:
 static void __exit prng_exit(void)
 {
 	/* wipe me */
-	memset(p->buf, 0, prng_chunk_size);
-	kfree(p->buf);
+	kzfree(p->buf);
 	kfree(p);
 
 	misc_deregister(&prng_dev);
--- a/drivers/s390/crypto/zcrypt_pcixcc.c
+++ b/drivers/s390/crypto/zcrypt_pcixcc.c
@@ -781,8 +781,7 @@ static long zcrypt_pcixcc_send_cprb(stru
 		/* Signal pending. */
 		ap_cancel_message(zdev->ap_dev, &ap_msg);
 out_free:
-	memset(ap_msg.message, 0x0, ap_msg.length);
-	kfree(ap_msg.message);
+	kzfree(ap_msg.message);
 	return rc;
 }
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 4/7] md: use kzfree()

[Johannes Weiner]

[-- Attachment #1: md-use-kzfree.patch --]
[-- Type: text/plain, Size: 1088 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Alasdair Kergon <dm-devel@redhat.com>
---
 drivers/md/dm-crypt.c |    6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1137,8 +1137,7 @@ bad_ivmode:
 	crypto_free_ablkcipher(tfm);
 bad_cipher:
 	/* Must zero key material before freeing */
-	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
-	kfree(cc);
+	kzfree(cc);
 	return -EINVAL;
 }
 
@@ -1164,8 +1163,7 @@ static void crypt_dtr(struct dm_target *
 	dm_put_device(ti, cc->dev);
 
 	/* Must zero key material before freeing */
-	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
-	kfree(cc);
+	kzfree(cc);
 }
 
 static int crypt_map(struct dm_target *ti, struct bio *bio,


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 5/7] usb: use kzfree()

[Johannes Weiner]

[-- Attachment #1: usb-use-kzfree.patch --]
[-- Type: text/plain, Size: 1221 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Cc: Greg Kroah-Hartman <gregkh@suse.de>
---
 drivers/usb/host/hwa-hc.c   |    3 +--
 drivers/usb/wusbcore/cbaf.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/drivers/usb/host/hwa-hc.c
+++ b/drivers/usb/host/hwa-hc.c
@@ -464,8 +464,7 @@ static int __hwahc_dev_set_key(struct wu
 			port_idx << 8 | iface_no,
 			keyd, keyd_len, 1000 /* FIXME: arbitrary */);
 
-	memset(keyd, 0, sizeof(*keyd));	/* clear keys etc. */
-	kfree(keyd);
+	kzfree(keyd); /* clear keys etc. */
 	return result;
 }
 
--- a/drivers/usb/wusbcore/cbaf.c
+++ b/drivers/usb/wusbcore/cbaf.c
@@ -638,8 +638,7 @@ static void cbaf_disconnect(struct usb_i
 	usb_put_intf(iface);
 	kfree(cbaf->buffer);
 	/* paranoia: clean up crypto keys */
-	memset(cbaf, 0, sizeof(*cbaf));
-	kfree(cbaf);
+	kzfree(cbaf);
 }
 
 static struct usb_device_id cbaf_id_table[] = {


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 6/7] cifs: use kzfree()

[Johannes Weiner]

[-- Attachment #1: cifs-use-kzfree.patch --]
[-- Type: text/plain, Size: 1739 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Acked-by: Steve French <sfrench@samba.org>
---
 fs/cifs/connect.c |    6 +-----
 fs/cifs/misc.c    |   10 ++--------
 2 files changed, 3 insertions(+), 13 deletions(-)

--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -2433,11 +2433,7 @@ mount_fail_check:
 out:
 	/* zero out password before freeing */
 	if (volume_info) {
-		if (volume_info->password != NULL) {
-			memset(volume_info->password, 0,
-				strlen(volume_info->password));
-			kfree(volume_info->password);
-		}
+		kzfree(volume_info->password);
 		kfree(volume_info->UNC);
 		kfree(volume_info->prepath);
 		kfree(volume_info);
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -97,10 +97,7 @@ sesInfoFree(struct cifsSesInfo *buf_to_f
 	kfree(buf_to_free->serverOS);
 	kfree(buf_to_free->serverDomain);
 	kfree(buf_to_free->serverNOS);
-	if (buf_to_free->password) {
-		memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-		kfree(buf_to_free->password);
-	}
+	kzfree(buf_to_free->password);
 	kfree(buf_to_free->domainName);
 	kfree(buf_to_free);
 }
@@ -132,10 +129,7 @@ tconInfoFree(struct cifsTconInfo *buf_to
 	}
 	atomic_dec(&tconInfoAllocCount);
 	kfree(buf_to_free->nativeFileSystem);
-	if (buf_to_free->password) {
-		memset(buf_to_free->password, 0, strlen(buf_to_free->password));
-		kfree(buf_to_free->password);
-	}
+	kzfree(buf_to_free->password);
 	kfree(buf_to_free);
 }
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[patch 7/7] ecryptfs: use kzfree()

[Johannes Weiner]

[-- Attachment #1: ecryptfs-use-kzfree.patch --]
[-- Type: text/plain, Size: 1250 bytes --]

Use kzfree() instead of memset() + kfree().

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
Acked-by: Tyler Hicks <tyhicks@linux.vnet.ibm.com>
---
 fs/ecryptfs/keystore.c  |    3 +--
 fs/ecryptfs/messaging.c |    3 +--
 2 files changed, 2 insertions(+), 4 deletions(-)

--- a/fs/ecryptfs/keystore.c
+++ b/fs/ecryptfs/keystore.c
@@ -740,8 +740,7 @@ ecryptfs_write_tag_70_packet(char *dest,
 out_release_free_unlock:
 	crypto_free_hash(s->hash_desc.tfm);
 out_free_unlock:
-	memset(s->block_aligned_filename, 0, s->block_aligned_filename_size);
-	kfree(s->block_aligned_filename);
+	kzfree(s->block_aligned_filename);
 out_unlock:
 	mutex_unlock(s->tfm_mutex);
 out:
--- a/fs/ecryptfs/messaging.c
+++ b/fs/ecryptfs/messaging.c
@@ -291,8 +291,7 @@ int ecryptfs_exorcise_daemon(struct ecry
 	if (daemon->user_ns)
 		put_user_ns(daemon->user_ns);
 	mutex_unlock(&daemon->mux);
-	memset(daemon, 0, sizeof(*daemon));
-	kfree(daemon);
+	kzfree(daemon);
 out:
 	return rc;
 }


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Christoph Lameter]

Acked-by: Christoph Lameter <cl@linux-foundation.org>


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[David Vrabel]

Johannes Weiner wrote:
> +void kzfree(const void *p)

Shouldn't this be void * since it writes to the memory?

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 5/7] usb: use kzfree()

[David Vrabel]

This is in the WUSB code so:

Acked-by: David Vrabel <david.vrabel@csr.com>

David
-- 
David Vrabel, Senior Software Engineer, Drivers
CSR, Churchill House, Cambridge Business Park,  Tel: +44 (0)1223 692562
Cowley Road, Cambridge, CB4 0WZ                 http://www.csr.com/

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Pekka Enberg]

On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
> Johannes Weiner wrote:
> > +void kzfree(const void *p)
> 
> Shouldn't this be void * since it writes to the memory?

No. kfree() writes to the memory as well to update freelists, poisoning
and such so kzfree() is not at all different from it.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[KOSAKI Motohiro]

> On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
> > Johannes Weiner wrote:
> > > +void kzfree(const void *p)
> > 
> > Shouldn't this be void * since it writes to the memory?
> 
> No. kfree() writes to the memory as well to update freelists, poisoning
> and such so kzfree() is not at all different from it.

I don't think so. It's debetable thing.

poisonig is transparent feature from caller.
but the caller of kzfree() know to fill memory and it should know.



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Pekka Enberg]

On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
> > > Johannes Weiner wrote:
> > > > +void kzfree(const void *p)
> > > 
> > > Shouldn't this be void * since it writes to the memory?
> > 
> > No. kfree() writes to the memory as well to update freelists, poisoning
> > and such so kzfree() is not at all different from it.

On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
> I don't think so. It's debetable thing.
> 
> poisonig is transparent feature from caller.
> but the caller of kzfree() know to fill memory and it should know.

Debatable, sure, but doesn't seem like a big enough reason to make
kzfree() differ from kfree().

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[KOSAKI Motohiro]

2009/2/19 Pekka Enberg <penberg@cs.helsinki.fi>:
> On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
>> > > Johannes Weiner wrote:
>> > > > +void kzfree(const void *p)
>> > >
>> > > Shouldn't this be void * since it writes to the memory?
>> >
>> > No. kfree() writes to the memory as well to update freelists, poisoning
>> > and such so kzfree() is not at all different from it.
>
> On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
>> I don't think so. It's debetable thing.
>>
>> poisonig is transparent feature from caller.
>> but the caller of kzfree() know to fill memory and it should know.
>
> Debatable, sure, but doesn't seem like a big enough reason to make
> kzfree() differ from kfree().

Sure.
ok, I don't oppse this :)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Hugh Dickins]

On Thu, 19 Feb 2009, Pekka Enberg wrote:
> On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
> > > > Johannes Weiner wrote:
> > > > > +void kzfree(const void *p)
> > > > 
> > > > Shouldn't this be void * since it writes to the memory?
> > > 
> > > No. kfree() writes to the memory as well to update freelists, poisoning
> > > and such so kzfree() is not at all different from it.
> 
> On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
> > I don't think so. It's debetable thing.
> > 
> > poisonig is transparent feature from caller.
> > but the caller of kzfree() know to fill memory and it should know.
> 
> Debatable, sure, but doesn't seem like a big enough reason to make
> kzfree() differ from kfree().

There may be more important things for us to worry about,
but I do strongly agree with KOSAKI-san on this.

kzfree() already differs from kfree() by a "z": that "z" says please
zero the buffer pointed to; "const" says it won't modify the buffer
pointed to.  What sense does kzfree(const void *) make?  Why is
keeping the declarations the same apart from the "z" desirable?

By all means refuse to add kzfree(), but please don't add it with const.

I can see that the "const" in kfree(const void *) is debatable
[looks to see how userspace free() is defined: without a const],
I can see that it might be nice to have some "goesaway" attribute
for such pointers instead; but I don't see how you can argue for
kzalloc(const void *).

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Matt Mackall]

On Thu, 2009-02-19 at 16:34 +0000, Hugh Dickins wrote:
> On Thu, 19 Feb 2009, Pekka Enberg wrote:
> > On Wed, 2009-02-18 at 10:50 +0000, David Vrabel wrote:
> > > > > Johannes Weiner wrote:
> > > > > > +void kzfree(const void *p)
> > > > > 
> > > > > Shouldn't this be void * since it writes to the memory?
> > > > 
> > > > No. kfree() writes to the memory as well to update freelists, poisoning
> > > > and such so kzfree() is not at all different from it.
> > 
> > On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
> > > I don't think so. It's debetable thing.
> > > 
> > > poisonig is transparent feature from caller.
> > > but the caller of kzfree() know to fill memory and it should know.
> > 
> > Debatable, sure, but doesn't seem like a big enough reason to make
> > kzfree() differ from kfree().
> 
> There may be more important things for us to worry about,
> but I do strongly agree with KOSAKI-san on this.
> 
> kzfree() already differs from kfree() by a "z": that "z" says please
> zero the buffer pointed to; "const" says it won't modify the buffer
> pointed to.  What sense does kzfree(const void *) make?  Why is
> keeping the declarations the same apart from the "z" desirable?
> 
> By all means refuse to add kzfree(), but please don't add it with const.
> 
> I can see that the "const" in kfree(const void *) is debatable
> [looks to see how userspace free() is defined: without a const],
> I can see that it might be nice to have some "goesaway" attribute
> for such pointers instead; but I don't see how you can argue for
> kzalloc(const void *).

This is what Linus said last time this came up:

http://lkml.org/lkml/2008/1/16/227

-- 
http://selenic.com : development and support for Mercurial and Linux


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Hugh Dickins]

On Thu, 19 Feb 2009, Matt Mackall wrote:
> On Thu, 2009-02-19 at 16:34 +0000, Hugh Dickins wrote:
> > On Thu, 19 Feb 2009, Pekka Enberg wrote:
> > > On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
> > > > 
> > > > poisonig is transparent feature from caller.
> > > > but the caller of kzfree() know to fill memory and it should know.
> > > 
> > > Debatable, sure, but doesn't seem like a big enough reason to make
> > > kzfree() differ from kfree().
> > 
> > There may be more important things for us to worry about,
> > but I do strongly agree with KOSAKI-san on this.
> > 
> > kzfree() already differs from kfree() by a "z": that "z" says please
> > zero the buffer pointed to; "const" says it won't modify the buffer
> > pointed to.  What sense does kzfree(const void *) make?  Why is
> > keeping the declarations the same apart from the "z" desirable?
> > 
> > By all means refuse to add kzfree(), but please don't add it with const.
> > 
> > I can see that the "const" in kfree(const void *) is debatable
> > [looks to see how userspace free() is defined: without a const],
> > I can see that it might be nice to have some "goesaway" attribute
> > for such pointers instead; but I don't see how you can argue for
> > kzalloc(const void *).
    ^^^^^^^^^^^^^^^^^^^^^
(Of course I meant to say "kzfree(const void *)" there.)

> 
> This is what Linus said last time this came up:
> 
> http://lkml.org/lkml/2008/1/16/227

Thanks for that, I remember it now.

Okay, that's some justification for kfree(const void *).

But I fail to see it as a justification for kzfree(const void *):
if someone has "const char *string = kmalloc(size)" and then
wants that string zeroed before it is freed, then I think it's
quite right to cast out the const when calling kzfree().

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Pekka Enberg]

Hi Hugh.

Hugh Dickins wrote:
> Thanks for that, I remember it now.
> 
> Okay, that's some justification for kfree(const void *).
> 
> But I fail to see it as a justification for kzfree(const void *):
> if someone has "const char *string = kmalloc(size)" and then
> wants that string zeroed before it is freed, then I think it's
> quite right to cast out the const when calling kzfree().

Quite frankly, I fail to see how kzfree() is fundamentally different 
from kfree(). I don't see kzfree() as a memset() + kfree() but rather as 
a kfree() "and make sure no one sees my data". So the zeroing happens 
_after_ you've invalidated the pointer with kzfree() so there's no 
"zeroing of buffer going on". So the way I see it, Linus' argument for 
having const for kfree() applies to kzfree().

That said, if you guys think it's a merge blocker, by all means remove 
the const. I just want few less open-coded ksize() users, that's all.

			Pekka

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Johannes Weiner]

On Thu, Feb 19, 2009 at 06:28:55PM +0000, Hugh Dickins wrote:
> On Thu, 19 Feb 2009, Matt Mackall wrote:
> > On Thu, 2009-02-19 at 16:34 +0000, Hugh Dickins wrote:
> > > On Thu, 19 Feb 2009, Pekka Enberg wrote:
> > > > On Thu, 2009-02-19 at 10:22 +0900, KOSAKI Motohiro wrote:
> > > > > 
> > > > > poisonig is transparent feature from caller.
> > > > > but the caller of kzfree() know to fill memory and it should know.
> > > > 
> > > > Debatable, sure, but doesn't seem like a big enough reason to make
> > > > kzfree() differ from kfree().
> > > 
> > > There may be more important things for us to worry about,
> > > but I do strongly agree with KOSAKI-san on this.
> > > 
> > > kzfree() already differs from kfree() by a "z": that "z" says please
> > > zero the buffer pointed to; "const" says it won't modify the buffer
> > > pointed to.  What sense does kzfree(const void *) make?  Why is
> > > keeping the declarations the same apart from the "z" desirable?
> > > 
> > > By all means refuse to add kzfree(), but please don't add it with const.
> > > 
> > > I can see that the "const" in kfree(const void *) is debatable
> > > [looks to see how userspace free() is defined: without a const],
> > > I can see that it might be nice to have some "goesaway" attribute
> > > for such pointers instead; but I don't see how you can argue for
> > > kzalloc(const void *).
>     ^^^^^^^^^^^^^^^^^^^^^
> (Of course I meant to say "kzfree(const void *)" there.)
> 
> > 
> > This is what Linus said last time this came up:
> > 
> > http://lkml.org/lkml/2008/1/16/227
> 
> Thanks for that, I remember it now.
> 
> Okay, that's some justification for kfree(const void *).
> 
> But I fail to see it as a justification for kzfree(const void *):
> if someone has "const char *string = kmalloc(size)" and then
> wants that string zeroed before it is freed, then I think it's
> quite right to cast out the const when calling kzfree().

You could argue that the pointer passed to kzfree() points to an
abstract slab object and kzfree() uses this to find the memory of that
object which it then zeroes.  The translation of course is a no-op as
the object pointer and the memory pointer coincide.

It depends on how transparent you want to make kzfree() for the
caller.  Is it 'zero out and then free the object' or is it 'free the
object, but note that it contains security-sensitive data, so make
sure that it never gets into the hands of somebody else'?

No strong opinion from me, though, I can not say which one feels
better.  I made it intuitively const, so I guess I would lean to the
more opaque version of the function.

> Hugh

	Hannes

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Hugh Dickins]

On Thu, 19 Feb 2009, Pekka Enberg wrote:
> Hugh Dickins wrote:
> > 
> > But I fail to see it as a justification for kzfree(const void *):
> > if someone has "const char *string = kmalloc(size)" and then
> > wants that string zeroed before it is freed, then I think it's
> > quite right to cast out the const when calling kzfree().
> 
> Quite frankly, I fail to see how kzfree() is fundamentally different from
> kfree(). I don't see kzfree() as a memset() + kfree() but rather as a kfree()
> "and make sure no one sees my data". So the zeroing happens _after_ you've
> invalidated the pointer with kzfree() so there's no "zeroing of buffer going
> on".

Well, that would be one way of picturing it, yes.
Imagine the "z" as for "zap" rather than "zero",
and the mechanism as opaque as Hannes suggests.

> So the way I see it, Linus' argument for having const for kfree() applies
> to kzfree().
> 
> That said, if you guys think it's a merge blocker, by all means remove the
> const. I just want few less open-coded ksize() users, that's all.

I wouldn't call it a merge blocker, no; though I still
think it makes far more sense without the "const" there.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 2/7] crypto: use kzfree()

[Herbert Xu]

Johannes Weiner <hannes@cmpxchg.org> wrote:
> Use kzfree() instead of memset() + kfree().
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> Reviewed-by: Pekka Enberg <penberg@cs.helsinki.fi>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>

Acked-by: Herbert Xu <herbert@gondor.apana.org.au>

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Nick Piggin]

On Friday 20 February 2009 07:36:48 Hugh Dickins wrote:
> On Thu, 19 Feb 2009, Pekka Enberg wrote:

> > Quite frankly, I fail to see how kzfree() is fundamentally different from
> > kfree(). I don't see kzfree() as a memset() + kfree() but rather as a
> > kfree() "and make sure no one sees my data". So the zeroing happens
> > _after_ you've invalidated the pointer with kzfree() so there's no
> > "zeroing of buffer going on".
>
> Well, that would be one way of picturing it, yes.
> Imagine the "z" as for "zap" rather than "zero",
> and the mechanism as opaque as Hannes suggests.
>
> > So the way I see it, Linus' argument for having const for kfree() applies
> > to kzfree().
> >
> > That said, if you guys think it's a merge blocker, by all means remove
> > the const. I just want few less open-coded ksize() users, that's all.
>
> I wouldn't call it a merge blocker, no; though I still
> think it makes far more sense without the "const" there.

Well, the buffer is only non-modified in the case of one of the
allocators (SLAB). All others overwrite some of the data region
with their own metadata.

I think it is OK to use const, though. Because k(z)free has the
knowledge that the data will not be touched by the caller any
longer.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Hugh Dickins]

On Tue, 24 Feb 2009, Nick Piggin wrote:
> 
> Well, the buffer is only non-modified in the case of one of the
> allocators (SLAB). All others overwrite some of the data region
> with their own metadata.
> 
> I think it is OK to use const, though. Because k(z)free has the
> knowledge that the data will not be touched by the caller any
> longer.

Sorry, you're not adding anything new to the thread here.

Yes, the caller is surrendering the buffer, so we can get
away with calling the argument const; and Linus argues that's
helpful in the case of kfree (to allow passing a const pointer
without having to cast it).

My contention is that kzfree(const void *ptr) is nonsensical
because it says please zero this buffer without modifying it.

But the change has gone in, I seem to be the only one still
bothered by it, and I've conceded that the "z" might stand
for zap rather than zero.

So it may be saying please hide the contents of this buffer,
rather than please zero it.  And then it can be argued that
the modification is an implementation detail which happens
(like other housekeeping internal to the sl?b allocator)
only after the original buffer has been freed.

Philosophy.

Hugh

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Nick Piggin]

On Tuesday 24 February 2009 01:51:05 Hugh Dickins wrote:
> On Tue, 24 Feb 2009, Nick Piggin wrote:
> > Well, the buffer is only non-modified in the case of one of the
> > allocators (SLAB). All others overwrite some of the data region
> > with their own metadata.
> >
> > I think it is OK to use const, though. Because k(z)free has the
> > knowledge that the data will not be touched by the caller any
> > longer.
>
> Sorry, you're not adding anything new to the thread here.
>
> Yes, the caller is surrendering the buffer, so we can get
> away with calling the argument const; and Linus argues that's
> helpful in the case of kfree (to allow passing a const pointer
> without having to cast it).

(Yes, not that I agree his argument is strong enough to be able
to call libc's definition wrong)

> My contention is that kzfree(const void *ptr) is nonsensical
> because it says please zero this buffer without modifying it.
>
> But the change has gone in, I seem to be the only one still
> bothered by it, and I've conceded that the "z" might stand
> for zap rather than zero.
>
> So it may be saying please hide the contents of this buffer,
> rather than please zero it.  And then it can be argued that
> the modification is an implementation detail which happens
> (like other housekeeping internal to the sl?b allocator)
> only after the original buffer has been freed.
>
> Philosophy.

Hmm, well it better if kzfree is defined to zap rather than zero
anyway. zap is a better definition because it theoretically allows
the implementation to do something else (poision it with some
other value; mark it as zapped and don't reallocate it without
zeroing it; etc). And also it doesn't imply that the caller still
cares about what it actually gets filled with.


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [patch 1/7] slab: introduce kzfree()

[Andrew Morton]

On Mon, 23 Feb 2009 14:51:05 +0000 (GMT)
Hugh Dickins <hugh@veritas.com> wrote:

> On Tue, 24 Feb 2009, Nick Piggin wrote:
> > 
> > Well, the buffer is only non-modified in the case of one of the
> > allocators (SLAB). All others overwrite some of the data region
> > with their own metadata.
> > 
> > I think it is OK to use const, though. Because k(z)free has the
> > knowledge that the data will not be touched by the caller any
> > longer.
> 
> Sorry, you're not adding anything new to the thread here.
> 
> Yes, the caller is surrendering the buffer, so we can get
> away with calling the argument const; and Linus argues that's
> helpful in the case of kfree (to allow passing a const pointer
> without having to cast it).
> 
> My contention is that kzfree(const void *ptr) is nonsensical
> because it says please zero this buffer without modifying it.

yup.  The intent of kzfree() is explicitly, overtly, deliberately to
modify the passed memory before freeing it.  Marking it const is dopey.

But the const marker is potentially useful to some caller.  An arguably
misdesigned caller.

> But the change has gone in, I seem to be the only one still
> bothered by it, and I've conceded that the "z" might stand
> for zap rather than zero.

Yeah.  But it's a very small bother.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>