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 <[email protected]>
Acked-by: Clemens Fruhwirth <[email protected]>
--- 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);
}
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"
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 <[email protected]>
Acked-by: Clemens Fruhwirth <[email protected]>
--- 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);
}
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 <[email protected]>
> Acked-by: Clemens Fruhwirth <[email protected]>
>
> --- 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
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
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
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
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 <[email protected]>
Acked-by: Clemens Fruhwirth <[email protected]>
--- 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);
}
>
>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 <[email protected]>
>Acked-by: Clemens Fruhwirth <[email protected]>
>
>--- 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 <[email protected]>
### 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;
}
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
On Tuesday January 24, [email protected] 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
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 <[email protected]>
> Acked-by: Clemens Fruhwirth <[email protected]>
>
> --- 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);
> }
>
>
> -
>
>