[PATCH] swiotlb: fix a typo

[PATCH] swiotlb: fix a typo

[Chao Gao]

"overwirte" isn't a word. It should be "overwrite".

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
BTW, I am wondering if copying the original buffer to the tlb buffer
unconditionally will leak the original buffer to the VMM, especially
when VMM isn't trusted e.g., by confidential VMs. Would it be better
to zero the tlb buffer for dir == DMA_FROM_DEVICE?

 kernel/dma/swiotlb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index c5a9190b218f..f67e5f50ee3c 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -765,7 +765,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
 	/*
 	 * When dir == DMA_FROM_DEVICE we could omit the copy from the orig
 	 * to the tlb buffer, if we knew for sure the device will
-	 * overwirte the entire current content. But we don't. Thus
+	 * overwrite the entire current content. But we don't. Thus
 	 * unconditional bounce may prevent leaking swiotlb content (i.e.
 	 * kernel memory) to user-space.
 	 */
-- 
2.25.1

Re: [PATCH] swiotlb: fix a typo

[Robin Murphy]

On 2022-08-26 10:50, Chao Gao wrote:
> "overwirte" isn't a word. It should be "overwrite".
> 
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> BTW, I am wondering if copying the original buffer to the tlb buffer
> unconditionally will leak the original buffer to the VMM, especially
> when VMM isn't trusted e.g., by confidential VMs. Would it be better
> to zero the tlb buffer for dir == DMA_FROM_DEVICE?

No, at the point of dma_map(), the buffer contents are owned by the 
caller, so if parts of that buffer are sensitive and shouldn't be 
exposed to DMA, then don't map the whole buffer for DMA. There are more 
DMA API implementations than SWIOTLB.

The whole point of bouncing the original contents here is that doing 
anything else effectively corrupts any part of the mapping that the 
device may end up *not* writing to - see the whole sordid original 
discussion (but don't be confused by the fact that the caller's original 
data happened to be zeros in that particular case).

Thanks,
Robin.

> 
>   kernel/dma/swiotlb.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
> index c5a9190b218f..f67e5f50ee3c 100644
> --- a/kernel/dma/swiotlb.c
> +++ b/kernel/dma/swiotlb.c
> @@ -765,7 +765,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *dev, phys_addr_t orig_addr,
>   	/*
>   	 * When dir == DMA_FROM_DEVICE we could omit the copy from the orig
>   	 * to the tlb buffer, if we knew for sure the device will
> -	 * overwirte the entire current content. But we don't. Thus
> +	 * overwrite the entire current content. But we don't. Thus
>   	 * unconditional bounce may prevent leaking swiotlb content (i.e.
>   	 * kernel memory) to user-space.
>   	 */

Re: [PATCH] swiotlb: fix a typo

[Chao Gao]

On Tue, Aug 30, 2022 at 10:23:51AM +0100, Robin Murphy wrote:
>On 2022-08-26 10:50, Chao Gao wrote:
>> "overwirte" isn't a word. It should be "overwrite".
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> BTW, I am wondering if copying the original buffer to the tlb buffer
>> unconditionally will leak the original buffer to the VMM, especially
>> when VMM isn't trusted e.g., by confidential VMs. Would it be better
>> to zero the tlb buffer for dir == DMA_FROM_DEVICE?
>
>No, at the point of dma_map(), the buffer contents are owned by the caller,
>so if parts of that buffer are sensitive and shouldn't be exposed to DMA,
>then don't map the whole buffer for DMA. There are more DMA API
>implementations than SWIOTLB.
>

I am not sure if all existing drivers ensure that all buffers allocated
for DMA_FROM_DEVICE are zeroed/poisoned so that they don't have sensitive
data before dma_map(). If that isn't the case, bouncing the original contents
(left by the previous user of the buffer) effectively makes the contents
visible to host/VMM. I am afraid it may be a concern for confidential VMs.

>The whole point of bouncing the original contents here is that doing anything
>else effectively corrupts any part of the mapping that the device may end up
>*not* writing to - see the whole sordid original discussion (but don't be
>confused by the fact that the caller's original data happened to be zeros in
>that particular case).

Got it. Thanks for the explanation.

Re: [PATCH] swiotlb: fix a typo

[Robin Murphy]

On 2022-08-31 05:22, Chao Gao wrote:
> On Tue, Aug 30, 2022 at 10:23:51AM +0100, Robin Murphy wrote:
>> On 2022-08-26 10:50, Chao Gao wrote:
>>> "overwirte" isn't a word. It should be "overwrite".
>>>
>>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>>> ---
>>> BTW, I am wondering if copying the original buffer to the tlb buffer
>>> unconditionally will leak the original buffer to the VMM, especially
>>> when VMM isn't trusted e.g., by confidential VMs. Would it be better
>>> to zero the tlb buffer for dir == DMA_FROM_DEVICE?
>>
>> No, at the point of dma_map(), the buffer contents are owned by the caller,
>> so if parts of that buffer are sensitive and shouldn't be exposed to DMA,
>> then don't map the whole buffer for DMA. There are more DMA API
>> implementations than SWIOTLB.
>>
> 
> I am not sure if all existing drivers ensure that all buffers allocated
> for DMA_FROM_DEVICE are zeroed/poisoned so that they don't have sensitive
> data before dma_map(). If that isn't the case, bouncing the original contents
> (left by the previous user of the buffer) effectively makes the contents
> visible to host/VMM. I am afraid it may be a concern for confidential VMs.

Sure, and in a scheme where pages can be dynamically shared in-place 
instead of using SWIOTLB to bounce through a pre-shared buffer, then 
those same drivers will still expose the same stale data. Similarly, a 
driver could massively over-map with DMA_TO_DEVICE or DMA_BIDIRECTIONAL 
and expose all manner of potential secrets that way. It's a concern that 
cannot be addressed at the DMA API level. Anyone who wants to completely 
trust drivers not to do anything stupid in a confidential compute 
scenario is going to have to audit and possibly fix those drivers.

Robin.

Re: [PATCH] swiotlb: fix a typo

[Christoph Hellwig]

Thanks, applied.