[Patch 2.6] dm-crypt: zero key before freeing it

[Patch 2.6] dm-crypt: zero key before freeing it

[Stefan Rompf]

Hi Andrew,

dm-crypt does not clear struct crypt_config before freeing it. Thus, 
information on the key could leak f.e. to a swsusp image even after the 
encrypted device has been removed. The attached patch against 2.6.14 / 2.6.15 
fixes it.

Signed-off-by: Stefan Rompf <stefan@loplof.de>
Acked-by: Clemens Fruhwirth <clemens@endorphin.org>

--- linux-2.6.14.4/drivers/md/dm-crypt.c.old	2005-12-16 18:27:05.000000000 +0100
+++ linux-2.6.14.4/drivers/md/dm-crypt.c	2005-12-28 12:49:13.000000000 +0100
@@ -694,6 +694,7 @@ bad3:
 bad2:
 	crypto_free_tfm(tfm);
 bad1:
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 	return -EINVAL;
 }
@@ -710,6 +711,7 @@ static void crypt_dtr(struct dm_target *
 		cc->iv_gen_ops->dtr(cc);
 	crypto_free_tfm(cc->tfm);
 	dm_put_device(ti, cc->dev);
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 }
 

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Arjan van de Ven]

On Wed, 2006-01-04 at 21:08 +0100, Stefan Rompf wrote:
> Hi Andrew,
> 
> dm-crypt does not clear struct crypt_config before freeing it. Thus, 
> information on the key could leak f.e. to a swsusp image even after the 
> encrypted device has been removed. The attached patch against 2.6.14 / 2.6.15 
> fixes it.

since a memset right before a free is a very unusual code pattern in the
kernel it may well be worth putting a short comment around it to prevent
someone later removing it as "optimization"

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Stefan Rompf]

Am Mittwoch 04 Januar 2006 21:09 schrieb Arjan van de Ven:

> since a memset right before a free is a very unusual code pattern in the
> kernel it may well be worth putting a short comment around it to prevent
> someone later removing it as "optimization"

Valid objection, here is an update (and see, I'm running 2.6.15 now ;-)

Stefan


Signed-off-by: Stefan Rompf <stefan@loplof.de>
Acked-by: Clemens Fruhwirth <clemens@endorphin.org>

--- linux-2.6.15/drivers/md/dm-crypt.c.orig	2006-01-04 01:01:16.000000000 +0100
+++ linux-2.6.15/drivers/md/dm-crypt.c	2006-01-04 21:23:34.000000000 +0100
@@ -690,6 +690,8 @@
 bad2:
 	crypto_free_tfm(tfm);
 bad1:
+	/* Must zero key material before freeing */
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 	return -EINVAL;
 }
@@ -706,6 +708,9 @@
 		cc->iv_gen_ops->dtr(cc);
 	crypto_free_tfm(cc->tfm);
 	dm_put_device(ti, cc->dev);
+
+	/* Must zero key material before freeing */
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 }
 

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Randy.Dunlap]

On Wed, 4 Jan 2006, Stefan Rompf wrote:

> Am Mittwoch 04 Januar 2006 21:09 schrieb Arjan van de Ven:
>
> > since a memset right before a free is a very unusual code pattern in the
> > kernel it may well be worth putting a short comment around it to prevent
> > someone later removing it as "optimization"
>
> Valid objection, here is an update (and see, I'm running 2.6.15 now ;-)

A reason "why" would be more helpful that a "what".


> Signed-off-by: Stefan Rompf <stefan@loplof.de>
> Acked-by: Clemens Fruhwirth <clemens@endorphin.org>
>
> --- linux-2.6.15/drivers/md/dm-crypt.c.orig	2006-01-04 01:01:16.000000000 +0100
> +++ linux-2.6.15/drivers/md/dm-crypt.c	2006-01-04 21:23:34.000000000 +0100
> @@ -690,6 +690,8 @@
>  bad2:
>  	crypto_free_tfm(tfm);
>  bad1:
> +	/* Must zero key material before freeing */
> +	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
>  	kfree(cc);
>  	return -EINVAL;
>  }
> @@ -706,6 +708,9 @@
>  		cc->iv_gen_ops->dtr(cc);
>  	crypto_free_tfm(cc->tfm);
>  	dm_put_device(ti, cc->dev);
> +
> +	/* Must zero key material before freeing */
> +	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
>  	kfree(cc);
>  }
>
> -

-- 
~Randy

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Jörn Engel]

On Wed, 4 January 2006 12:28:59 -0800, Randy.Dunlap wrote:
> On Wed, 4 Jan 2006, Stefan Rompf wrote:
> > Am Mittwoch 04 Januar 2006 21:09 schrieb Arjan van de Ven:
> >
> > > since a memset right before a free is a very unusual code pattern in the
> > > kernel it may well be worth putting a short comment around it to prevent
> > > someone later removing it as "optimization"
> >
> > Valid objection, here is an update (and see, I'm running 2.6.15 now ;-)
> 
> A reason "why" would be more helpful that a "what".

"prevent information leak"

This is still a "what", but at least not a "how".

Jörn

-- 
Linux is more the core point of a concept that surrounds "open source"
which, in turn, is based on a false concept. This concept is that
people actually want to look at source code.
-- Rob Enderle

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Randy.Dunlap]

On Wed, 4 Jan 2006, Jörn Engel wrote:

> On Wed, 4 January 2006 12:28:59 -0800, Randy.Dunlap wrote:
> > On Wed, 4 Jan 2006, Stefan Rompf wrote:
> > > Am Mittwoch 04 Januar 2006 21:09 schrieb Arjan van de Ven:
> > >
> > > > since a memset right before a free is a very unusual code pattern in the
> > > > kernel it may well be worth putting a short comment around it to prevent
> > > > someone later removing it as "optimization"
> > >
> > > Valid objection, here is an update (and see, I'm running 2.6.15 now ;-)
> >
> > A reason "why" would be more helpful that a "what".
>
> "prevent information leak"
>
> This is still a "what", but at least not a "how".

OK, that's a much better changelog entry or source code comment...
if it could be put in one of those places.

Thanks.
-- 
~Randy

Re: [stable] Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Greg KH]

On Wed, Jan 04, 2006 at 12:43:21PM -0800, Randy.Dunlap wrote:
> On Wed, 4 Jan 2006, J?rn Engel wrote:
> 
> > On Wed, 4 January 2006 12:28:59 -0800, Randy.Dunlap wrote:
> > > On Wed, 4 Jan 2006, Stefan Rompf wrote:
> > > > Am Mittwoch 04 Januar 2006 21:09 schrieb Arjan van de Ven:
> > > >
> > > > > since a memset right before a free is a very unusual code pattern in the
> > > > > kernel it may well be worth putting a short comment around it to prevent
> > > > > someone later removing it as "optimization"
> > > >
> > > > Valid objection, here is an update (and see, I'm running 2.6.15 now ;-)
> > >
> > > A reason "why" would be more helpful that a "what".
> >
> > "prevent information leak"
> >
> > This is still a "what", but at least not a "how".
> 
> OK, that's a much better changelog entry or source code comment...
> if it could be put in one of those places.

Yes, Stefan, care to redo this with an updated changelog command?

thanks,

greg k-h

[Patch 2.6] dm-crypt: Zero key material before free to avoid information leak

[Stefan Rompf]

Am Mittwoch 04 Januar 2006 22:15 schrieb Greg KH:

> Yes, Stefan, care to redo this with an updated changelog command?


dm-crypt should clear struct crypt_config before freeing it to
avoid information leak f.e. to a swsusp image.

Signed-off-by: Stefan Rompf <stefan@loplof.de>
Acked-by: Clemens Fruhwirth <clemens@endorphin.org>

--- linux-2.6.15/drivers/md/dm-crypt.c.orig	2006-01-04 01:01:16.000000000 +0100
+++ linux-2.6.15/drivers/md/dm-crypt.c	2006-01-04 22:35:13.000000000 +0100
@@ -690,6 +690,8 @@
 bad2:
 	crypto_free_tfm(tfm);
 bad1:
+	/* Zero key material before free to avoid information leak */
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 	return -EINVAL;
 }
@@ -706,6 +708,9 @@
 		cc->iv_gen_ops->dtr(cc);
 	crypto_free_tfm(cc->tfm);
 	dm_put_device(ti, cc->dev);
+
+	/* Zero key material before free to avoid information leak */
+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
 	kfree(cc);
 }
 

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Neil Brown]

>
>Hi Andrew,
>
>dm-crypt does not clear struct crypt_config before freeing it. Thus, 
>information on the key could leak f.e. to a swsusp image even after the 
>encrypted device has been removed. The attached patch against 2.6.14 / 2.6.15 
>fixes it.
>
>Signed-off-by: Stefan Rompf <stefan@loplof.de>
>Acked-by: Clemens Fruhwirth <clemens@endorphin.org>
>
>--- linux-2.6.14.4/drivers/md/dm-crypt.c.old	2005-12-16 18:27:05.000000000 +0100
>+++ linux-2.6.14.4/drivers/md/dm-crypt.c	2005-12-28 12:49:13.000000000 +0100
>@@ -694,6 +694,7 @@ bad3:
> bad2:
> 	crypto_free_tfm(tfm);
> bad1:
>+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
> 	kfree(cc);
> 	return -EINVAL;
> }

There is a small problem with this patch.
If the 'goto bad1' branch is taken, then 'cc->key_size' will not be
defined.
I think you need the following patch on top.

(Is that "sizeof(u8)" *really* necessary?? :-)

