Re: [PATCH] crypto: ccm - avoid scatterlist for MAC encryption

On Mon, Oct 17, 2016 at 12:37 AM, Ard Biesheuvel
[off-list ref] wrote:

On 17 October 2016 at 08:28, Johannes Berg [off-list ref] wrote:

quoted

On Sat, 2016-10-15 at 18:16 +0100, Ard Biesheuvel wrote:

quoted

The CCM code goes out of its way to perform the CTR encryption of the
MAC using the subordinate CTR driver. To this end, it tweaks the
input and output scatterlists so the aead_req 'odata' and/or
'auth_tag' fields [which may live on the stack] are prepended to the
CTR payload. This involves calling sg_set_buf() on addresses which
are not direct mapped, which is not supported.

quoted

Since the calculation of the MAC keystream involves a single call
into the cipher, to which we have a handle already given that the
CBC-MAC calculation uses it as well, just calculate the MAC keystream
directly, and record it in the aead_req private context so we can
apply it to the MAC in cypto_ccm_auth_mac(). This greatly simplifies
the scatterlist manipulation, and no longer requires scatterlists to
refer to buffers that may live on the stack.

No objection from me, Herbert?

I'm getting a bit nervous though - I'd rather have any fix first so
people get things working again - so maybe I'll apply your other patch
and mine first, and then we can replace yours by this later.

Could we get a statement first whether it is supported to allocate
aead_req (and other crypto req structures) on the stack? If not, then
we have our work cut out for us. But if it is, I'd rather we didn't
apply the kzalloc/kfree patch, since it is just a workaround for the
broken generic CCM driver, for which a fix is already available.

I'm not a crypto person, but I don't see why not.  There's even a
helper called SKCIPHER_REQUEST_ON_STACK. :)  The only problem I know
of is pointing a scatterlist at the stack, which is bad for much the
same reason as doing real DMA from the stack.