* [Patch 2.6] dm-crypt: zero key before freeing it
@ 2006-01-04 20:08 Stefan Rompf
2006-01-04 20:09 ` Arjan van de Ven
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Stefan Rompf @ 2006-01-04 20:08 UTC (permalink / raw)
To: Andrew Morton; +Cc: Clemens Fruhwirth, linux-kernel, stable
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);
}
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:08 [Patch 2.6] dm-crypt: zero key before freeing it Stefan Rompf
@ 2006-01-04 20:09 ` Arjan van de Ven
2006-01-04 20:26 ` Stefan Rompf
2006-01-24 4:49 ` [Patch 2.6] dm-crypt: zero key before freeing it Neil Brown
2006-01-24 23:03 ` Phillip Susi
2 siblings, 1 reply; 12+ messages in thread
From: Arjan van de Ven @ 2006-01-04 20:09 UTC (permalink / raw)
To: Stefan Rompf; +Cc: Andrew Morton, Clemens Fruhwirth, linux-kernel, stable
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"
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:09 ` Arjan van de Ven
@ 2006-01-04 20:26 ` Stefan Rompf
2006-01-04 20:28 ` Randy.Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Rompf @ 2006-01-04 20:26 UTC (permalink / raw)
To: Arjan van de Ven; +Cc: Andrew Morton, Clemens Fruhwirth, linux-kernel, stable
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);
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:26 ` Stefan Rompf
@ 2006-01-04 20:28 ` Randy.Dunlap
2006-01-04 20:41 ` Jörn Engel
0 siblings, 1 reply; 12+ messages in thread
From: Randy.Dunlap @ 2006-01-04 20:28 UTC (permalink / raw)
To: Stefan Rompf
Cc: Arjan van de Ven, Andrew Morton, Clemens Fruhwirth, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:28 ` Randy.Dunlap
@ 2006-01-04 20:41 ` Jörn Engel
2006-01-04 20:43 ` Randy.Dunlap
0 siblings, 1 reply; 12+ messages in thread
From: Jörn Engel @ 2006-01-04 20:41 UTC (permalink / raw)
To: Randy.Dunlap
Cc: Stefan Rompf, Arjan van de Ven, Andrew Morton, Clemens Fruhwirth,
linux-kernel, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:41 ` Jörn Engel
@ 2006-01-04 20:43 ` Randy.Dunlap
2006-01-04 21:15 ` [stable] " Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Randy.Dunlap @ 2006-01-04 20:43 UTC (permalink / raw)
To: Jörn Engel
Cc: Randy.Dunlap, Stefan Rompf, Arjan van de Ven, Andrew Morton,
Clemens Fruhwirth, linux-kernel, stable
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [stable] Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:43 ` Randy.Dunlap
@ 2006-01-04 21:15 ` Greg KH
2006-01-04 21:44 ` [Patch 2.6] dm-crypt: Zero key material before free to avoid information leak Stefan Rompf
0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2006-01-04 21:15 UTC (permalink / raw)
To: Randy.Dunlap
Cc: J?rn Engel, linux-kernel, Stefan Rompf, Clemens Fruhwirth,
stable, Arjan van de Ven
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [Patch 2.6] dm-crypt: Zero key material before free to avoid information leak
2006-01-04 21:15 ` [stable] " Greg KH
@ 2006-01-04 21:44 ` Stefan Rompf
0 siblings, 0 replies; 12+ messages in thread
From: Stefan Rompf @ 2006-01-04 21:44 UTC (permalink / raw)
To: Greg KH
Cc: Randy.Dunlap, J?rn Engel, linux-kernel, Clemens Fruhwirth,
stable, Arjan van de Ven
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);
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:08 [Patch 2.6] dm-crypt: zero key before freeing it Stefan Rompf
2006-01-04 20:09 ` Arjan van de Ven
@ 2006-01-24 4:49 ` Neil Brown
2006-01-24 21:29 ` Stefan Rompf
2006-01-24 23:03 ` Phillip Susi
2 siblings, 1 reply; 12+ messages in thread
From: Neil Brown @ 2006-01-24 4:49 UTC (permalink / raw)
To: Stefan Rompf, Clemens Fruhwirth; +Cc: linux-kernel
>
>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;
}
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-24 4:49 ` [Patch 2.6] dm-crypt: zero key before freeing it Neil Brown
@ 2006-01-24 21:29 ` Stefan Rompf
2006-01-24 21:44 ` Neil Brown
0 siblings, 1 reply; 12+ messages in thread
From: Stefan Rompf @ 2006-01-24 21:29 UTC (permalink / raw)
To: Neil Brown; +Cc: Clemens Fruhwirth, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-24 21:29 ` Stefan Rompf
@ 2006-01-24 21:44 ` Neil Brown
0 siblings, 0 replies; 12+ messages in thread
From: Neil Brown @ 2006-01-24 21:44 UTC (permalink / raw)
To: Stefan Rompf; +Cc: Clemens Fruhwirth, linux-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Patch 2.6] dm-crypt: zero key before freeing it
2006-01-04 20:08 [Patch 2.6] dm-crypt: zero key before freeing it Stefan Rompf
2006-01-04 20:09 ` Arjan van de Ven
2006-01-24 4:49 ` [Patch 2.6] dm-crypt: zero key before freeing it Neil Brown
@ 2006-01-24 23:03 ` Phillip Susi
2 siblings, 0 replies; 12+ messages in thread
From: Phillip Susi @ 2006-01-24 23:03 UTC (permalink / raw)
To: Stefan Rompf; +Cc: Andrew Morton, Clemens Fruhwirth, linux-kernel, stable
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);
> }
>
>
> -
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2006-01-24 23:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-01-04 20:08 [Patch 2.6] dm-crypt: zero key before freeing it Stefan Rompf
2006-01-04 20:09 ` Arjan van de Ven
2006-01-04 20:26 ` Stefan Rompf
2006-01-04 20:28 ` Randy.Dunlap
2006-01-04 20:41 ` Jörn Engel
2006-01-04 20:43 ` Randy.Dunlap
2006-01-04 21:15 ` [stable] " Greg KH
2006-01-04 21:44 ` [Patch 2.6] dm-crypt: Zero key material before free to avoid information leak Stefan Rompf
2006-01-24 4:49 ` [Patch 2.6] dm-crypt: zero key before freeing it Neil Brown
2006-01-24 21:29 ` Stefan Rompf
2006-01-24 21:44 ` Neil Brown
2006-01-24 23:03 ` Phillip Susi
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).