Re: [PATCH v2] KEYS: Convert KEYCTL_DH_COMPUTE to use the crypto KPP API

[Eric Biggers <ebiggers3@gmail.com>]

Hi Stephan,

On Mon, Jun 05, 2017 at 11:51:32PM +0200, Stephan Müller wrote:
> Am Montag, 5. Juni 2017, 05:41:32 CEST schrieb Eric Biggers:
> 
> Hi Eric,
> > 
> > Also, Stephan, what is the purpose of the "__spare" field in
> > keyctl_kdf_params? How is it ever supposed to be used for anything if the
> > kernel doesn't even validate that it contains 0's?  Some flag hacked into
> > the 'otherinfolen' field...?
> 
> The idea in the very first patch set was the presence of a "flags" field. We 
> have a proliferation of system calls in the last few years which just add a 
> "flags" parameter compared to an identical older system call.
> 
> To prevent falling into this trap, I wanted to have a flags field. The core 
> reason is that there are multiple options allowed and possible how to derive a 
> key. The current approach is the easiest and smallest one, i.e. using the 
> SP800-108 counter KDF. Other KDFs are possible too. To allow such openness, we 
> decided to leave some room.
> 
> See https://patchwork.kernel.org/patch/9224837/ and search for __spare[8] in 
> the answer from Mat.
> 

Well, regardless of what it's reserved *for* (if anything), this is *not* the
correct way to do a reserved field.  The API needs to verify that it has value 0
and return -EINVAL otherwise, i.e.

	if (memchr_inv(kdfcopy->__spare, 0, sizeof(kdfcopy->__spare)))
		return -EINVAL;

It can't simply be ignored, otherwise users will have no way to know whether any
new fields are recognized or not.  Also if there is no validation, users will
screw up and pass in a nonzero value (perhaps through leaving it uninitialized)
before it's un-reserved, which will then break as soon as the space is actually
used for something.

Eric