Memory clearing in swiotlb_update_mem_attributes()

Memory clearing in swiotlb_update_mem_attributes()

[Kirill A. Shutemov]

Hi Tom,

For larger TDX VM, memset() after set_memory_decrypted() in
swiotlb_update_mem_attributes() takes substantial portion of boot time.

It makes me wounder why do we need it there? Malicious VMM can mess with
decrypted/shared buffer at any point and for normal use it will be
populated with real data anyway.

Can we drop it?

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

Re: Memory clearing in swiotlb_update_mem_attributes()

[Tom Lendacky via iommu]

On 1/4/22 4:49 PM, Kirill A. Shutemov wrote:
> Hi Tom,
> 
> For larger TDX VM, memset() after set_memory_decrypted() in
> swiotlb_update_mem_attributes() takes substantial portion of boot time.
> 
> It makes me wounder why do we need it there? Malicious VMM can mess with
> decrypted/shared buffer at any point and for normal use it will be
> populated with real data anyway.
> 
> Can we drop it?

Probably more a question for Christoph. Does SWIOTLB need to be 
initialized to zeroes? If it does, then the memset after the 
set_memory_decrypted() is required, otherwise it will appear as ciphertext 
to SWIOTLB.

If I get some time over the next couple of days, I can also try and test 
to see what happens.

Thanks,
Tom

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

Re: Memory clearing in swiotlb_update_mem_attributes()

[Christoph Hellwig]

On Wed, Jan 05, 2022 at 08:06:10AM -0600, Tom Lendacky wrote:
> On 1/4/22 4:49 PM, Kirill A. Shutemov wrote:
> > Hi Tom,
> > 
> > For larger TDX VM, memset() after set_memory_decrypted() in
> > swiotlb_update_mem_attributes() takes substantial portion of boot time.
> > 
> > It makes me wounder why do we need it there? Malicious VMM can mess with
> > decrypted/shared buffer at any point and for normal use it will be
> > populated with real data anyway.
> > 
> > Can we drop it?
> 
> Probably more a question for Christoph. Does SWIOTLB need to be initialized
> to zeroes? If it does, then the memset after the set_memory_decrypted() is
> required, otherwise it will appear as ciphertext to SWIOTLB.

While the traditional swiotlb initialization zeroes it I can't really
see any reason why we would want to zero it.  If we really care about
not leaking data to the device we'd need to zero the padding at mapping
time.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Memory clearing in swiotlb_update_mem_attributes()

[Kirill A. Shutemov]

On Wed, Jan 05, 2022 at 06:12:34AM -0800, Christoph Hellwig wrote:
> On Wed, Jan 05, 2022 at 08:06:10AM -0600, Tom Lendacky wrote:
> > On 1/4/22 4:49 PM, Kirill A. Shutemov wrote:
> > > Hi Tom,
> > > 
> > > For larger TDX VM, memset() after set_memory_decrypted() in
> > > swiotlb_update_mem_attributes() takes substantial portion of boot time.
> > > 
> > > It makes me wounder why do we need it there? Malicious VMM can mess with
> > > decrypted/shared buffer at any point and for normal use it will be
> > > populated with real data anyway.
> > > 
> > > Can we drop it?
> > 
> > Probably more a question for Christoph. Does SWIOTLB need to be initialized
> > to zeroes? If it does, then the memset after the set_memory_decrypted() is
> > required, otherwise it will appear as ciphertext to SWIOTLB.
> 
> While the traditional swiotlb initialization zeroes it I can't really
> see any reason why we would want to zero it.  If we really care about
> not leaking data to the device we'd need to zero the padding at mapping
> time.

Does the patch below look fine?

From ab1aa6abbdbba9e34ac7b86e5af1f9f488afaa07 Mon Sep 17 00:00:00 2001
From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Date: Wed, 5 Jan 2022 18:48:01 +0300
Subject: [PATCH] swiotlb: Do not zero buffer in set_memory_decrypted()

For larger TDX VM, memset() after set_memory_decrypted() in
swiotlb_update_mem_attributes() takes substantial portion of boot time.

Zeroing doesn't serve any functional purpose. Malicious VMM can mess
with decrypted/shared buffer at any point.

Remove the memset().

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 kernel/dma/swiotlb.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/dma/swiotlb.c b/kernel/dma/swiotlb.c
index 8e840fbbed7c..4546c834accb 100644
--- a/kernel/dma/swiotlb.c
+++ b/kernel/dma/swiotlb.c
@@ -172,7 +172,6 @@ void __init swiotlb_update_mem_attributes(void)
 	vaddr = phys_to_virt(mem->start);
 	bytes = PAGE_ALIGN(mem->nslabs << IO_TLB_SHIFT);
 	set_memory_decrypted((unsigned long)vaddr, bytes >> PAGE_SHIFT);
-	memset(vaddr, 0, bytes);
 }
 
 static void swiotlb_init_io_tlb_mem(struct io_tlb_mem *mem, phys_addr_t start,
-- 
 Kirill A. Shutemov
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: Memory clearing in swiotlb_update_mem_attributes()

[Tom Lendacky via iommu]

On 1/5/22 8:06 AM, Tom Lendacky wrote:
> On 1/4/22 4:49 PM, Kirill A. Shutemov wrote:
>> Hi Tom,
>>
>> For larger TDX VM, memset() after set_memory_decrypted() in
>> swiotlb_update_mem_attributes() takes substantial portion of boot time.
>>
>> It makes me wounder why do we need it there? Malicious VMM can mess with
>> decrypted/shared buffer at any point and for normal use it will be
>> populated with real data anyway.
>>
>> Can we drop it?
> 
> Probably more a question for Christoph. Does SWIOTLB need to be 
> initialized to zeroes? If it does, then the memset after the 
> set_memory_decrypted() is required, otherwise it will appear as ciphertext 
> to SWIOTLB.
> 
> If I get some time over the next couple of days, I can also try and test 
> to see what happens.

FWIW, I removed the memset() and it doesn't appear to cause any issues. My 
tests weren't extensive (boot, kernel build, netperf/iperf), though.

Thanks,
Tom

> 
> Thanks,
> Tom
> 
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu