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. */
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?
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
linux-stable-mirror@lists.linaro.org
-
Dave Hansen -
Paluri, PavanKumar -
Pavan Kumar Paluri