Re: [PATCH 12/18] Hibernate: generate and verify signature of snapshot

[joeyli <jlee@suse.com>]

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@srcf.ucam.org>
> > Reviewed-by: Jiri Kosina <jkosina@suse.cz>
> > Signed-off-by: Lee, Chun-Yi <jlee@suse.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