From: Eric Biggers <ebiggers3@gmail.com>
To: Chen Yu <yu.c.chen@intel.com>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Pavel Machek <pavel@ucw.cz>, Len Brown <len.brown@intel.com>,
"Lee, Chun-Yi" <jlee@suse.com>, Borislav Petkov <bp@alien8.de>,
linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
Theodore Ts'o <tytso@mit.edu>,
Stephan Mueller <smueller@chronox.de>,
Denis Kenzior <denkenz@gmail.com>
Subject: Re: [PATCH 3/3][RFC] tools: create power/crypto utility
Date: Wed, 20 Jun 2018 10:41:42 -0700 [thread overview]
Message-ID: <20180620174142.GA76265@gmail.com> (raw)
In-Reply-To: <78af30838d0bac69bdd6e138b659bcbb8464fd13.1529486870.git.yu.c.chen@intel.com>
Hi Chen,
On Wed, Jun 20, 2018 at 05:40:51PM +0800, Chen Yu wrote:
> crypto_hibernate is a user-space utility to generate
> 512bits AES key and pass it to the kernel via ioctl
> for hibernation encryption.(We can also add the key
> into kernel via keyctl if necessary, but currently
> using ioctl seems to be more straightforward as we
> need both the key and salt transit).
>
> The key derivation is based on a simplified implementation
> of PBKDF2[1] in e2fsprogs - both the key length and the hash
> bytes are the same - 512bits. crypto_hibernate will firstly
> probe the user for passphrase and get salt from kernel, then
> uses them to generate a 512bits AES key based on PBKDF2.
What is a "512bits AES key"? Do you mean AES-256-XTS (which takes a 512-bit
key, which the XTS mode internally splits into two keys)? Do you allow for
other algorithms, or is it hardcoded to AES-256-XTS? What if someone wants to
use a different algorithm?
BTW, it's difficult to review this with only patch 3/3 Cc'ed to me, as there is
no context about the problem you are trying to solve and what your actual
proposed kernel changes are. I suggest Cc'ing linux-crypto on all 3 patches.
A few more comments below, from a very brief reading of the code:
[...]
> +
> +#define PBKDF2_ITERATIONS 0xFFFF
> +#define SHA512_BLOCKSIZE 128
> +#define SHA512_LENGTH 64
> +#define SALT_BYTES 16
> +#define SYM_KEY_BYTES SHA512_LENGTH
> +#define TOTAL_USER_INFO_LEN (SALT_BYTES+SYM_KEY_BYTES)
> +#define MAX_PASSPHRASE_SIZE 1024
> +
> +struct hibernation_crypto_keys {
> + char derived_key[SYM_KEY_BYTES];
> + char salt[SALT_BYTES];
> + bool valid;
> +};
> +
> +struct hibernation_crypto_keys hib_keys;
> +
> +static char *get_key_ptr(void)
> +{
> + return hib_keys.derived_key;
> +}
> +
> +static char *get_salt_ptr(void)
> +{
> + return hib_keys.salt;
> +}
[...]
> +
> +
> +#define HIBERNATE_SALT_READ _IOW('C', 3, struct hibernation_crypto_keys)
> +#define HIBERNATE_KEY_WRITE _IOW('C', 4, struct hibernation_crypto_keys)
Why are the ioctl numbers defined based on the size of 'struct
hibernation_crypto_keys'? It's not a UAPI structure, right?
> +
> +static void get_passphrase(char *passphrase, int len)
> +{
> + char *p;
> + struct termios current_settings;
> +
> + assert(len > 0);
> + disable_echo(¤t_settings);
> + p = fgets(passphrase, len, stdin);
> + tcsetattr(0, TCSANOW, ¤t_settings);
> + printf("\n");
> + if (!p) {
> + printf("Aborting.\n");
> + exit(1);
> + }
> + p = strrchr(passphrase, '\n');
> + if (!p)
> + p = passphrase + len - 1;
> + *p = '\0';
> +}
> +
> +#define CRYPTO_FILE "/dev/crypto_hibernate"
> +
> +static int write_keys(void)
> +{
> + int fd;
> +
> + fd = open(CRYPTO_FILE, O_RDWR);
> + if (fd < 0) {
> + printf("Cannot open device file...\n");
> + return -EINVAL;
> + }
> + ioctl(fd, HIBERNATE_KEY_WRITE, get_key_ptr());
> + return 0;
No error checking on the ioctl?
Also, how is the kernel supposed to know how long the key is, and which
algorithm it's supposed to be used for? I assume it's hardcoded to AES-256-XTS?
What if someone wants to use a different algorithm?
> +}
> +
> +static int read_salt(void)
> +{
> + int fd;
> +
> + fd = open(CRYPTO_FILE, O_RDWR);
> + if (fd < 0) {
> + printf("Cannot open device file...\n");
> + return -EINVAL;
> + }
> + ioctl(fd, HIBERNATE_SALT_READ, get_salt_ptr());
> + return 0;
> +}
No error checking on the ioctl?
> +int main(int argc, char *argv[])
> +{
> + int opt, option_index = 0;
> + char in_passphrase[MAX_PASSPHRASE_SIZE];
> +
> + while ((opt = getopt_long_only(argc, argv, "+p:s:h",
> + NULL, &option_index)) != -1) {
> + switch (opt) {
> + case 'p':
> + {
> + char *p = optarg;
> +
> + if (strlen(p) >= (MAX_PASSPHRASE_SIZE - 1)) {
> + printf("Please provide passphrase less than %d bytes.\n",
> + MAX_PASSPHRASE_SIZE);
> + exit(1);
> + }
> + strcpy(in_passphrase, p);
I haven't read this super closely, but this really looks like an off-by-one
error. It seems you intended MAX_PASSPHRASE_SIZE to include a null terminator,
so the correct check would be 'strlen(p) >= MAX_PASSPHRASE_SIZE'.
> + }
> + break;
> + case 's':
> + {
> + char *p = optarg;
> +
> + if (strlen(p) != (SALT_BYTES - 1)) {
> + printf("Please provide salt with len less than %d bytes.\n",
> + SALT_BYTES);
> + exit(1);
> + }
> + strcpy(get_salt_ptr(), p);
> + }
> + break;
Salts don't need to be human-readable. How about making the salt binary? So, a
salt specified on the command-line would be hex.
Eric
next prev parent reply other threads:[~2018-06-20 17:41 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-20 9:39 [PATCH 0/3][RFC] Introduce the in-kernel hibernation encryption Chen Yu
2018-06-20 9:39 ` [PATCH 1/3][RFC] PM / Hibernate: Add helper functions for " Chen Yu
2018-06-20 9:40 ` [PATCH 2/3][RFC] PM / Hibernate: Encrypt the snapshot pages before submitted to the block device Chen Yu
2018-06-28 13:07 ` joeyli
2018-06-28 13:50 ` Yu Chen
2018-06-28 14:28 ` joeyli
2018-06-28 14:52 ` Yu Chen
2018-06-29 12:59 ` joeyli
2018-07-06 15:28 ` Yu Chen
2018-07-12 10:10 ` joeyli
2018-07-13 7:34 ` Yu Chen
2018-07-18 15:48 ` joeyli
2018-07-19 9:16 ` Yu Chen
2018-06-20 9:40 ` [PATCH 3/3][RFC] tools: create power/crypto utility Chen Yu
2018-06-20 17:41 ` Eric Biggers [this message]
2018-06-22 2:39 ` Yu Chen
2018-06-22 2:59 ` Eric Biggers
2018-06-21 9:01 ` Pavel Machek
2018-06-21 12:10 ` Rafael J. Wysocki
2018-06-21 19:04 ` Pavel Machek
2018-06-25 7:06 ` Rafael J. Wysocki
2018-06-25 11:54 ` Pavel Machek
2018-06-25 21:56 ` Rafael J. Wysocki
2018-06-25 22:16 ` Pavel Machek
[not found] ` <1530009024.20417.5.camel@suse.com>
2018-06-26 11:12 ` Pavel Machek
2018-06-21 8:53 ` [PATCH 0/3][RFC] Introduce the in-kernel hibernation encryption Pavel Machek
2018-06-21 12:08 ` Rafael J. Wysocki
2018-06-21 19:14 ` Pavel Machek
2018-06-22 2:14 ` Yu Chen
2018-06-25 11:55 ` Pavel Machek
2018-06-25 7:16 ` Rafael J. Wysocki
2018-06-25 11:59 ` Pavel Machek
2018-06-25 22:14 ` Rafael J. Wysocki
2018-07-05 16:16 ` joeyli
2018-07-06 13:42 ` Yu Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180620174142.GA76265@gmail.com \
--to=ebiggers3@gmail.com \
--cc=bp@alien8.de \
--cc=denkenz@gmail.com \
--cc=jlee@suse.com \
--cc=len.brown@intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=pavel@ucw.cz \
--cc=rafael.j.wysocki@intel.com \
--cc=rafael@kernel.org \
--cc=smueller@chronox.de \
--cc=tytso@mit.edu \
--cc=yu.c.chen@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).