From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756083Ab1KBRul (ORCPT ); Wed, 2 Nov 2011 13:50:41 -0400 Received: from igw2.watson.ibm.com ([129.34.20.6]:59121 "EHLO igw2.watson.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755910Ab1KBRui convert rfc822-to-8bit (ORCPT ); Wed, 2 Nov 2011 13:50:38 -0400 Subject: Re: [PATCH 1/2] trusted-key: allow overwriting the migratable flag From: David Safford To: Roberto Sassu Cc: keyrings@linux-nfs.org, linux-security-module@vger.kernel.org, linux-kernel@vger.kernel.org, zohar@us.ibm.com, dhowells@redhat.com, jmorris@namei.org Date: Wed, 02 Nov 2011 13:46:32 -0400 In-Reply-To: <4EB17FE1.7050307@polito.it> References: <1320237682-3857-1-git-send-email-roberto.sassu@polito.it> <1320253136.3225.13.camel@localhost> <4EB17FE1.7050307@polito.it> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8BIT X-Mailer: Evolution 3.0.3 (3.0.3-1.fc15) Message-ID: <1320255992.3225.23.camel@localhost> Mime-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 2011-11-02 at 18:37 +0100, Roberto Sassu wrote: > On 11/02/2011 05:58 PM, David Safford wrote: > > On Wed, 2011-11-02 at 13:41 +0100, Roberto Sassu wrote: > >> The migratable should be modifiable during the key update() method. This > >> allows for example to update a migratable trusted key, wrapped by a TPM > >> key, to a a non-migratable one sealed under the SRK with a PCR set. > >> > >> Signed-off-by: Roberto Sassu > > > > I can see a use case for updating a migratable key to a non-migratable > > one - such as keeping a migratable master on a flash drive, and keeping > > only the non-migratable copy on-line. I certainly don't want the > > ability to change a non-migratable to migratable, as that would defeat > > the entire purpose of non-migratable. > > > > I don't think this patch actually does either, though. > > > >> --- > >> security/keys/trusted.c | 1 - > >> 1 files changed, 0 insertions(+), 1 deletions(-) > >> > >> diff --git a/security/keys/trusted.c b/security/keys/trusted.c > >> index 0c33e2e..8777015 100644 > >> --- a/security/keys/trusted.c > >> +++ b/security/keys/trusted.c > >> @@ -1036,7 +1036,6 @@ static int trusted_update(struct key *key, const void *data, size_t datalen) > >> goto out; > >> } > >> /* copy old key values, and reseal with new pcrs */ > >> - new_p->migratable = p->migratable; > > > > Taking out this line appears only to remove a redundant assignment. > > We can only get here if the old key is already migratable, and the > > earlier trusted_payload_alloc() initializes the new copy to > > migratable by default. I don't see how the flag can be changed > > with this patch. Perhaps I'm missing something or this was just > > the start, and there is more to come? > > > > Hi Dave > > i think this line should be removed because it overwrites > the assignment made in datablob_parse() -> getoptions(). > > You can see this behaviour by executing the following commands: > > 1) keyctl add trusted kmk "new 32" @u > 2) keyctl update $(keyctl search @u trusted kmk) "update migratable=0" > 3) keyctl update $(keyctl search @u trusted kmk) "update migratable=0" > > The third operation should fail because of the lines at the > begin of trusted_update(): > > --------- > if (!p->migratable) > return -EPERM; > --------- > > but instead, without the patch, it is performed successfully. > > Thanks > > Roberto Sassu Sorry - you are absolutely correct. The patch looks good, and it is safe, as it allows only migratable -> non-migratable updates. Give me a day to test both... dave > > > dave > > > >> new_p->key_len = p->key_len; > >> memcpy(new_p->key, p->key, p->key_len); > >> dump_payload(p); > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >