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

From: Robin Murphy <robin.murphy@arm.com>
To: Gilad Ben-Yossef <gilad@benyossef.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>,
	Linux kernel mailing list <linux-kernel@vger.kernel.org>,
	iommu@lists.linux-foundation.org,
	Corentin Labbe <clabbe.montjoie@gmail.com>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [BUG] crypto: ccree: driver does not handle case where cryptlen = authsize =0
Date: Mon, 7 Mar 2022 13:03:28 +0000	[thread overview]
Message-ID: <371ef3f2-883d-91ab-ed96-da8921efb465@arm.com> (raw)
In-Reply-To: <CAOtvUMedqSNKx9Aah0R_aAyjKO0pn4K75MrCnbh_zX+Zw9vRQA@mail.gmail.com>

On 2022-03-07 12:47, Gilad Ben-Yossef wrote:
> On Mon, Mar 7, 2022 at 2:36 PM Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2022-03-07 12:17, Gilad Ben-Yossef wrote:
>>> On Mon, Mar 7, 2022 at 1:14 PM Robin Murphy <robin.murphy@arm.com> 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?

Because it's the second one that isn't. The warning is triggered by 
adding the DMA_FROM_DEVICE entry, which *is* checked, and finds the 
DMA_TO_DEVICE entry already present. What's not great is that if those 
two mappings happened to be made in the opposite order then it would be 
missed entirely.

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu