Re: [PATCH v1.3 4/4] keys: add new key-type encrypted

Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
From: David Howells
Date: Friday, November 12, 2010 - 12:45 pm

Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:


Many of the comments I made against patch #3 also apply here.  Use 'Define'
rather than 'Defines' here for example.


Don't include the names of files in the files.


That comment is not really needed.


But the variable name is 'encrypted_data'...


If any of these are binary data rather than character data, I'd use u8 rather
than char.


Don't do that.


Merge with the documentation file in Documentation/ from patch 3 and stick a
pointer to that file here.


const.


???  This is a numeric constant, but you're telling the compiler you want to
store it in the .data section.  Is there a reason?


const.


Initialised numeric constants.


It's only used in one place in the file, and it only sets global vars.  Why
pass pointers to them?


Why skip 0?


const.


Is case significant?  If not, you don't need duplicated constants.  Why do you
care about the capitalisation here (and only in a restricted sense) when you
didn't in the trusted keys patch?


I would split this over three lines.


Unnecessary cast.  iv is void*.


That's what square brackets are for.


datalen is not a string.


You can't do this.  You've defined an update method, so the pointer may change
under you, and the pointee may be destroyed without warning.  You've been
using RCU, so you should use that.  You need to use rcu_read_{,un}lock() and
rcu_dereference() and you must be aware that the payload may be destroyed from
the moment you drop the RCU read lock.

You're returning a pointer to the key, so you perhaps don't really need to
access the payload at this point.


Ditto.


Use shash so that you don't have to use the SG interface.


Can key be const?  Should digest, key and buf be u8* or void* if they're
binary data rather than string data?  This sort of thing should perhaps be
percolated through all your functions.


Can buf be const?


master_key should be const.  master_keylen should perhaps be size_t.


Why do you allow the master key to be supplied by a user-defined key rather
than requiring a trusted-key unconditionally?

Note that the description of a user-defined key should prefixed to distinguish
it from other random user-defined keys.


Should derived_key be const and derived_keylen be size_t?


More const and size_t?  There are plenty more places where you should probably
be using const or size_t.  I'm not going to list any further


I presume you don't need to release the stuff pointed to by epayload before
the memset because __ekey_init() makes the pointers point at offsets into the
payload blob.


Merge those two lines into one.


You don't need to cast master_keylen like this.  The compiler will do that
automatically.  In fact, master_key should be const and master_key_len should
probably be size_t.


const.


const and size_t.  And further instances thereof further down.

David
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[PATCH v1.3 0/4] keys: trusted and encrypted keys, Mimi Zohar, (Wed Nov 10, 8:51 am)
[PATCH v1.3 2/4] key: add tpm_send command, Mimi Zohar, (Wed Nov 10, 8:51 am)
[PATCH v1.3 3/4] keys: add new trusted key-type, Mimi Zohar, (Wed Nov 10, 8:51 am)
[PATCH v1.3 4/4] keys: add new key-type encrypted, Mimi Zohar, (Wed Nov 10, 8:51 am)
Re: [PATCH v1.3 2/4] key: add tpm_send command, David Howells, (Thu Nov 11, 12:48 pm)
Re: [PATCH v1.3 3/4] keys: add new trusted key-type, David Howells, (Thu Nov 11, 2:57 pm)
Re: [PATCH v1.3 2/4] key: add tpm_send command, Mimi Zohar, (Thu Nov 11, 3:25 pm)
Re: [PATCH v1.3 3/4] keys: add new trusted key-type, David Safford, (Fri Nov 12, 5:58 am)
Re: [PATCH v1.3 2/4] key: add tpm_send command, David Howells, (Fri Nov 12, 7:11 am)
Re: [PATCH v1.3 2/4] key: add tpm_send command, David Safford, (Fri Nov 12, 7:48 am)
Re: [PATCH v1.3 3/4] keys: add new trusted key-type, David Howells, (Fri Nov 12, 9:52 am)
Re: [PATCH v1.3 3/4] keys: add new trusted key-type, David Safford, (Fri Nov 12, 10:39 am)
Re: [PATCH v1.3 3/4] keys: add new trusted key-type, David Howells, (Fri Nov 12, 11:36 am)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Fri Nov 12, 12:45 pm)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Fri Nov 12, 2:23 pm)
Re: [PATCH v1.3 2/4] key: add tpm_send command, Rajiv Andrade, (Fri Nov 12, 2:24 pm)
Re: [PATCH v1.3 2/4] key: add tpm_send command, David Safford, (Fri Nov 12, 3:06 pm)
Re: [PATCH v1.3 2/4] key: add tpm_send command, David Howells, (Fri Nov 12, 3:11 pm)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Mon Nov 15, 9:18 am)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, Mimi Zohar, (Mon Nov 15, 12:35 pm)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Tue Nov 16, 7:08 am)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Tue Nov 16, 10:50 am)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, Mimi Zohar, (Tue Nov 16, 11:54 am)
Re: [PATCH v1.3 4/4] keys: add new key-type encrypted, David Howells, (Tue Nov 16, 11:58 am)
Re: [PATCH v1.3 2/4] key: add tpm_send command, Rajiv Andrade, (Wed Nov 17, 6:12 am)