NeilBrown


Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./drivers/md/dm-crypt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff ./drivers/md/dm-crypt.c~current~ ./drivers/md/dm-crypt.c
--- ./drivers/md/dm-crypt.c~current~	2006-01-24 15:42:52.000000000 +1100
+++ ./drivers/md/dm-crypt.c	2006-01-24 15:43:07.000000000 +1100
@@ -691,7 +691,7 @@ bad2:
 	crypto_free_tfm(tfm);
 bad1:
 	/* Must zero key material before freeing */
-	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
+	memset(cc, 0, sizeof(*cc) + key_size * sizeof(u8));
 	kfree(cc);
 	return -EINVAL;
 }

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Stefan Rompf]

Am Dienstag 24 Januar 2006 05:49 schrieb Neil Brown:

> >--- linux-2.6.14.4/drivers/md/dm-crypt.c.old	2005-12-16 18:27:05.000000000
> > +0100 +++ linux-2.6.14.4/drivers/md/dm-crypt.c	2005-12-28
> > 12:49:13.000000000 +0100 @@ -694,6 +694,7 @@ bad3:
> > bad2:
> > 	crypto_free_tfm(tfm);
> > bad1:
> >+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
> > 	kfree(cc);
> > 	return -EINVAL;
> > }
>
> There is a small problem with this patch.
> If the 'goto bad1' branch is taken, then 'cc->key_size' will not be
> defined.
> I think you need the following patch on top.

Why? This is from today's git, just before the first goto bad1

 559         cc->key_size = key_size;
 560         if ((!key_size && strcmp(argv[1], "-") != 0) ||
 561             (key_size && crypt_decode_key(cc->key, argv[1], key_size) < 
0)) {
 562                 ti->error = PFX "Error decoding key";
 563                 goto bad1;
 564         }

Stefan

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Neil Brown]

On Tuesday January 24, stefan@loplof.de wrote:
> Am Dienstag 24 Januar 2006 05:49 schrieb Neil Brown:
> 
> > >--- linux-2.6.14.4/drivers/md/dm-crypt.c.old	2005-12-16 18:27:05.000000000
> > > +0100 +++ linux-2.6.14.4/drivers/md/dm-crypt.c	2005-12-28
> > > 12:49:13.000000000 +0100 @@ -694,6 +694,7 @@ bad3:
> > > bad2:
> > > 	crypto_free_tfm(tfm);
> > > bad1:
> > >+	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
> > > 	kfree(cc);
> > > 	return -EINVAL;
> > > }
> >
> > There is a small problem with this patch.
> > If the 'goto bad1' branch is taken, then 'cc->key_size' will not be
> > defined.
> > I think you need the following patch on top.
> 
> Why? This is from today's git, just before the first goto bad1
> 
>  559         cc->key_size = key_size;
>  560         if ((!key_size && strcmp(argv[1], "-") != 0) ||
>  561             (key_size && crypt_decode_key(cc->key, argv[1], key_size) < 
> 0)) {
>  562                 ti->error = PFX "Error decoding key";
>  563                 goto bad1;
>  564         }
> 
> Stefan


Ahhh.... sorry, 'bout that.  You are right.  I was looking at an
older kernel and assumed that bit of code hadn't been re-arrange...
My bad.  Pardon the noise.

NeilBrown

Re: [Patch 2.6] dm-crypt: zero key before freeing it

[Phillip Susi]

Once the page is placed on the free list, doesn't that prevent it from 
being swapped out?  swsusp doesn't bother saving free pages and before 
the pages can be recycled, the kernel zeros them right?

Stefan Rompf wrote:
> Hi Andrew,
>
> dm-crypt does not clear struct crypt_config before freeing it. Thus, 
> information on the key could leak f.e. to a swsusp image even after the 
> encrypted device has been removed. The attached patch against 2.6.14 / 2.6.15 
> fixes it.
>
> Signed-off-by: Stefan Rompf <stefan@loplof.de>
> Acked-by: Clemens Fruhwirth <clemens@endorphin.org>
>
> --- linux-2.6.14.4/drivers/md/dm-crypt.c.old	2005-12-16 18:27:05.000000000 +0100
> +++ linux-2.6.14.4/drivers/md/dm-crypt.c	2005-12-28 12:49:13.000000000 +0100
> @@ -694,6 +694,7 @@ bad3:
>  bad2:
>  	crypto_free_tfm(tfm);
>  bad1:
> +	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
>  	kfree(cc);
>  	return -EINVAL;
>  }
> @@ -710,6 +711,7 @@ static void crypt_dtr(struct dm_target *
>  		cc->iv_gen_ops->dtr(cc);
>  	crypto_free_tfm(cc->tfm);
>  	dm_put_device(ti, cc->dev);
> +	memset(cc, 0, sizeof(*cc) + cc->key_size * sizeof(u8));
>  	kfree(cc);
>  }
>  
>
> -
>
>