[lkml]   [2014]   [Dec]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
SubjectRe: [PATCH v6 1/4] crypto: AF_ALG: add AEAD support
Am Montag, 29. Dezember 2014, 21:33:19 schrieb Herbert Xu:

Hi Herbert,

> On Thu, Dec 25, 2014 at 11:01:47PM +0100, Stephan Mueller wrote:
> > + err = -ENOMEM;
> This should be EINVAL.

> > + if (!aead_sufficient_data(ctx))
> > + goto unlock;
> So we're checking two things here, one that we have enough data
> for AD and two we have the authentication tag. The latter is
> redundant as the underlying implementation should be able to cope
> with short input so we should only check the assoclen here.

Agreed, will change it to

if (ctx->used < ctx->aead_assoclen)

> Also this check should be moved to the sendmsg side as that'll
> make it more obvious as to what went wrong.

I would be a bit uneasy about that as this would open up a potential kernel
crasher: the sleep in aead_readable() can wake up recvmsg in two conditions:
either we received sufficient data or we do not expect more data (due to !ctx-
>more). If the latter triggers, we still may have insufficient AD data. Yet,
the following code now sets the AD with aead_request_set_assoc using the
initially expected data. So, the data buffer provided to
aead_request_set_assoc is not long enough. The mentioned check shall prevent
this problem.

In addition, I do not see how we can move that check to the sendmsg/sendpage
side: the code currently allows the caller to freely invoke the syscall
arbitrary amount of times. Thus, one particular invocation of sendmsg/sendpage
does not mean we receive all AD.

Again, to allow the caller the greatest degree of freedom, you can call
sendmsg with an arbitrary amount of bytes as often as you want (until we fill
up all buffers) before the recvmsg is triggered. So, there is no need to send
the entire AD (or even AD+message) buffer in one sendmsg call. Compare the
AEAD interface with a hash interface:

- the AEAD sendmsg/sendpage is logically equivalent to a hash update that you
can call an arbitrary number of times with an arbitrary number of bytes.

- the AEAD recvmsg is logically equivalent to the hash final.

This would mean that the check must stay in recvmsg as only here we know that
the caller wants data to be processed.

> PS we should add a length check for missing/partial auth tags
> to crypto_aead_decrypt. We can then remove such checks from
> individual implementations.

I agree in full here. Shall I create such a patch together with the AEAD
AF_ALG interface, or can we merge the AEAD without that patch now and create a
separate patch later?
> Thanks,


 \ /
  Last update: 2014-12-29 16:21    [W:0.193 / U:0.064 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site