[PATCH] x86/sev: Adjust the alignment of vaddr_end

[PATCH] x86/sev: Adjust the alignment of vaddr_end

[Pavan Kumar Paluri]

As vaddr is aligned down, vaddr_end accordingly needs to be aligned.
Otherwise, vaddr_end would be one page short. While at it, compute vaddr
and vaddr_end using kernel defined headers.

Fixes: dc3f3d2474b8 ("x86/mm: Validate memory when changing the C-bit")
Fixes: 5e5ccff60a29 ("x86/sev: Add helper for validating pages in early enc attribute changes")
Suggested-by: Nikunj Dadhania <nikunj.dadhania@amd.com>
Cc: stable@vger.kernel.org
Signed-off-by: Pavan Kumar Paluri <papaluri@amd.com>
---
 arch/x86/kernel/sev.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 679026a640ef..0f261cb306c3 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -648,8 +648,8 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
 	unsigned long vaddr_end;
 	int rc;
 
-	vaddr = vaddr & PAGE_MASK;
-	vaddr_end = vaddr + (npages << PAGE_SHIFT);
+	vaddr_end = PAGE_ALIGN(vaddr + (npages << PAGE_SHIFT));
+	vaddr = PAGE_ALIGN_DOWN(vaddr);
 
 	while (vaddr < vaddr_end) {
 		rc = pvalidate(vaddr, RMP_PG_SIZE_4K, validate);
@@ -886,8 +886,8 @@ static void set_pages_state(unsigned long vaddr, unsigned int npages, int op)
 	if (!desc)
 		panic("SNP: failed to allocate memory for PSC descriptor\n");
 
-	vaddr = vaddr & PAGE_MASK;
-	vaddr_end = vaddr + (npages << PAGE_SHIFT);
+	vaddr_end = PAGE_ALIGN(vaddr + (npages << PAGE_SHIFT));
+	vaddr = PAGE_ALIGN_DOWN(vaddr);
 
 	while (vaddr < vaddr_end) {
 		/* Calculate the last vaddr that fits in one struct snp_psc_desc. */
-- 
2.25.1

Re: [PATCH] x86/sev: Adjust the alignment of vaddr_end

[Dave Hansen]

On 2/10/23 08:58, Pavan Kumar Paluri wrote:
> --- a/arch/x86/kernel/sev.c
> +++ b/arch/x86/kernel/sev.c
> @@ -648,8 +648,8 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
>  	unsigned long vaddr_end;
>  	int rc;
>  
> -	vaddr = vaddr & PAGE_MASK;
> -	vaddr_end = vaddr + (npages << PAGE_SHIFT);
> +	vaddr_end = PAGE_ALIGN(vaddr + (npages << PAGE_SHIFT));
> +	vaddr = PAGE_ALIGN_DOWN(vaddr);

Could you folks please go take a look at all of the callers that call
into pvalidate_pages() and set_pages_state()?

I think part of the problem here is that the API is quite inconsistent.
There have been a few bugs in this area.  Ideally, if you have an
interface that deals in 'pages', then the addresses should be aligned
already before hitting that interface.

But, there are messes like this:

static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
                                     unsigned long paddr, bool decrypt)
{
        unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
	...
	early_snp_set_memory_shared((unsigned long)__va(paddr), paddr,
				    npages);

That's taking a theoretically unaligned 'paddr', page-aligning the size,
then passing the result into an 'npages' interface.  That makes zero
sense if, for instance, it tried to do:

	paddr = 0x0fff
	sz = 2

That would pass along:

	paddr = 0x0fff
	npages = 1

npages (the effective end address) is aligned, but paddr is not.

We can keep on hacking around these bugs as they present themselves
one-by-one.  Could you look a bit more widely, please, and see if
there's something more fundamental that should be done?

Re: [PATCH] x86/sev: Adjust the alignment of vaddr_end

[Paluri, PavanKumar]

Hi Dave,

On 2/10/2023 11:38 AM, Dave Hansen wrote:
> On 2/10/23 08:58, Pavan Kumar Paluri wrote:
>> --- a/arch/x86/kernel/sev.c
>> +++ b/arch/x86/kernel/sev.c
>> @@ -648,8 +648,8 @@ static void pvalidate_pages(unsigned long vaddr, unsigned int npages, bool valid
>>  	unsigned long vaddr_end;
>>  	int rc;
>>  
>> -	vaddr = vaddr & PAGE_MASK;
>> -	vaddr_end = vaddr + (npages << PAGE_SHIFT);
>> +	vaddr_end = PAGE_ALIGN(vaddr + (npages << PAGE_SHIFT));
>> +	vaddr = PAGE_ALIGN_DOWN(vaddr);
> 
> Could you folks please go take a look at all of the callers that call
> into pvalidate_pages() and set_pages_state()?
> 
> I think part of the problem here is that the API is quite inconsistent.
> There have been a few bugs in this area.  Ideally, if you have an
> interface that deals in 'pages', then the addresses should be aligned
> already before hitting that interface.

I agree
> 
> But, there are messes like this:
> 
> static inline void __init snp_memcpy(void *dst, void *src, size_t sz,
>                                      unsigned long paddr, bool decrypt)
> {
>         unsigned long npages = PAGE_ALIGN(sz) >> PAGE_SHIFT;
> 	...
> 	early_snp_set_memory_shared((unsigned long)__va(paddr), paddr,
> 				    npages);
> 
> That's taking a theoretically unaligned 'paddr', page-aligning the size,
> then passing the result into an 'npages' interface.  That makes zero
> sense if, for instance, it tried to do:
> 
> 	paddr = 0x0fff
> 	sz = 2
> 
> That would pass along:
> 
> 	paddr = 0x0fff
> 	npages = 1
> 
> npages (the effective end address) is aligned, but paddr is not.
> 
> We can keep on hacking around these bugs as they present themselves
> one-by-one.  Could you look a bit more widely, please, and see if
> there's something more fundamental that should be done?

We will try to fix paddr and vaddr alignments before they begin to touch
the interfaces.

Thanks,
Pavan