On 09/04/2011 12:58 PM, Michael S. Tsirkin wrote:
On Thu, Sep 01, 2011 at 10:23:51PM -0400, Stefan Berger wrote:
Checks are added that test
- whether encryption is supported follwing the revision of the directory
structure (rev>= 2)
You never generate rev 1 code, right?
I did this in the previous patch that implemented rev 1 that knew
nothing about the encryption added in rev 2.
So why keep that support around in code?
The first version merged into qemu should be revision 0 (or 1, as you like).
I chose '1'. See patch 9:
+#define BS_DIR_REV1 1
+
+#define BS_DIR_REV_CURRENT BS_DIR_REV1
+
So I think it's the proper thing to do to increase the revision
number from 1 to 2 since it's in two separate patches (even if they
were to be applied immediately).
Look, the code is not merged yet and you already have
legacy with revision history to support? Why create work?
Ok, I won't build the intermediate version then.
Don't support legacy with old version of your patch.
- whether a key has been provided although all data are stored in clear-text
- whether a key is missing for decryption.
In either one of the cases the backend reports an error message to the user
and Qemu terminates.
-v7:
- cleaned up function parsing key
-v6:
- changed the format of the key= to take the type of encryption into
account: key=aes-cbc:0x12345... and reworked code for encryption and
decryption of blobs;
separate type and data:
keytype=aes-cbc,key=0x123 to avoid introducing more option parsing.
Also, are people likely to have the key in a file?
If yes maybe read a key from there and skip parsing completely?
I think both choices should probably exist. Now what's a good file
format? Would we expect to find a hex number in there or should it
always be assumed to be a binary file?
binary
So then I'd keep the possibility to pass the key via command line and
add the option to read it from a file as well.
Two empty lines in a row :)
Probably this is not the only occurrence... Is this a problem?
Yes. They aren't helpful. checkpatch should complain about these
not sure whether it does.
It so far doesn't.
+static bool tpm_builtin_parse_as_hexkey(const char *rawkey,
+ unsigned char *keyvalue,
+ int *keysize)
+{
+ size_t c = 0;
+
+ /* skip over leading '0x' */
+ if (!strncmp(rawkey, "0x", 2)) {
+ rawkey += 2;
+ }
+
+ c = stream_to_bin(rawkey, keyvalue, *keysize);
+
+ if (c == 256/4) {
+ *keysize = 256;
+ } else if (c>= 192/4) {
+ *keysize = 192;
+ } else if (c>= 128/4) {
+ *keysize = 128;
+ } else {
+ return false;
Want to tell the user what went wrong?
Here's what the key parser handles:
- all keys>= 256 bits are truncated to 256 bits
- all keys>= 192 bits are truncated to 192 bits
- all keys>= 128 bits are truncated to 128 bits
- all keys< 128 bits are assumed to not be given as a hexadecimal
number but the string itself is the key, i.e. 'HELLOWORLD' becomes a
valid key.
Oh my. I presume this makes sense in some world ...
So then I will require to have the exact size of a 128, 192 or 256 bit
key then and maybe allow 'passwords', which really was what the last
example above was falling back to. Looking around in the QCoW2 code
passwords seem to be also possible there (block/qcow2.c:qcow_set_key()).
Also, you don't allow skipping leading zeroes?
An AES key should be allowed to have leading zeros, no?
+ }
+
+ return true;
Always put spaces around /.
But where does the /4 come from? 4 bits per character?
c is the number of 'nibbles'. 4 bits in a nibble - that's what the
/4 comes from.
+}
+
+
static TPMBackend *tpm_builtin_create(QemuOpts *opts, const char *id,
const char *model)
{
TPMBackend *driver;
const char *value;
+ unsigned char keyvalue[256/8];
+ int keysize = sizeof(keyvalue);
driver = g_malloc(sizeof(TPMBackend));
if (!driver) {
@@ -1523,6 +1769,33 @@ static TPMBackend *tpm_builtin_create(Qe
goto err_exit;
}
+ value = qemu_opt_get(opts, "key");
+ if (value) {
+ if (!strncasecmp(value, "aes-cbc:", 8)) {
+ memset(keyvalue, 0x0, sizeof(keyvalue));
+
+ if (!tpm_builtin_parse_as_hexkey(&value[8], keyvalue,&keysize)) {
+ keysize = 128;
+ strncpy((char *)keyvalue, value, 128/8);
Here first the input is attempted to be parsed as hex key and if
that fails the input string is taken as the key. It should be
&value[8] here -- so that's a bug.
Stefan
These should be different options.
key_format=hex/string
etc.
To summarize it:
enc_mode=<aes-cbc> # redundant for now since this is the only
supported encryption scheme; so could drop it and assume as default
key_format=<hex/binary> # hex for a string hex number; binary would mean
the found string is directly converted to a char[] that is then directly
used as the AES key (like a password)
key=<128, 192, or 256 bit>hex key or string
key_file=<path to file containing 128,192 or 256 bit hex key or string>
Stefan
