dma-direct: set_memory_{en,de}crypted() take number of pages
Subsystems (4)
PATCH 1/2 dma-direct: set_memory_{en,de}crypted() take number of pages
From: Ram Pai <[email protected]>
set_memory_encrypted() and set_memory_decrypted() expect the number of PAGE_SIZE pages to encrypt or decrypt. dma_direct_alloc() and dma_direct_free() instead pass number of bytes. This encrypts/decrypts a huge number of pages resulting in data corruption.
Fixed it.
--- kernel/dma/direct.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)
Notes:
1. This was tested on powerpc with patches adding support for running under the ultravisor, which are not yet upstream.
2. The lines changed in this patch were added by commit c10f07aa27da ("dma/direct: Handle force decryption for DMA coherent buffers in common code"), but it only moves the code from an x86-specific file. Therefore the Fixes tag references the commit that first introduced the code.
kernel/dma/direct.c +2 -4
@@ -166,7 +166,7 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size, ret =page_address (page);if (force_dma_unencrypted ()) { -set_memory_decrypted ((unsigned long )ret,1 <<get_order (size)); +set_memory_decrypted ((unsigned long )ret,1 ); *dma_handle =__phys_to_dma (dev,page_to_phys (page)); }else { *dma_handle =phys_to_dma (dev,page_to_phys (page));
@@ -186,10 +186,8 @@ void __dma_direct_free_pages(struct device *dev, size_t size, struct page *page)void dma_direct_free_pages (struct device *dev,size_t size,void *cpu_addr,dma_addr_t dma_addr,unsigned long attrs) { -unsigned int page_order =get_order (size); -if (force_dma_unencrypted ()) -set_memory_encrypted ((unsigned long )cpu_addr,1 << page_order); +set_memory_encrypted ((unsigned long )cpu_addr,1 );__dma_direct_free_pages (dev, size,virt_to_page (cpu_addr)); }
PATCH 2/2 x86/kvmclock: set_memory_decrypted() takes number of pages
From: Ram Pai <[email protected]>
set_memory_decrypted() expects the number of PAGE_SIZE pages to decrypt. kvmclock_init_mem() instead passes number of bytes. This decrypts a huge number of pages resulting in data corruption.
Fixed it.
--- arch/x86/kernel/kvmclock.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Note: Found by code inspection. I don't have a way to test.
arch/x86/kernel/kvmclock.c +1 -2
@@ -251,8 +251,7 @@ static void __init kvmclock_init_mem(void)* be mapped decrypted. */ if (sev_active ()) { - r =set_memory_decrypted ((unsigned long ) hvclock_mem, -1UL << order); + r =set_memory_decrypted ((unsigned long ) hvclock_mem,1 );if (r) {__free_pages (p, order); hvclock_mem =NULL ;
Re: [PATCH 2/2] x86/kvmclock: set_memory_decrypted() takes number of pages
Quoted 5 lines · Thiago Jung Bauermann
From: Ram Pai <[email protected]>
set_memory_decrypted()expects the number ofPAGE_SIZEpages to decrypt.kvmclock_init_mem()instead passes number of bytes. This decrypts a huge number of pages resulting in data corruption.
Same comment as patch 1/2 in this series. This is not correct. See comments below.
Quoted 18 lines
Fixed it.
[ bauermann: Slightly reworded commit message and added Fixes: tag. ]Fixes 6a1cac56f41f ("x86/kvm: Use __bss_decrypted attribute in shared variables")Signed-off-by Ram Pai <[email protected]>Signed-off-by Thiago Jung Bauermann <[email protected]>--- arch/x86/kernel/kvmclock.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)Note: Found by code inspection. I don't have a way to test.
arch/x86/kernel/kvmclock.c +0
-0
arch/x86/kernel/kvmclock.c static void __init kvmclock_init_mem(void) · line 254
Quoted 6 lines
arch/x86/kernel/kvmclock.c +1
-2@@ -251,8 +251,7 @@ static void __init kvmclock_init_mem(void)* be mapped decrypted. */ if (sev_active ()) { - r =set_memory_decrypted ((unsigned long ) hvclock_mem, -1UL << order); + r =set_memory_decrypted ((unsigned long ) hvclock_mem,1 );
Again, not correct. A number of pages were allocated based on the order. That number is calculated using the shift in the call. Hardcoding this to 1 is wrong.
Thanks,
Tom
Quoted 4 lines
arch/x86/kernel/kvmclock.c +0
-0if (r) {__free_pages (p, order); hvclock_mem =NULL ;
Quoted 6 lines · Thiago Jung Bauermann
From: Ram Pai <[email protected]>
set_memory_encrypted()andset_memory_decrypted()expect the number ofPAGE_SIZEpages to encrypt or decrypt.dma_direct_alloc()anddma_direct_free()instead pass number of bytes. This encrypts/decrypts a huge number of pages resulting in data corruption.
That is not what it is doing. See comments below.
Quoted 27 lines
Fixed it.
[ bauermann: Slightly reworded commit message and added Fixes: tag. ]Fixes d7b417fa08d1 ("x86/mm: Add DMA support for SEV memory encryption")Signed-off-by Ram Pai <[email protected]>Signed-off-by Thiago Jung Bauermann <[email protected]>--- kernel/dma/direct.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)Notes:
1. This was tested on powerpc with patches adding support for running under the ultravisor, which are not yet upstream.
2. The lines changed in this patch were added by commit
c10f07aa27da("dma/direct: Handle force decryption for DMA coherent buffers in common code"), but it only moves the code from an x86-specific file. Therefore the Fixes tag references the commit that first introduced the code.kernel/dma/direct.c +0
-0
kernel/dma/direct.c void *dma_direct_alloc_pages(struct device *dev, size_t size, · line 169
Quoted 5 lines
kernel/dma/direct.c +1
-1@@ -166,7 +166,7 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size, ret =page_address (page);if (force_dma_unencrypted ()) { -set_memory_decrypted ((unsigned long )ret,1 <<get_order (size)); +set_memory_decrypted ((unsigned long )ret,1 );
The get_order() function will return the order for the specified size. To then get the number of pages you perform the shift as is being done. The change is definitely wrong since you are now hardcoding the page count to
1. The call to __dma_direct_alloc_pages() will allocate more than one page if the size is greater than a page.
kernel/dma/direct.c void __dma_direct_free_pages(struct device *dev, size_t size, struct page *page) · line 190
Quoted 12 lines
kernel/dma/direct.c +1
-3*dma_handle =__phys_to_dma (dev,page_to_phys (page)); }else { *dma_handle =phys_to_dma (dev,page_to_phys (page));@@ -186,10 +186,8 @@ void __dma_direct_free_pages(struct device *dev, size_t size, struct page *page)void dma_direct_free_pages (struct device *dev,size_t size,void *cpu_addr,dma_addr_t dma_addr,unsigned long attrs) { -unsigned int page_order =get_order (size); -if (force_dma_unencrypted ()) -set_memory_encrypted ((unsigned long )cpu_addr,1 << page_order); +set_memory_encrypted ((unsigned long )cpu_addr,1 );
Same comment here as above.
Thanks,
Tom
Quoted 4 lines
kernel/dma/direct.c +0
-0__dma_direct_free_pages (dev, size,virt_to_page (cpu_addr)); }
Quoted 7 lines · Thomas
Quoted 6 lines · Thiago Jung Bauermann
kernel/dma/direct.c +0
-0@@ -166,7 +166,7 @@ void *dma_direct_alloc_pages(struct device *dev, size_t size,
Quoted 10 lines
kernel/dma/direct.c void *dma_direct_alloc_pages(struct device *dev, size_t size, · line 169
Quoted 4 lines
kernel/dma/direct.c +1
-1ret =page_address (page);if (force_dma_unencrypted ()) { -set_memory_decrypted ((unsigned long )ret,1 <<get_order (size)); +set_memory_decrypted ((unsigned long )ret,1 );The
get_order()function will return the order for the specified size. To then get the number of pages you perform the shift as is being done. The change is definitely wrong since you are now hardcoding the page count to
1. The call to__dma_direct_alloc_pages()will allocate more than one page if the size is greater than a page.
<facepalm>
You are correct, of course. Sorry for the noise and thanks for explaining.
Signature · 2 lines
Thiago Jung Bauermann IBM Linux Technology Center