[PATCH v2 1/3] staging: ccree: copy IV to DMAable memory

[Gilad Ben-Yossef]

Hi,

On Wed, Nov 8, 2017 at 12:26 PM, Horia Geantă <horia.geanta at nxp.com> wrote:
> On 11/2/2017 10:14 AM, Gilad Ben-Yossef wrote:
>> We are being passed an IV buffer from unknown origin, which may be
>> stack allocated and thus not safe for DMA. Allocate a DMA safe
>> buffer for the IV and use that instead.
>>
> IIUC this fixes only the (a)blkcipher / skcipher algorithms.
> What about aead, authenc?

AFAIK the implementation of aead/authenc in ccree already copies the IV to a
internally allocated buffer because how it deals with GCM and CTR.

But you did trigger me to double check that, so thanks for that :-)

>
> The fact that only the skcipher tcrypt tests use IVs on stack doesn't
> mean aead, authenc implementations are safe - other crypto API users
> could provide IVs laying in non-DMAable memory.

Of course. In fact, it might be a good idea to actually make tcrypt
explicitly use a stack allocated IV just to trigger proper warnings
from the DMA debug code. Does that sounds sane?

>
> To reiterate, the proper approach is to fix the crypto API to guarantee
> IVs are DMAable.
> However Herbert suggests he is not willing to do this work:
> https://www.mail-archive.com/linux-crypto@vger.kernel.org/msg28821.html
>
> A few high-level details mentioning what this implies would be helpful,
> in case somebody else decides its worth pursuing this path.
>
> The compromise is to fix all crypto drivers that need DMAable IVs.
> IMHO this is suboptimal, both in terms of performance (memory
> allocation, memcpy) and increased code complexity.

As a HW based crypto driver maintainer I sympathize, but let's play
devil's advocate for a second here:

In the current state, HW based crypto drivers need to allocate a buffer
and copy the IV, because they don't know if they got a DMAable buffer
or not. SW based crypto drivers don't need to do anything.

If we switch to an API that guarantee IVs are DMAable all crypto API
*users* will be forced to make sure IV are located in DMAable memory,
possibly resulting in the need for extra buffer allocation, whether this is
needed or not and SW based crypto drivers suffer added complexicty
of extracting the IV from scatter/gather list (I'm assuming we'll just add
it there).

Despite this hurting the driver I care about, I'm not sure this is a good trade
off, really.

Thinking aloud, would it be sane to add an API of "alloc and copy if buffer
is on stack"?

Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

"If you take a class in large-scale robotics, can you end up in a
situation where the homework eats your dog?"
 -- Jean-Baptiste Queru