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 > > Reviewed-by: Jiri Kosina > > Signed-off-by: Lee, Chun-Yi > > --- > > 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 > > #include > > > > +/* 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-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Announce loop-AES-v3.7a file/swap crypto package
loop-AES changes since previous release: - Code and struct cleanup, make it work on 3.11-rc kernels. - Fixed directory permissions bug in util-linux-2.23.2 when "mount -a" set up encrypted file system using ramdom keys. util-linux-2.21.2 and older versions are not affected. bzip2 compressed tarball is here: http://loop-aes.sourceforge.net/loop-AES/loop-AES-v3.7a.tar.bz2 md5sum f267cb2108ee94b09a30b2fc7e292104 http://loop-aes.sourceforge.net/loop-AES/loop-AES-v3.7a.tar.bz2.sign -- Jari Ruusu 1024R/3A220F51 5B 4B F9 BB D3 3F 52 E9 DB 1D EB E3 24 0E A9 DD -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/18] asymmetric keys: separate the length checking of octet string from RSA_I2OSP
Hi! > > > Due to RSA_I2OSP is not only used by signature verification path but also > > > used > > > in signature generation path. So, separate the length checking of octet > > > string > > > because it's not for generate 0x00 0x01 leading string when used in > > > signature > > > generation. > > > > > > Reviewed-by: Jiri Kosina > > > Signed-off-by: Lee, Chun-Yi > > > > > +static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X) > > > +{ > > > + unsigned x_size; > > > + unsigned X_size; > > > + u8 *X = NULL; > > > > Is this kernel code or entry into obfuscated C code contest? This is not > > funny. > > > The small "x" is the input integer that will transfer to big "X" that is > a octet sting. > > Sorry for I direct give variable name to match with spec, maybe I need > use big_X or Having variables that differ only in case is confusing. Actually RSA_I2OSP is not a good name for function, either. If it converts x into X, perhaps you can name one input and second output? > Do you have good suggest for the naming? Not really, sorry. Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 03/18] asymmetric keys: separate the length checking of octet string from RSA_I2OSP
於 日,2013-08-25 於 18:01 +0200,Pavel Machek 提到: > On Thu 2013-08-22 19:01:42, Lee, Chun-Yi wrote: > > Due to RSA_I2OSP is not only used by signature verification path but also > > used > > in signature generation path. So, separate the length checking of octet > > string > > because it's not for generate 0x00 0x01 leading string when used in > > signature > > generation. > > > > Reviewed-by: Jiri Kosina > > Signed-off-by: Lee, Chun-Yi > > > +static int RSA_I2OSP(MPI x, size_t xLen, u8 **_X) > > +{ > > + unsigned x_size; > > + unsigned X_size; > > + u8 *X = NULL; > > Is this kernel code or entry into obfuscated C code contest? This is not > funny. > > Pavel The small "x" is the input integer that will transfer to big "X" that is a octet sting. Sorry for I direct give variable name to match with spec, maybe I need use big_X or Do you have good suggest for the naming? Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/18] asymmetric keys: implement EMSA_PKCS1-v1_5-ENCODE in rsa
Hi Pavel, First, thanks for your review! 於 日,2013-08-25 於 17:53 +0200,Pavel Machek 提到: > On Thu 2013-08-22 19:01:41, Lee, Chun-Yi wrote: > > Implement EMSA_PKCS1-v1_5-ENCODE [RFC3447 sec 9.2] in rsa.c. It's the > > first step of signature generation operation > > (RSASSA-PKCS1-v1_5-SIGN). > > Is this your own code, or did you copy it from somewhere? > It's my own code, development base on RSA PKCS#1 spec. So the naming of variables are match with PKCS#1 spec. > > + if (!T) > > + goto error_T; > > + > > + memcpy(T, RSA_ASN1_templates[hash_algo].data, > > RSA_ASN1_templates[hash_algo].size); > > + memcpy(T + RSA_ASN1_templates[hash_algo].size, pks->digest, > > pks->digest_size); > > + > > + /* 3) check If emLen < tLen + 11, output "intended encoded message > > length too short" */ > > + if (emLen < tLen + 11) { > > + ret = EINVAL; > > + goto error_emLen; > > + } > > Normal kernel calling convention is 0 / -EINVAL. Yes, here is my mistake, I will modify it. > > > + memcpy(EM + 2, PS, emLen - tLen - 3); > > + EM[2 + emLen - tLen - 3] = 0x00; > > + memcpy(EM + 2 + emLen - tLen - 3 + 1, T, tLen); > > ThisDoesNotLookLikeKernelCode, NoCamelCase, please. > Thanks for you point out, I will change it. > > + *_EM = EM; > > And we don't usually use _ prefix like this. > Thanks! I will change it. > > > --- a/include/crypto/public_key.h > > +++ b/include/crypto/public_key.h > > @@ -110,6 +110,8 @@ extern void public_key_destroy(void *payload); > > struct public_key_signature { > > u8 *digest; > > u8 digest_size; /* Number of bytes in digest */ > > + u8 *S; /* signature S of length k octets */ > > u8 *signature? Yes, this 'S' is signature. I put the naming full match with spec for development, I will change it to match kernel rule. e.g. signature_S > > > + size_t k; /* length k of signature S */ > > u8 *signature_length. > I will use signature_leng_k to also match with PKCS#1 spec, I think it's better for review source code with the spec for debugging. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html