Re: [BUG] crypto: ccree: driver does not handle case where cryptlen = authsize =0

[Gilad Ben-Yossef]

On Mon, Mar 7, 2022 at 2:36 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
>
> On 2022-03-07 12:17, Gilad Ben-Yossef wrote:
> > On Mon, Mar 7, 2022 at 1:14 PM Robin Murphy <robin.murphy@xxxxxxx> wrote:
> >
> >> The "overlap" is in the sense of having more than one mapping within the
> >> same cacheline:
> >>
> >> [  142.458120] DMA-API: add_dma_entry start P=ba79f200 N=ba79f
> >> D=ba79f200 L=10 DMA_FROM_DEVICE attrs=0
> >> [  142.458156] DMA-API: add_dma_entry start P=445dc010 N=445dc
> >> D=445dc010 L=10 DMA_TO_DEVICE attrs=0
> >> [  142.458178] sun8i-ss 1c15000.crypto: SRC 0/1/1 445dc000 len=16 bi=0
> >> [  142.458215] sun8i-ss 1c15000.crypto: DST 0/1/1 ba79f200 len=16 bi=0
> >> [  142.458234] DMA-API: add_dma_entry start P=ba79f210 N=ba79f
> >> D=ba79f210 L=10 DMA_FROM_DEVICE attrs=0
> >>
> >> This actually illustrates exactly the reason why this is unsupportable.
> >> ba79f200 is mapped for DMA_FROM_DEVICE, therefore subsequently mapping
> >> ba79f210 for DMA_TO_DEVICE may cause the cacheline covering the range
> >> ba79f200-ba79f23f to be written back over the top of data that the
> >> device has already started to write to memory. Hello data corruption.
> >>
> >> Separate DMA mappings should be from separate memory allocations,
> >> respecting ARCH_DMA_MINALIGN.
> >
> > hmm... I know I'm missing something here, but how does this align with
> > the following from active_cacheline_insert() in kernel/dma/debug.c ?
> >
> >          /* If the device is not writing memory then we don't have any
> >           * concerns about the cpu consuming stale data.  This mitigates
> >           * legitimate usages of overlapping mappings.
> >           */
> >          if (entry->direction == DMA_TO_DEVICE)
> >                  return 0;
>
> It's OK to have multiple mappings that are *all* DMA_TO_DEVICE, which
> looks to be the case that this check was intended to allow. However I
> think you're right that it should still actually check for conflicting
> directions between the new entry and any existing ones, otherwise it
> ends up a bit too lenient.
>
> Cheers,
> Robin.

I understand what you are saying about why checking for conflicting
directions may be a good thing, but given that the code is as it is
right now, how are we seeing the warning for two mapping that one of
them is DMA_TO_DEVICE?

Gilad


-- 
Gilad Ben-Yossef
Chief Coffee Drinker

values of β will give rise to dom!