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

[Mimi Zohar <zohar@linux.vnet.ibm.com>]

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