Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, 2015-12-09 at 16:24 +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote: > > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > break; > > > > > > > > > > + case Opt_policydigest: > > > > > > > > > > + if (!tpm2 || > > > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > > > opt->digest_len)) > > > > > > > > > > + return -EINVAL; > > > > > > > > > > + kfree(opt->policydigest); > > > > > > > > > > + opt->policydigest = > > > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > > > + GFP_KERNEL); > > > > You're allocating the exact amount of storage needed. There's no reason > > to use kzalloc here or elsewhere in the patch. > > Yup. I'll change this. > > > > > > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before > > > > > > > > > allocating it? > > > > > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > > > > > This would surely signify an error? > > > > > > > > > > > > I'm following the semantics of other options. That's why I > > > > > > implemented > > > > > > it that way for example: > > > > > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > > > keyhandle=0x8000" > > > > > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > > > behaved in a different way... > > > > > > > > > > It seems broken to me -- if you're messing up keyctl commands you > > > > > might > > > > > want to know about it, but we should remain consistent. > > > > > > > > So should I return error if policyhandle/digest appears a second time? I > > > > agree that it'd be better to return -EINVAL. > > > > > > > > The existing behavior is such that any option can appear multiple times > > > > and I chose to be consistent with that. > > > > > > Mimi, David? > > > > I don't have a problem with changing the existing behavior to allow the > > options to be specified only once. > > I don't think this patch is right place to change the behavior as it > should be done for other options too. I think the easiest way of checking if a token has already been seen would be to define a flag and use test_and_set_bit(token, ) after the following code snippet. while ((p = strsep(, " \t"))) { if (*p == '\0' || *p == ' ' || *p == '\t') continue; token = match_token(p, key_tokens, args); Having a separate patch is probably a good idea. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote: > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > > > } > > > > > > > > > break; > > > > > > > > > + case Opt_policydigest: > > > > > > > > > + if (!tpm2 || > > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > > opt->digest_len)) > > > > > > > > > + return -EINVAL; > > > > > > > > > + kfree(opt->policydigest); > > > > > > > > > + opt->policydigest = > > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > > + GFP_KERNEL); > > You're allocating the exact amount of storage needed. There's no reason > to use kzalloc here or elsewhere in the patch. Yup. I'll change this. > > > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating > > > > > > > > it? > > > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > > > This would surely signify an error? > > > > > > > > > > I'm following the semantics of other options. That's why I implemented > > > > > it that way for example: > > > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > > keyhandle=0x8000" > > > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > > behaved in a different way... > > > > > > > > It seems broken to me -- if you're messing up keyctl commands you might > > > > want to know about it, but we should remain consistent. > > > > > > So should I return error if policyhandle/digest appears a second time? I > > > agree that it'd be better to return -EINVAL. > > > > > > The existing behavior is such that any option can appear multiple times > > > and I chose to be consistent with that. > > > > Mimi, David? > > I don't have a problem with changing the existing behavior to allow the > options to be specified only once. I don't think this patch is right place to change the behavior as it should be done for other options too. > BTW, you might want to fail the getoptions() parsing earlier, rather > than waiting until after the while loop to test "policydigest_len != > opt->digest_len". In both Opt_hash and Opt_policydigest you can check > to see if the other option has already been specified. Agreed. > Mimi /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote: > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > > > } > > > > > > > > > break; > > > > > > > > > + case Opt_policydigest: > > > > > > > > > + if (!tpm2 || > > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > > opt->digest_len)) > > > > > > > > > + return -EINVAL; > > > > > > > > > + kfree(opt->policydigest); > > > > > > > > > + opt->policydigest = > > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > > + GFP_KERNEL); > > You're allocating the exact amount of storage needed. There's no reason > to use kzalloc here or elsewhere in the patch. Yup. I'll change this. > > > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating > > > > > > > > it? > > > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > > > This would surely signify an error? > > > > > > > > > > I'm following the semantics of other options. That's why I implemented > > > > > it that way for example: > > > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > > keyhandle=0x8000" > > > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > > behaved in a different way... > > > > > > > > It seems broken to me -- if you're messing up keyctl commands you might > > > > want to know about it, but we should remain consistent. > > > > > > So should I return error if policyhandle/digest appears a second time? I > > > agree that it'd be better to return -EINVAL. > > > > > > The existing behavior is such that any option can appear multiple times > > > and I chose to be consistent with that. > > > > Mimi, David? > > I don't have a problem with changing the existing behavior to allow the > options to be specified only once. I don't think this patch is right place to change the behavior as it should be done for other options too. > BTW, you might want to fail the getoptions() parsing earlier, rather > than waiting until after the while loop to test "policydigest_len != > opt->digest_len". In both Opt_hash and Opt_policydigest you can check > to see if the other option has already been specified. Agreed. > Mimi /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, 2015-12-09 at 16:24 +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 06:56:17PM -0500, Mimi Zohar wrote: > > On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > > > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > break; > > > > > > > > > > + case Opt_policydigest: > > > > > > > > > > + if (!tpm2 || > > > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > > > opt->digest_len)) > > > > > > > > > > + return -EINVAL; > > > > > > > > > > + kfree(opt->policydigest); > > > > > > > > > > + opt->policydigest = > > > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > > > + GFP_KERNEL); > > > > You're allocating the exact amount of storage needed. There's no reason > > to use kzalloc here or elsewhere in the patch. > > Yup. I'll change this. > > > > > > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before > > > > > > > > > allocating it? > > > > > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > > > > > This would surely signify an error? > > > > > > > > > > > > I'm following the semantics of other options. That's why I > > > > > > implemented > > > > > > it that way for example: > > > > > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > > > keyhandle=0x8000" > > > > > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > > > behaved in a different way... > > > > > > > > > > It seems broken to me -- if you're messing up keyctl commands you > > > > > might > > > > > want to know about it, but we should remain consistent. > > > > > > > > So should I return error if policyhandle/digest appears a second time? I > > > > agree that it'd be better to return -EINVAL. > > > > > > > > The existing behavior is such that any option can appear multiple times > > > > and I chose to be consistent with that. > > > > > > Mimi, David? > > > > I don't have a problem with changing the existing behavior to allow the > > options to be specified only once. > > I don't think this patch is right place to change the behavior as it > should be done for other options too. I think the easiest way of checking if a token has already been seen would be to define a flag and use test_and_set_bit(token, ) after the following code snippet. while ((p = strsep(, " \t"))) { if (*p == '\0' || *p == ' ' || *p == '\t') continue; token = match_token(p, key_tokens, args); Having a separate patch is probably a good idea. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > } > > > > > > > > break; > > > > > > > > + case Opt_policydigest: > > > > > > > > + if (!tpm2 || > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > opt->digest_len)) > > > > > > > > + return -EINVAL; > > > > > > > > + kfree(opt->policydigest); > > > > > > > > + opt->policydigest = > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > + GFP_KERNEL); You're allocating the exact amount of storage needed. There's no reason to use kzalloc here or elsewhere in the patch. > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating > > > > > > > it? > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > This would surely signify an error? > > > > > > > > I'm following the semantics of other options. That's why I implemented > > > > it that way for example: > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > keyhandle=0x8000" > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > behaved in a different way... > > > > > > It seems broken to me -- if you're messing up keyctl commands you might > > > want to know about it, but we should remain consistent. > > > > So should I return error if policyhandle/digest appears a second time? I > > agree that it'd be better to return -EINVAL. > > > > The existing behavior is such that any option can appear multiple times > > and I chose to be consistent with that. > > Mimi, David? I don't have a problem with changing the existing behavior to allow the options to be specified only once. BTW, you might want to fail the getoptions() parsing earlier, rather than waiting until after the while loop to test "policydigest_len != opt->digest_len". In both Opt_hash and Opt_policydigest you can check to see if the other option has already been specified. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > } > > > > > > > break; > > > > > > > + case Opt_policydigest: > > > > > > > + if (!tpm2 || > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > opt->digest_len)) > > > > > > > + return -EINVAL; > > > > > > > + kfree(opt->policydigest); > > > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > > > + GFP_KERNEL); > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > This would surely signify an error? > > > > > > I'm following the semantics of other options. That's why I implemented > > > it that way for example: > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > behaved in a different way... > > > > It seems broken to me -- if you're messing up keyctl commands you might > > want to know about it, but we should remain consistent. > > So should I return error if policyhandle/digest appears a second time? I > agree that it'd be better to return -EINVAL. > > The existing behavior is such that any option can appear multiple times > and I chose to be consistent with that. Mimi, David? /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > } > > > > > > break; > > > > > > + case Opt_policydigest: > > > > > > + if (!tpm2 || > > > > > > + strlen(args[0].from) != (2 * > > > > > > opt->digest_len)) > > > > > > + return -EINVAL; > > > > > > + kfree(opt->policydigest); > > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > > + GFP_KERNEL); > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > This would surely signify an error? > > > > I'm following the semantics of other options. That's why I implemented > > it that way for example: > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > > > is perfectly OK. I just thought that it'd be more odd if this option > > behaved in a different way... > > It seems broken to me -- if you're messing up keyctl commands you might > want to know about it, but we should remain consistent. So should I return error if policyhandle/digest appears a second time? I agree that it'd be better to return -EINVAL. The existing behavior is such that any option can appear multiple times and I chose to be consistent with that. > -- > James Morris > /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > } > > > > > > break; > > > > > > + case Opt_policydigest: > > > > > > + if (!tpm2 || > > > > > > + strlen(args[0].from) != (2 * > > > > > > opt->digest_len)) > > > > > > + return -EINVAL; > > > > > > + kfree(opt->policydigest); > > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > > + GFP_KERNEL); > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > This would surely signify an error? > > > > I'm following the semantics of other options. That's why I implemented > > it that way for example: > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > > > is perfectly OK. I just thought that it'd be more odd if this option > > behaved in a different way... > > It seems broken to me -- if you're messing up keyctl commands you might > want to know about it, but we should remain consistent. So should I return error if policyhandle/digest appears a second time? I agree that it'd be better to return -EINVAL. The existing behavior is such that any option can appear multiple times and I chose to be consistent with that. > -- > James Morris >/Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > } > > > > > > > break; > > > > > > > + case Opt_policydigest: > > > > > > > + if (!tpm2 || > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > opt->digest_len)) > > > > > > > + return -EINVAL; > > > > > > > + kfree(opt->policydigest); > > > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > > > + GFP_KERNEL); > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > This would surely signify an error? > > > > > > I'm following the semantics of other options. That's why I implemented > > > it that way for example: > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > behaved in a different way... > > > > It seems broken to me -- if you're messing up keyctl commands you might > > want to know about it, but we should remain consistent. > > So should I return error if policyhandle/digest appears a second time? I > agree that it'd be better to return -EINVAL. > > The existing behavior is such that any option can appear multiple times > and I chose to be consistent with that. Mimi, David? /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, 2015-12-08 at 22:24 +0200, Jarkko Sakkinen wrote: > On Tue, Dec 08, 2015 at 01:01:02PM +0200, Jarkko Sakkinen wrote: > > On Tue, Dec 08, 2015 at 09:35:05AM +1100, James Morris wrote: > > > On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > > > > > > > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > > > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > > > > > > > } > > > > > > > > break; > > > > > > > > + case Opt_policydigest: > > > > > > > > + if (!tpm2 || > > > > > > > > + strlen(args[0].from) != (2 * > > > > > > > > opt->digest_len)) > > > > > > > > + return -EINVAL; > > > > > > > > + kfree(opt->policydigest); > > > > > > > > + opt->policydigest = > > > > > > > > kzalloc(opt->digest_len, > > > > > > > > + GFP_KERNEL); You're allocating the exact amount of storage needed. There's no reason to use kzalloc here or elsewhere in the patch. > > > > > > > > > > > > > > Is it correct to kfree opt->policydigest here before allocating > > > > > > > it? > > > > > > > > > > > > I think so. The same option might be encountered multiple times. > > > > > > > > > > This would surely signify an error? > > > > > > > > I'm following the semantics of other options. That's why I implemented > > > > it that way for example: > > > > > > > > keyctl add trusted kmk "new 32 keyhandle=0x8000 > > > > keyhandle=0x8000" > > > > > > > > is perfectly OK. I just thought that it'd be more odd if this option > > > > behaved in a different way... > > > > > > It seems broken to me -- if you're messing up keyctl commands you might > > > want to know about it, but we should remain consistent. > > > > So should I return error if policyhandle/digest appears a second time? I > > agree that it'd be better to return -EINVAL. > > > > The existing behavior is such that any option can appear multiple times > > and I chose to be consistent with that. > > Mimi, David? I don't have a problem with changing the existing behavior to allow the options to be specified only once. BTW, you might want to fail the getoptions() parsing earlier, rather than waiting until after the while loop to test "policydigest_len != opt->digest_len". In both Opt_hash and Opt_policydigest you can check to see if the other option has already been specified. Mimi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > } > > > > > break; > > > > > + case Opt_policydigest: > > > > > + if (!tpm2 || > > > > > + strlen(args[0].from) != (2 * > > > > > opt->digest_len)) > > > > > + return -EINVAL; > > > > > + kfree(opt->policydigest); > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > + GFP_KERNEL); > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > I think so. The same option might be encountered multiple times. > > > > This would surely signify an error? > > I'm following the semantics of other options. That's why I implemented > it that way for example: > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > is perfectly OK. I just thought that it'd be more odd if this option > behaved in a different way... It seems broken to me -- if you're messing up keyctl commands you might want to know about it, but we should remain consistent. -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > } > > > > break; > > > > + case Opt_policydigest: > > > > + if (!tpm2 || > > > > + strlen(args[0].from) != (2 * > > > > opt->digest_len)) > > > > + return -EINVAL; > > > > + kfree(opt->policydigest); > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > + GFP_KERNEL); > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > I think so. The same option might be encountered multiple times. > > This would surely signify an error? I'm following the semantics of other options. That's why I implemented it that way for example: keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" is perfectly OK. I just thought that it'd be more odd if this option behaved in a different way... > -- > James Morris > /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > } > > > > break; > > > > + case Opt_policydigest: > > > > + if (!tpm2 || > > > > + strlen(args[0].from) != (2 * > > > > opt->digest_len)) > > > > + return -EINVAL; > > > > + kfree(opt->policydigest); > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > + GFP_KERNEL); > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > I think so. The same option might be encountered multiple times. > > This would surely signify an error? I'm following the semantics of other options. That's why I implemented it that way for example: keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" is perfectly OK. I just thought that it'd be more odd if this option behaved in a different way... > -- > James Morris >/Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Mon, 7 Dec 2015, Jarkko Sakkinen wrote: > On Fri, Nov 20, 2015 at 01:34:35PM +1100, James Morris wrote: > > On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > > > > > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > > > > > } > > > > > break; > > > > > + case Opt_policydigest: > > > > > + if (!tpm2 || > > > > > + strlen(args[0].from) != (2 * > > > > > opt->digest_len)) > > > > > + return -EINVAL; > > > > > + kfree(opt->policydigest); > > > > > + opt->policydigest = kzalloc(opt->digest_len, > > > > > + GFP_KERNEL); > > > > > > > > Is it correct to kfree opt->policydigest here before allocating it? > > > > > > I think so. The same option might be encountered multiple times. > > > > This would surely signify an error? > > I'm following the semantics of other options. That's why I implemented > it that way for example: > > keyctl add trusted kmk "new 32 keyhandle=0x8000 keyhandle=0x8000" > > is perfectly OK. I just thought that it'd be more odd if this option > behaved in a different way... It seems broken to me -- if you're messing up keyctl commands you might want to know about it, but we should remain consistent. -- James Morris-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote: > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. > > Signed-off-by: Jarkko Sakkinen This patch has been now peer tested by Colin Ian King. There's still one thing that I'm thinking before daring to put this into pull request. Should the option names reflect that they associate to the blob and not to the root key? My *guess* would be that it's not very common use case to seal primary keys with policies (PCRs and so forth) and therefore I think these names are fine. [1] In TPM 2.0 there is no fixed root key in the chip. Instead tit contains random seeds from which you can derive primary keys by using the TPM2_CreatePrimary command. That's why we require for example keyhandle as an explicit option when used with a TPM 2.0 chip. /Jarkko > --- > Documentation/security/keys-trusted-encrypted.txt | 34 ++--- > drivers/char/tpm/tpm2-cmd.c | 24 ++-- > include/keys/trusted-type.h | 3 ++ > security/keys/trusted.c | 46 > ++- > 4 files changed, 87 insertions(+), 20 deletions(-) > > diff --git a/Documentation/security/keys-trusted-encrypted.txt > b/Documentation/security/keys-trusted-encrypted.txt > index fd2565b..324ddf5 100644 > --- a/Documentation/security/keys-trusted-encrypted.txt > +++ b/Documentation/security/keys-trusted-encrypted.txt > @@ -27,20 +27,26 @@ Usage: > keyctl print keyid > > options: > - keyhandle= ascii hex value of sealing key default 0x4000 (SRK) > - keyauth=ascii hex auth for sealing key default 0x00...i > - (40 ascii zeros) > - blobauth= ascii hex auth for sealed data default 0x00... > - (40 ascii zeros) > - blobauth= ascii hex auth for sealed data default 0x00... > - (40 ascii zeros) > - pcrinfo=ascii hex of PCR_INFO or PCR_INFO_LONG (no default) > - pcrlock=pcr number to be extended to "lock" blob > - migratable= 0|1 indicating permission to reseal to new PCR values, > - default 1 (resealing allowed) > - hash= hash algorithm name as a string. For TPM 1.x the only > - allowed value is sha1. For TPM 2.x the allowed values > - are sha1, sha256, sha384, sha512 and sm3-256. > + keyhandle=ascii hex value of sealing key default 0x4000 (SRK) > + keyauth= ascii hex auth for sealing key default 0x00...i > + (40 ascii zeros) > + blobauth= ascii hex auth for sealed data default 0x00... > + (40 ascii zeros) > + blobauth= ascii hex auth for sealed data default 0x00... > + (40 ascii zeros) > + pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) > + pcrlock= pcr number to be extended to "lock" blob > + migratable= 0|1 indicating permission to reseal to new PCR values, > + default 1 (resealing allowed) > + hash= hash algorithm name as a string. For TPM 1.x the only > + allowed value is sha1. For TPM 2.x the allowed values > + are sha1, sha256, sha384, sha512 and sm3-256. > + policydigest= digest for the authorization policy. must be calculated > + with the same hash algorithm as specified by the 'hash=' > + option. > + policyhandle= handle to an authorization policy session that defines > the > + same policy and with the same hash algorithm as was > used to > + seal the key. > > "keyctl print" returns an ascii hex copy of the sealed key, which is in > standard > TPM_STORED_DATA format. The key length for new keys are always in bytes. > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index d9d0822..45a6340 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > tpm_buf_append_u8(, payload->migratable); > > /* public */ > - tpm_buf_append_u16(, 14); > + if (options->policydigest) > + tpm_buf_append_u16(, 14 + options->digest_len); > + else > + tpm_buf_append_u16(, 14); > > tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH); > tpm_buf_append_u16(, hash); > - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); > - tpm_buf_append_u16(, 0); /* policy digest size */ > + > + /* policy */ > + if (options->policydigest) { > + tpm_buf_append_u32(, 0); > + tpm_buf_append_u16(, options->digest_len); > + tpm_buf_append(,
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote: > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. > > Signed-off-by: Jarkko SakkinenThis patch has been now peer tested by Colin Ian King. There's still one thing that I'm thinking before daring to put this into pull request. Should the option names reflect that they associate to the blob and not to the root key? My *guess* would be that it's not very common use case to seal primary keys with policies (PCRs and so forth) and therefore I think these names are fine. [1] In TPM 2.0 there is no fixed root key in the chip. Instead tit contains random seeds from which you can derive primary keys by using the TPM2_CreatePrimary command. That's why we require for example keyhandle as an explicit option when used with a TPM 2.0 chip. /Jarkko > --- > Documentation/security/keys-trusted-encrypted.txt | 34 ++--- > drivers/char/tpm/tpm2-cmd.c | 24 ++-- > include/keys/trusted-type.h | 3 ++ > security/keys/trusted.c | 46 > ++- > 4 files changed, 87 insertions(+), 20 deletions(-) > > diff --git a/Documentation/security/keys-trusted-encrypted.txt > b/Documentation/security/keys-trusted-encrypted.txt > index fd2565b..324ddf5 100644 > --- a/Documentation/security/keys-trusted-encrypted.txt > +++ b/Documentation/security/keys-trusted-encrypted.txt > @@ -27,20 +27,26 @@ Usage: > keyctl print keyid > > options: > - keyhandle= ascii hex value of sealing key default 0x4000 (SRK) > - keyauth=ascii hex auth for sealing key default 0x00...i > - (40 ascii zeros) > - blobauth= ascii hex auth for sealed data default 0x00... > - (40 ascii zeros) > - blobauth= ascii hex auth for sealed data default 0x00... > - (40 ascii zeros) > - pcrinfo=ascii hex of PCR_INFO or PCR_INFO_LONG (no default) > - pcrlock=pcr number to be extended to "lock" blob > - migratable= 0|1 indicating permission to reseal to new PCR values, > - default 1 (resealing allowed) > - hash= hash algorithm name as a string. For TPM 1.x the only > - allowed value is sha1. For TPM 2.x the allowed values > - are sha1, sha256, sha384, sha512 and sm3-256. > + keyhandle=ascii hex value of sealing key default 0x4000 (SRK) > + keyauth= ascii hex auth for sealing key default 0x00...i > + (40 ascii zeros) > + blobauth= ascii hex auth for sealed data default 0x00... > + (40 ascii zeros) > + blobauth= ascii hex auth for sealed data default 0x00... > + (40 ascii zeros) > + pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) > + pcrlock= pcr number to be extended to "lock" blob > + migratable= 0|1 indicating permission to reseal to new PCR values, > + default 1 (resealing allowed) > + hash= hash algorithm name as a string. For TPM 1.x the only > + allowed value is sha1. For TPM 2.x the allowed values > + are sha1, sha256, sha384, sha512 and sm3-256. > + policydigest= digest for the authorization policy. must be calculated > + with the same hash algorithm as specified by the 'hash=' > + option. > + policyhandle= handle to an authorization policy session that defines > the > + same policy and with the same hash algorithm as was > used to > + seal the key. > > "keyctl print" returns an ascii hex copy of the sealed key, which is in > standard > TPM_STORED_DATA format. The key length for new keys are always in bytes. > diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c > index d9d0822..45a6340 100644 > --- a/drivers/char/tpm/tpm2-cmd.c > +++ b/drivers/char/tpm/tpm2-cmd.c > @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, > tpm_buf_append_u8(, payload->migratable); > > /* public */ > - tpm_buf_append_u16(, 14); > + if (options->policydigest) > + tpm_buf_append_u16(, 14 + options->digest_len); > + else > + tpm_buf_append_u16(, 14); > > tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH); > tpm_buf_append_u16(, hash); > - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); > - tpm_buf_append_u16(, 0); /* policy digest size */ > + > + /* policy */ > + if (options->policydigest) { > + tpm_buf_append_u32(, 0); > + tpm_buf_append_u16(, options->digest_len); > +
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote: > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. I think it is good to say a word about how to test this since the user space supports is still lagging a bit (there's no way to do a "sticky" handle in TSS2 yet). I have my own low-level test scripts over here: https://github.com/jsakkine/tpm2-scripts Trivial example: KEYHANDLE=$(sudo ./tpm2-root-key) POLICYDIGEST=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1 --trial) POLICYHANDLE=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1) KEYID=$(keyctl add trusted kmk "new 32 keyhandle=$KEYHANDLE hash=sha256 policydigest=$POLICYDIGEST" @u) keyctl pipe $KEYID keyctl clear @u keyctl add trusted kmk "load `cat blob.hex` keyhandle=$KEYHANDLE policyhandle=0x0300" @u keyctl clear @u sudo ./tpm2-flush $KEYHANDLE /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, Nov 17, 2015 at 06:27:22PM +0200, Jarkko Sakkinen wrote: > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. I think it is good to say a word about how to test this since the user space supports is still lagging a bit (there's no way to do a "sticky" handle in TSS2 yet). I have my own low-level test scripts over here: https://github.com/jsakkine/tpm2-scripts Trivial example: KEYHANDLE=$(sudo ./tpm2-root-key) POLICYDIGEST=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1 --trial) POLICYHANDLE=$(sudo ./tpm2-pcr-policy --pcr 16 --name-alg=sha256 --bank=sha1) KEYID=$(keyctl add trusted kmk "new 32 keyhandle=$KEYHANDLE hash=sha256 policydigest=$POLICYDIGEST" @u) keyctl pipe $KEYID keyctl clear @u keyctl add trusted kmk "load `cat blob.hex` keyhandle=$KEYHANDLE policyhandle=0x0300" @u keyctl clear @u sudo ./tpm2-flush $KEYHANDLE /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy
On Thu, Nov 19, 2015 at 10:59:57AM +, Fuchs, Andreas wrote: > > > > From: Jarkko Sakkinen [jarkko.sakki...@linux.intel.com] > > Sent: Tuesday, November 17, 2015 17:27 > > > > Support for sealing with a authorization policy. > > > > Two new options for trusted keys: > > > > * 'policydigest=': provide an auth policy digest for sealing. > > * 'policyhandle=': provide a policy session handle for unsealing. > > Hi Jarkko, > > just out of curiosity; when testing this, how did you calculate the blobauth > parameter ? > Since its calculation requires the cpHash for the unseal()-command... > If you "predict" the cpHash in userSpace, this would mean that userspace > needs to know the > kernels way of constructing the unseal()-command to the TPM, which in turn > would make > this part of the ABI and require documentation before upstreaming, imho. Is this a comment about the patch? Have you actually read the source code or where is this coming from? Please read the source code. > Cheers, > Andreas-- /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy
On Thu, Nov 19, 2015 at 10:59:57AM +, Fuchs, Andreas wrote: > > > > From: Jarkko Sakkinen [jarkko.sakki...@linux.intel.com] > > Sent: Tuesday, November 17, 2015 17:27 > > > > Support for sealing with a authorization policy. > > > > Two new options for trusted keys: > > > > * 'policydigest=': provide an auth policy digest for sealing. > > * 'policyhandle=': provide a policy session handle for unsealing. > > Hi Jarkko, > > just out of curiosity; when testing this, how did you calculate the blobauth > parameter ? > Since its calculation requires the cpHash for the unseal()-command... > If you "predict" the cpHash in userSpace, this would mean that userspace > needs to know the > kernels way of constructing the unseal()-command to the TPM, which in turn > would make > this part of the ABI and require documentation before upstreaming, imho. Is this a comment about the patch? Have you actually read the source code or where is this coming from? Please read the source code. > Cheers, > Andreas-- /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > } > > > break; > > > + case Opt_policydigest: > > > + if (!tpm2 || > > > + strlen(args[0].from) != (2 * opt->digest_len)) > > > + return -EINVAL; > > > + kfree(opt->policydigest); > > > + opt->policydigest = kzalloc(opt->digest_len, > > > + GFP_KERNEL); > > > > Is it correct to kfree opt->policydigest here before allocating it? > > I think so. The same option might be encountered multiple times. This would surely signify an error? -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy
> > From: Jarkko Sakkinen [jarkko.sakki...@linux.intel.com] > Sent: Tuesday, November 17, 2015 17:27 > > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. Hi Jarkko, just out of curiosity; when testing this, how did you calculate the blobauth parameter ? Since its calculation requires the cpHash for the unseal()-command... If you "predict" the cpHash in userSpace, this would mean that userspace needs to know the kernels way of constructing the unseal()-command to the TPM, which in turn would make this part of the ABI and require documentation before upstreaming, imho. Cheers, Andreas-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [tpmdd-devel] [PATCH 2/2] keys, trusted: seal with a policy
> > From: Jarkko Sakkinen [jarkko.sakki...@linux.intel.com] > Sent: Tuesday, November 17, 2015 17:27 > > Support for sealing with a authorization policy. > > Two new options for trusted keys: > > * 'policydigest=': provide an auth policy digest for sealing. > * 'policyhandle=': provide a policy session handle for unsealing. Hi Jarkko, just out of curiosity; when testing this, how did you calculate the blobauth parameter ? Since its calculation requires the cpHash for the unseal()-command... If you "predict" the cpHash in userSpace, this would mean that userspace needs to know the kernels way of constructing the unseal()-command to the TPM, which in turn would make this part of the ABI and require documentation before upstreaming, imho. Cheers, Andreas-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, 18 Nov 2015, Jarkko Sakkinen wrote: > On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > > > } > > > break; > > > + case Opt_policydigest: > > > + if (!tpm2 || > > > + strlen(args[0].from) != (2 * opt->digest_len)) > > > + return -EINVAL; > > > + kfree(opt->policydigest); > > > + opt->policydigest = kzalloc(opt->digest_len, > > > + GFP_KERNEL); > > > > Is it correct to kfree opt->policydigest here before allocating it? > > I think so. The same option might be encountered multiple times. This would surely signify an error? -- James Morris-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > } > > break; > > + case Opt_policydigest: > > + if (!tpm2 || > > + strlen(args[0].from) != (2 * opt->digest_len)) > > + return -EINVAL; > > + kfree(opt->policydigest); > > + opt->policydigest = kzalloc(opt->digest_len, > > + GFP_KERNEL); > > Is it correct to kfree opt->policydigest here before allocating it? I think so. The same option might be encountered multiple times. I don't have the check for nulliy because opt is kzalloc'd and checkpatch.pl complained that WARNING: kfree(NULL) is safe and this check is probably not required #20: FILE: security/keys/trusted.c:829: + if (opt->policydigest) + kfree(opt->policydigest); > > + if (!opt->policydigest) > > + return -ENOMEM; > > + res = hex2bin(opt->policydigest, args[0].from, > > + opt->digest_len); > > + if (res < 0) > > + return -EINVAL; > > Do you need to kfree it here on error? trusted_options_free() will kfree it. > -- > James Morris > /Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > } > break; > + case Opt_policydigest: > + if (!tpm2 || > + strlen(args[0].from) != (2 * opt->digest_len)) > + return -EINVAL; > + kfree(opt->policydigest); > + opt->policydigest = kzalloc(opt->digest_len, > + GFP_KERNEL); Is it correct to kfree opt->policydigest here before allocating it? > + if (!opt->policydigest) > + return -ENOMEM; > + res = hex2bin(opt->policydigest, args[0].from, > + opt->digest_len); > + if (res < 0) > + return -EINVAL; Do you need to kfree it here on error? -- James Morris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 2/2] keys, trusted: seal with a policy
Support for sealing with a authorization policy. Two new options for trusted keys: * 'policydigest=': provide an auth policy digest for sealing. * 'policyhandle=': provide a policy session handle for unsealing. Signed-off-by: Jarkko Sakkinen --- Documentation/security/keys-trusted-encrypted.txt | 34 ++--- drivers/char/tpm/tpm2-cmd.c | 24 ++-- include/keys/trusted-type.h | 3 ++ security/keys/trusted.c | 46 ++- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt index fd2565b..324ddf5 100644 --- a/Documentation/security/keys-trusted-encrypted.txt +++ b/Documentation/security/keys-trusted-encrypted.txt @@ -27,20 +27,26 @@ Usage: keyctl print keyid options: - keyhandle= ascii hex value of sealing key default 0x4000 (SRK) - keyauth= ascii hex auth for sealing key default 0x00...i - (40 ascii zeros) - blobauth= ascii hex auth for sealed data default 0x00... - (40 ascii zeros) - blobauth= ascii hex auth for sealed data default 0x00... - (40 ascii zeros) - pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) - pcrlock= pcr number to be extended to "lock" blob - migratable= 0|1 indicating permission to reseal to new PCR values, - default 1 (resealing allowed) - hash= hash algorithm name as a string. For TPM 1.x the only - allowed value is sha1. For TPM 2.x the allowed values - are sha1, sha256, sha384, sha512 and sm3-256. + keyhandle=ascii hex value of sealing key default 0x4000 (SRK) + keyauth= ascii hex auth for sealing key default 0x00...i + (40 ascii zeros) + blobauth= ascii hex auth for sealed data default 0x00... + (40 ascii zeros) + blobauth= ascii hex auth for sealed data default 0x00... + (40 ascii zeros) + pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) + pcrlock= pcr number to be extended to "lock" blob + migratable= 0|1 indicating permission to reseal to new PCR values, + default 1 (resealing allowed) + hash= hash algorithm name as a string. For TPM 1.x the only + allowed value is sha1. For TPM 2.x the allowed values + are sha1, sha256, sha384, sha512 and sm3-256. + policydigest= digest for the authorization policy. must be calculated + with the same hash algorithm as specified by the 'hash=' + option. + policyhandle= handle to an authorization policy session that defines the + same policy and with the same hash algorithm as was used to + seal the key. "keyctl print" returns an ascii hex copy of the sealed key, which is in standard TPM_STORED_DATA format. The key length for new keys are always in bytes. diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index d9d0822..45a6340 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, tpm_buf_append_u8(, payload->migratable); /* public */ - tpm_buf_append_u16(, 14); + if (options->policydigest) + tpm_buf_append_u16(, 14 + options->digest_len); + else + tpm_buf_append_u16(, 14); tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH); tpm_buf_append_u16(, hash); - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); - tpm_buf_append_u16(, 0); /* policy digest size */ + + /* policy */ + if (options->policydigest) { + tpm_buf_append_u32(, 0); + tpm_buf_append_u16(, options->digest_len); + tpm_buf_append(, options->policydigest, + options->digest_len); + } else { + tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); + tpm_buf_append_u16(, 0); + } + + /* public parameters */ tpm_buf_append_u16(, TPM2_ALG_NULL); tpm_buf_append_u16(, 0); @@ -613,7 +627,9 @@ static int tpm2_unseal(struct tpm_chip *chip, return rc; tpm_buf_append_u32(, blob_handle); - tpm2_buf_append_auth(, TPM2_RS_PW, + tpm2_buf_append_auth(, +options->policyhandle ? +options->policyhandle : TPM2_RS_PW, NULL /* nonce */, 0, 0 /* session_attributes */, options->blobauth /* hmac */, diff --git a/include/keys/trusted-type.h
[PATCH 2/2] keys, trusted: seal with a policy
Support for sealing with a authorization policy. Two new options for trusted keys: * 'policydigest=': provide an auth policy digest for sealing. * 'policyhandle=': provide a policy session handle for unsealing. Signed-off-by: Jarkko Sakkinen--- Documentation/security/keys-trusted-encrypted.txt | 34 ++--- drivers/char/tpm/tpm2-cmd.c | 24 ++-- include/keys/trusted-type.h | 3 ++ security/keys/trusted.c | 46 ++- 4 files changed, 87 insertions(+), 20 deletions(-) diff --git a/Documentation/security/keys-trusted-encrypted.txt b/Documentation/security/keys-trusted-encrypted.txt index fd2565b..324ddf5 100644 --- a/Documentation/security/keys-trusted-encrypted.txt +++ b/Documentation/security/keys-trusted-encrypted.txt @@ -27,20 +27,26 @@ Usage: keyctl print keyid options: - keyhandle= ascii hex value of sealing key default 0x4000 (SRK) - keyauth= ascii hex auth for sealing key default 0x00...i - (40 ascii zeros) - blobauth= ascii hex auth for sealed data default 0x00... - (40 ascii zeros) - blobauth= ascii hex auth for sealed data default 0x00... - (40 ascii zeros) - pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) - pcrlock= pcr number to be extended to "lock" blob - migratable= 0|1 indicating permission to reseal to new PCR values, - default 1 (resealing allowed) - hash= hash algorithm name as a string. For TPM 1.x the only - allowed value is sha1. For TPM 2.x the allowed values - are sha1, sha256, sha384, sha512 and sm3-256. + keyhandle=ascii hex value of sealing key default 0x4000 (SRK) + keyauth= ascii hex auth for sealing key default 0x00...i + (40 ascii zeros) + blobauth= ascii hex auth for sealed data default 0x00... + (40 ascii zeros) + blobauth= ascii hex auth for sealed data default 0x00... + (40 ascii zeros) + pcrinfo= ascii hex of PCR_INFO or PCR_INFO_LONG (no default) + pcrlock= pcr number to be extended to "lock" blob + migratable= 0|1 indicating permission to reseal to new PCR values, + default 1 (resealing allowed) + hash= hash algorithm name as a string. For TPM 1.x the only + allowed value is sha1. For TPM 2.x the allowed values + are sha1, sha256, sha384, sha512 and sm3-256. + policydigest= digest for the authorization policy. must be calculated + with the same hash algorithm as specified by the 'hash=' + option. + policyhandle= handle to an authorization policy session that defines the + same policy and with the same hash algorithm as was used to + seal the key. "keyctl print" returns an ascii hex copy of the sealed key, which is in standard TPM_STORED_DATA format. The key length for new keys are always in bytes. diff --git a/drivers/char/tpm/tpm2-cmd.c b/drivers/char/tpm/tpm2-cmd.c index d9d0822..45a6340 100644 --- a/drivers/char/tpm/tpm2-cmd.c +++ b/drivers/char/tpm/tpm2-cmd.c @@ -478,12 +478,26 @@ int tpm2_seal_trusted(struct tpm_chip *chip, tpm_buf_append_u8(, payload->migratable); /* public */ - tpm_buf_append_u16(, 14); + if (options->policydigest) + tpm_buf_append_u16(, 14 + options->digest_len); + else + tpm_buf_append_u16(, 14); tpm_buf_append_u16(, TPM2_ALG_KEYEDHASH); tpm_buf_append_u16(, hash); - tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); - tpm_buf_append_u16(, 0); /* policy digest size */ + + /* policy */ + if (options->policydigest) { + tpm_buf_append_u32(, 0); + tpm_buf_append_u16(, options->digest_len); + tpm_buf_append(, options->policydigest, + options->digest_len); + } else { + tpm_buf_append_u32(, TPM2_ATTR_USER_WITH_AUTH); + tpm_buf_append_u16(, 0); + } + + /* public parameters */ tpm_buf_append_u16(, TPM2_ALG_NULL); tpm_buf_append_u16(, 0); @@ -613,7 +627,9 @@ static int tpm2_unseal(struct tpm_chip *chip, return rc; tpm_buf_append_u32(, blob_handle); - tpm2_buf_append_auth(, TPM2_RS_PW, + tpm2_buf_append_auth(, +options->policyhandle ? +options->policyhandle : TPM2_RS_PW, NULL /* nonce */, 0, 0 /* session_attributes */, options->blobauth /* hmac */, diff --git
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > } > break; > + case Opt_policydigest: > + if (!tpm2 || > + strlen(args[0].from) != (2 * opt->digest_len)) > + return -EINVAL; > + kfree(opt->policydigest); > + opt->policydigest = kzalloc(opt->digest_len, > + GFP_KERNEL); Is it correct to kfree opt->policydigest here before allocating it? > + if (!opt->policydigest) > + return -ENOMEM; > + res = hex2bin(opt->policydigest, args[0].from, > + opt->digest_len); > + if (res < 0) > + return -EINVAL; Do you need to kfree it here on error? -- James Morris-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/2] keys, trusted: seal with a policy
On Wed, Nov 18, 2015 at 11:21:01AM +1100, James Morris wrote: > On Tue, 17 Nov 2015, Jarkko Sakkinen wrote: > > > } > > break; > > + case Opt_policydigest: > > + if (!tpm2 || > > + strlen(args[0].from) != (2 * opt->digest_len)) > > + return -EINVAL; > > + kfree(opt->policydigest); > > + opt->policydigest = kzalloc(opt->digest_len, > > + GFP_KERNEL); > > Is it correct to kfree opt->policydigest here before allocating it? I think so. The same option might be encountered multiple times. I don't have the check for nulliy because opt is kzalloc'd and checkpatch.pl complained that WARNING: kfree(NULL) is safe and this check is probably not required #20: FILE: security/keys/trusted.c:829: + if (opt->policydigest) + kfree(opt->policydigest); > > + if (!opt->policydigest) > > + return -ENOMEM; > > + res = hex2bin(opt->policydigest, args[0].from, > > + opt->digest_len); > > + if (res < 0) > > + return -EINVAL; > > Do you need to kfree it here on error? trusted_options_free() will kfree it. > -- > James Morris >/Jarkko -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/