| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1377573748.27967.60.camel@linux-s257.site>
Date: Tue, 27 Aug 2013 11:22:28 +0800
From: joeyli <jlee@...e.com>
To: Pavel Machek <pavel@....cz>
Cc: linux-kernel@...r.kernel.org,
linux-security-module@...r.kernel.org, linux-efi@...r.kernel.org,
linux-pm@...r.kernel.org, linux-crypto@...r.kernel.org,
opensuse-kernel@...nsuse.org, David Howells <dhowells@...hat.com>,
"Rafael J. Wysocki" <rjw@...k.pl>,
Matthew Garrett <mjg59@...f.ucam.org>,
Len Brown <len.brown@...el.com>,
Josh Boyer <jwboyer@...hat.com>,
Vojtech Pavlik <vojtech@...e.cz>,
Matt Fleming <matt.fleming@...el.com>,
James Bottomley <james.bottomley@...senpartnership.com>,
Greg KH <gregkh@...uxfoundation.org>, JKosina@...e.com,
Rusty Russell <rusty@...tcorp.com.au>,
Herbert Xu <herbert@...dor.apana.org.au>,
"David S. Miller" <davem@...emloft.net>,
"H. Peter Anvin" <hpa@...or.com>, Michal Marek <mmarek@...e.cz>,
Gary Lin <GLin@...e.com>, Vivek Goyal <vgoyal@...hat.com>
Subject: Re: [PATCH 12/18] Hibernate: generate and verify signature of
snapshot
Hi Pavel,
Thanks for your time to review my patches.
於 日，2013-08-25 於 18:36 +0200，Pavel Machek 提到：
> On Thu 2013-08-22 19:01:51, Lee, Chun-Yi wrote:
> > This patch add the code for generate/verify signature of snapshot, it
> > put the signature to snapshot header. This approach can support both
> > on userspace hibernate and in-kernel hibernate.
> >
> > v2:
> > - Due to loaded S4 sign key before ExitBootServices, we need forward key from
> > boot kernel to resume target kernel. So this patch add a empty page in
> > snapshot image, then we keep the pfn of this empty page in snapshot header.
> > When system resume from hibernate, we fill new sign key to this empty page
> > space after snapshot image checked pass. This mechanism let boot kernel can
> > forward new sign key to resume target kernel but don't need write new private
> > key to any other storage, e.g. swap.
> >
> > Cc: Matthew Garrett <mjg59@...f.ucam.org>
> > Reviewed-by: Jiri Kosina <jkosina@...e.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@...e.com>
> > ---
> > kernel/power/power.h | 6 +
> > kernel/power/snapshot.c | 280 +++++++++++++++++++++++++++++++++++++++++++++-
> > kernel/power/swap.c | 14 +++
> > kernel/power/user.c | 9 ++
> > 4 files changed, 302 insertions(+), 7 deletions(-)
> >
> > diff --git a/kernel/power/power.h b/kernel/power/power.h
> > index 69a81d8..84e0b06 100644
> > --- a/kernel/power/power.h
> > +++ b/kernel/power/power.h
> > @@ -3,6 +3,9 @@
> > #include <linux/utsname.h>
> > #include <linux/freezer.h>
> >
> > +/* The maximum length of snapshot signature */
> > +#define SIG_LENG 512
> > +
> > struct swsusp_info {
> > struct new_utsname uts;
> > u32 version_code;
> > @@ -11,6 +14,8 @@ struct swsusp_info {
> > unsigned long image_pages;
> > unsigned long pages;
> > unsigned long size;
> > + unsigned long skey_data_buf_pfn;
> > + u8 signature[SIG_LENG];
> > } __attribute__((aligned(PAGE_SIZE)));
>
> SIG_LEN or SIG_LENGTH. Select one.
>
I will use SIG_LEN at next version, thanks!
>
> > +static int
> > copy_data_pages(struct memory_bitmap *copy_bm, struct memory_bitmap *orig_bm)
> > {
> > struct zone *zone;
> > - unsigned long pfn;
> > + unsigned long pfn, dst_pfn;
> ...
> > + tfm = crypto_alloc_shash(SNAPSHOT_HASH, 0, 0);
> > + if (IS_ERR(tfm)) {
> > + pr_err("IS_ERR(tfm): %ld", PTR_ERR(tfm));
> > + return PTR_ERR(tfm);
> > + }
> > +
> > + desc_size = crypto_shash_descsize(tfm) + sizeof(*desc);
> > + digest_size = crypto_shash_digestsize(tfm);
> > + digest = kzalloc(digest_size + desc_size, GFP_KERNEL);
>
> Are you sure GFP_KERNEL allocation is okay at this phase of
> hibernation?
>
> Could the hashing be done at later phase, when writing the image to
> disk?
>
Thanks for you point out!
Yes, call memory allocate here is not a good design due to it causes
garbage in snapshot that will not released by resumed kernel.
I just finished another implementation, the respin patch extracts the
signature generation code to another function then call the function in
swsusp_save() after copy_data_pages() finished. We can write to memory
at that stage.
> >
> > +void **h_buf;
>
> helpfully named.
>
I will change the name to handle_buffers;
> > + ret = verify_signature(s4_wake_key, pks);
> > + if (ret) {
> > + pr_err("snapshot S4 signature verification fail: %d\n", ret);
> > + goto error_verify;
> > + } else
> > + pr_info("snapshot S4 signature verification pass!\n");
> > +
> > + if (pks->rsa.s)
> > + mpi_free(pks->rsa.s);
> > + kfree(pks);
>
> ret = 0 and fall through?
>
When verification success, verify_signature() will return 0.
Yes, here have duplicate code, I will clear up it.
> > + return 0;
> > +
> > +error_verify:
> > + if (pks->rsa.s)
> > + mpi_free(pks->rsa.s);
> > +error_mpi:
> > + kfree(pks);
> > + return ret;
> > +}
>
>
> > + ret = crypto_shash_final(desc, digest);
> > + if (ret)
> > + goto error_shash;
> > +
> > + ret = snapshot_verify_signature(digest, digest_size);
> > + if (ret)
> > + goto error_verify;
> > +
> > + kfree(h_buf);
> > + kfree(digest);
> > + crypto_free_shash(tfm);
> > + return 0;
>
> These four lines can be deleted.
>
Yes, here also duplicate, I will remove.
> > +
> > +error_verify:
> > +error_shash:
> > + kfree(h_buf);
> > + kfree(digest);
> > +error_digest:
> > + crypto_free_shash(tfm);
> > + return ret;
> > +}
> > +
> Pavel
Thanks a lot!
Joey Lee
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists