Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1285117 > unrolled thread

Re: [PATCH 2/2] keys, trusted: seal with a policy

Started byJarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
First post2015-12-07 10:20 +0100
Last post2015-12-09 17:20 +0100
Articles 7 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 2/2] keys, trusted: seal with a policy Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-07 10:20 +0100
    Re: [PATCH 2/2] keys, trusted: seal with a policy James Morris <jmorris@namei.org> - 2015-12-07 23:40 +0100
      Re: [PATCH 2/2] keys, trusted: seal with a policy Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-08 12:10 +0100
        Re: [PATCH 2/2] keys, trusted: seal with a policy Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-08 21:30 +0100
          Re: [PATCH 2/2] keys, trusted: seal with a policy Mimi Zohar <zohar@linux.vnet.ibm.com> - 2015-12-09 01:00 +0100
            Re: [PATCH 2/2] keys, trusted: seal with a policy Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> - 2015-12-09 15:30 +0100
              Re: [PATCH 2/2] keys, trusted: seal with a policy Mimi Zohar <zohar@linux.vnet.ibm.com> - 2015-12-09 17:20 +0100

#1285117 — Re: [PATCH 2/2] keys, trusted: seal with a policy

FromJarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date2015-12-07 10:20 +0100
SubjectRe: [PATCH 2/2] keys, trusted: seal with a policy
Message-ID<qD07F-2A3-41@gated-at.bofh.it>
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=0x80000000 keyhandle=0x80000000"

is perfectly OK. I just thought that it'd be more odd if this option
behaved in a different way...

> -- 
> James Morris
> <jmorris@namei.org>

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1286055

FromJames Morris <jmorris@namei.org>
Date2015-12-07 23:40 +0100
Message-ID<qDcBR-2jb-39@gated-at.bofh.it>
In reply to#1285117
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=0x80000000 keyhandle=0x80000000"
> 
> 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
<jmorris@namei.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286392

FromJarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date2015-12-08 12:10 +0100
Message-ID<qDojD-1AO-3@gated-at.bofh.it>
In reply to#1286055
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=0x80000000 keyhandle=0x80000000"
> > 
> > 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
> <jmorris@namei.org>

/Jarkko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286830

FromJarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date2015-12-08 21:30 +0100
Message-ID<qDx3A-728-21@gated-at.bofh.it>
In reply to#1286392
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=0x80000000 keyhandle=0x80000000"
> > > 
> > > 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286923

FromMimi Zohar <zohar@linux.vnet.ibm.com>
Date2015-12-09 01:00 +0100
Message-ID<qDAkO-yZ-9@gated-at.bofh.it>
In reply to#1286830
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=0x80000000 keyhandle=0x80000000"
> > > > 
> > > > 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287521

FromJarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Date2015-12-09 15:30 +0100
Message-ID<qDNUJ-15S-1@gated-at.bofh.it>
In reply to#1286923
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=0x80000000 keyhandle=0x80000000"
> > > > > 
> > > > > 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287619

FromMimi Zohar <zohar@linux.vnet.ibm.com>
Date2015-12-09 17:20 +0100
Message-ID<qDPDc-2e2-11@gated-at.bofh.it>
In reply to#1287521
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=0x80000000 keyhandle=0x80000000"
> > > > > > 
> > > > > > 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, <flag>) after the following code
snippet.

          while ((p = strsep(&c, " \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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web