Re: [PATCH] crypto: caam - Do not overwrite IV

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Wed, 13 Feb 2019 at 21:48, Horia Geanta <horia.geanta@nxp.com> wrote:
>
> On 2/12/2019 11:13 AM, Ard Biesheuvel wrote:
> > On Fri, 8 Feb 2019 at 09:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >>
> >> On Fri, 8 Feb 2019 at 09:41, Horia Geanta <horia.geanta@nxp.com> wrote:
> >>>
> >>> On 2/8/2019 9:16 AM, Herbert Xu wrote:
> >>>> On Mon, Feb 04, 2019 at 12:26:26PM +0000, Horia Geanta wrote:
> >>>>>
> >>>>> The root cause of the issue is cache line sharing.
> >>>>>
> >>>>> struct crypto_gcm_req_priv_ctx {
> >>>>>         u8 iv[16];
> >>>>>         u8 auth_tag[16];
> >>>>>      [...]
> >>>>> };
> >>>>>
> >>>>> Since caam does not support ghash on i.MX6, only ctr skcipher part of the gcm is
> >>>>> offloaded.
> >>>>> The skcipher request received by caam has req->src pointing to auth_tag[16] (1st
> >>>>> S/G entry) and req->iv pointing to iv[16].
> >>>>> caam driver:
> >>>>> 1-DMA maps req->src
> >>>>> 2-copies original req->iv to internal buffer
> >>>>> 3-updates req->iv (scatterwalk_map_and_copy from last block in req->src)
> >>
> >> This violates the DMA api, since you are touching memory that is owned
> >> by the device at this point (even though the addresses do not actually
> >> overlap). Note that on architectures that support non-cache coherent
> >> DMA, the kmalloc alignment is at least the cacheline size, for this
> >> exact reason.
> >>
> >
> > Actually, the driver does violate the DMA api in another way:
> > scatterwalk_map_and_copy() is accessing req->src after DMA mapping it.
> > Does the issue still exist if scatterwalk_map_and_copy() is done
> > before the DMA map?
> >
> > (On a non-cache coherent system, the DMA map will typically perform a
> > clean+invalidate, which means that the invalidate that occurs at unmap
> > time cannot corrupt adjacent data, but this only works if the CPU does
> > not write to the same cacheline while it is mapped for the device)
> >
> scatterwalk_map_and_copy() is reading from req->src.
> Are you saying it's forbidden for CPU to read from an area after it's DMA mapped?
>

OK, I read that the wrong way around. The DMA api does permit reading
from a region after it has been mapped for DMA.