* [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it
@ 2021-10-29 21:48 Aaron Lewis
2021-10-29 22:48 ` Sean Christopherson
0 siblings, 1 reply; 5+ messages in thread
From: Aaron Lewis @ 2021-10-29 21:48 UTC (permalink / raw)
To: kvm; +Cc: pbonzini, jmattson, seanjc, Aaron Lewis
Rather than assuming which PTE the SMEP test is running on, look it up
to ensure we have the correct entry. If this test were to run on a
different page table (ie: run in an L2 test) the wrong PTE would be set.
Switch to looking up the PTE to avoid this from happening.
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
x86/access.c | 9 ++++++---
x86/cstart64.S | 1 -
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/x86/access.c b/x86/access.c
index 4725bbd..a4d72d9 100644
--- a/x86/access.c
+++ b/x86/access.c
@@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
static unsigned set_cr4_smep(int smep)
{
unsigned long cr4 = shadow_cr4;
- extern u64 ptl2[];
+ pteval_t *pte;
unsigned r;
cr4 &= ~CR4_SMEP_MASK;
@@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
if (cr4 == shadow_cr4)
return 0;
+ pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
+ assert(pte);
+
if (smep)
- ptl2[2] &= ~PT_USER_MASK;
+ *pte &= ~PT_USER_MASK;
r = write_cr4_checking(cr4);
if (r || !smep) {
- ptl2[2] |= PT_USER_MASK;
+ *pte |= PT_USER_MASK;
/* Flush to avoid spurious #PF */
invlpg((void *)(2 << 21));
diff --git a/x86/cstart64.S b/x86/cstart64.S
index 5c6ad38..4ba9943 100644
--- a/x86/cstart64.S
+++ b/x86/cstart64.S
@@ -26,7 +26,6 @@ ring0stacktop:
.data
.align 4096
-.globl ptl2
ptl2:
i = 0
.rept 512 * 4
--
2.33.1.1089.g2158813163f-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it
2021-10-29 21:48 [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it Aaron Lewis
@ 2021-10-29 22:48 ` Sean Christopherson
2021-10-29 22:59 ` Jim Mattson
0 siblings, 1 reply; 5+ messages in thread
From: Sean Christopherson @ 2021-10-29 22:48 UTC (permalink / raw)
To: Aaron Lewis; +Cc: kvm, pbonzini, jmattson
On Fri, Oct 29, 2021, Aaron Lewis wrote:
> Rather than assuming which PTE the SMEP test is running on, look it up
> to ensure we have the correct entry. If this test were to run on a
> different page table (ie: run in an L2 test) the wrong PTE would be set.
> Switch to looking up the PTE to avoid this from happening.
>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
> x86/access.c | 9 ++++++---
> x86/cstart64.S | 1 -
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/x86/access.c b/x86/access.c
> index 4725bbd..a4d72d9 100644
> --- a/x86/access.c
> +++ b/x86/access.c
> @@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
> static unsigned set_cr4_smep(int smep)
> {
> unsigned long cr4 = shadow_cr4;
> - extern u64 ptl2[];
> + pteval_t *pte;
> unsigned r;
>
> cr4 &= ~CR4_SMEP_MASK;
> @@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
> if (cr4 == shadow_cr4)
> return 0;
>
> + pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
What guarantees are there that set_cr4_smep() and the rest of the test are mapped
by the same PTE? I 100% agree the current code is ugly, e.g. I can't remember how
ptl2[2] is guaranteed to be used, but if we're going to fix it then we should aim
for a more robust solution.
> + assert(pte);
> +
> if (smep)
> - ptl2[2] &= ~PT_USER_MASK;
> + *pte &= ~PT_USER_MASK;
> r = write_cr4_checking(cr4);
> if (r || !smep) {
> - ptl2[2] |= PT_USER_MASK;
> + *pte |= PT_USER_MASK;
>
> /* Flush to avoid spurious #PF */
> invlpg((void *)(2 << 21));
This invlpg() should be updated as well.
> diff --git a/x86/cstart64.S b/x86/cstart64.S
> index 5c6ad38..4ba9943 100644
> --- a/x86/cstart64.S
> +++ b/x86/cstart64.S
> @@ -26,7 +26,6 @@ ring0stacktop:
> .data
>
> .align 4096
> -.globl ptl2
> ptl2:
> i = 0
> .rept 512 * 4
> --
> 2.33.1.1089.g2158813163f-goog
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it
2021-10-29 22:48 ` Sean Christopherson
@ 2021-10-29 22:59 ` Jim Mattson
2021-11-01 15:00 ` Aaron Lewis
0 siblings, 1 reply; 5+ messages in thread
From: Jim Mattson @ 2021-10-29 22:59 UTC (permalink / raw)
To: Sean Christopherson; +Cc: Aaron Lewis, kvm, pbonzini
On Fri, Oct 29, 2021 at 3:48 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, Oct 29, 2021, Aaron Lewis wrote:
> > Rather than assuming which PTE the SMEP test is running on, look it up
> > to ensure we have the correct entry. If this test were to run on a
> > different page table (ie: run in an L2 test) the wrong PTE would be set.
> > Switch to looking up the PTE to avoid this from happening.
> >
> > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > ---
> > x86/access.c | 9 ++++++---
> > x86/cstart64.S | 1 -
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/x86/access.c b/x86/access.c
> > index 4725bbd..a4d72d9 100644
> > --- a/x86/access.c
> > +++ b/x86/access.c
> > @@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
> > static unsigned set_cr4_smep(int smep)
> > {
> > unsigned long cr4 = shadow_cr4;
> > - extern u64 ptl2[];
> > + pteval_t *pte;
> > unsigned r;
> >
> > cr4 &= ~CR4_SMEP_MASK;
> > @@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
> > if (cr4 == shadow_cr4)
> > return 0;
> >
> > + pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
>
> What guarantees are there that set_cr4_smep() and the rest of the test are mapped
> by the same PTE? I 100% agree the current code is ugly, e.g. I can't remember how
> ptl2[2] is guaranteed to be used, but if we're going to fix it then we should aim
> for a more robust solution.
One possible solution is to put labels around the code that has to run
with SMEP and then to implement a function to modify the PTEs for a
range of addresses, using gcc's double-ampersand syntax to get the
addresses of the labels.
> > + assert(pte);
> > +
> > if (smep)
> > - ptl2[2] &= ~PT_USER_MASK;
> > + *pte &= ~PT_USER_MASK;
> > r = write_cr4_checking(cr4);
> > if (r || !smep) {
> > - ptl2[2] |= PT_USER_MASK;
> > + *pte |= PT_USER_MASK;
> >
> > /* Flush to avoid spurious #PF */
> > invlpg((void *)(2 << 21));
>
> This invlpg() should be updated as well.
>
> > diff --git a/x86/cstart64.S b/x86/cstart64.S
> > index 5c6ad38..4ba9943 100644
> > --- a/x86/cstart64.S
> > +++ b/x86/cstart64.S
> > @@ -26,7 +26,6 @@ ring0stacktop:
> > .data
> >
> > .align 4096
> > -.globl ptl2
> > ptl2:
> > i = 0
> > .rept 512 * 4
> > --
> > 2.33.1.1089.g2158813163f-goog
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it
2021-10-29 22:59 ` Jim Mattson
@ 2021-11-01 15:00 ` Aaron Lewis
2021-11-01 17:10 ` Jim Mattson
0 siblings, 1 reply; 5+ messages in thread
From: Aaron Lewis @ 2021-11-01 15:00 UTC (permalink / raw)
To: Jim Mattson; +Cc: Sean Christopherson, kvm, pbonzini
On Fri, Oct 29, 2021 at 11:00 PM Jim Mattson <jmattson@google.com> wrote:
>
> On Fri, Oct 29, 2021 at 3:48 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, Oct 29, 2021, Aaron Lewis wrote:
> > > Rather than assuming which PTE the SMEP test is running on, look it up
> > > to ensure we have the correct entry. If this test were to run on a
> > > different page table (ie: run in an L2 test) the wrong PTE would be set.
> > > Switch to looking up the PTE to avoid this from happening.
> > >
> > > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > > ---
> > > x86/access.c | 9 ++++++---
> > > x86/cstart64.S | 1 -
> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/x86/access.c b/x86/access.c
> > > index 4725bbd..a4d72d9 100644
> > > --- a/x86/access.c
> > > +++ b/x86/access.c
> > > @@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
> > > static unsigned set_cr4_smep(int smep)
> > > {
> > > unsigned long cr4 = shadow_cr4;
> > > - extern u64 ptl2[];
> > > + pteval_t *pte;
> > > unsigned r;
> > >
> > > cr4 &= ~CR4_SMEP_MASK;
> > > @@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
> > > if (cr4 == shadow_cr4)
> > > return 0;
> > >
> > > + pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
> >
> > What guarantees are there that set_cr4_smep() and the rest of the test are mapped
> > by the same PTE? I 100% agree the current code is ugly, e.g. I can't remember how
> > ptl2[2] is guaranteed to be used, but if we're going to fix it then we should aim
> > for a more robust solution.
>
> One possible solution is to put labels around the code that has to run
> with SMEP and then to implement a function to modify the PTEs for a
> range of addresses, using gcc's double-ampersand syntax to get the
> addresses of the labels.
>
Would it be safer to just mark the start and end of the text section,
then I could remove the PT_USER_MASK flags from that whole range?
> > > + assert(pte);
> > > +
> > > if (smep)
> > > - ptl2[2] &= ~PT_USER_MASK;
> > > + *pte &= ~PT_USER_MASK;
> > > r = write_cr4_checking(cr4);
> > > if (r || !smep) {
> > > - ptl2[2] |= PT_USER_MASK;
> > > + *pte |= PT_USER_MASK;
> > >
> > > /* Flush to avoid spurious #PF */
> > > invlpg((void *)(2 << 21));
> >
> > This invlpg() should be updated as well.
Good catch. I'll update this.
> >
> > > diff --git a/x86/cstart64.S b/x86/cstart64.S
> > > index 5c6ad38..4ba9943 100644
> > > --- a/x86/cstart64.S
> > > +++ b/x86/cstart64.S
> > > @@ -26,7 +26,6 @@ ring0stacktop:
> > > .data
> > >
> > > .align 4096
> > > -.globl ptl2
> > > ptl2:
> > > i = 0
> > > .rept 512 * 4
> > > --
> > > 2.33.1.1089.g2158813163f-goog
> > >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it
2021-11-01 15:00 ` Aaron Lewis
@ 2021-11-01 17:10 ` Jim Mattson
0 siblings, 0 replies; 5+ messages in thread
From: Jim Mattson @ 2021-11-01 17:10 UTC (permalink / raw)
To: Aaron Lewis; +Cc: Sean Christopherson, kvm, pbonzini
On Mon, Nov 1, 2021 at 8:00 AM Aaron Lewis <aaronlewis@google.com> wrote:
>
> On Fri, Oct 29, 2021 at 11:00 PM Jim Mattson <jmattson@google.com> wrote:
> >
> > On Fri, Oct 29, 2021 at 3:48 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Fri, Oct 29, 2021, Aaron Lewis wrote:
> > > > Rather than assuming which PTE the SMEP test is running on, look it up
> > > > to ensure we have the correct entry. If this test were to run on a
> > > > different page table (ie: run in an L2 test) the wrong PTE would be set.
> > > > Switch to looking up the PTE to avoid this from happening.
> > > >
> > > > Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> > > > ---
> > > > x86/access.c | 9 ++++++---
> > > > x86/cstart64.S | 1 -
> > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/x86/access.c b/x86/access.c
> > > > index 4725bbd..a4d72d9 100644
> > > > --- a/x86/access.c
> > > > +++ b/x86/access.c
> > > > @@ -204,7 +204,7 @@ static void set_cr0_wp(int wp)
> > > > static unsigned set_cr4_smep(int smep)
> > > > {
> > > > unsigned long cr4 = shadow_cr4;
> > > > - extern u64 ptl2[];
> > > > + pteval_t *pte;
> > > > unsigned r;
> > > >
> > > > cr4 &= ~CR4_SMEP_MASK;
> > > > @@ -213,11 +213,14 @@ static unsigned set_cr4_smep(int smep)
> > > > if (cr4 == shadow_cr4)
> > > > return 0;
> > > >
> > > > + pte = get_pte(phys_to_virt(read_cr3()), set_cr4_smep);
> > >
> > > What guarantees are there that set_cr4_smep() and the rest of the test are mapped
> > > by the same PTE? I 100% agree the current code is ugly, e.g. I can't remember how
> > > ptl2[2] is guaranteed to be used, but if we're going to fix it then we should aim
> > > for a more robust solution.
> >
> > One possible solution is to put labels around the code that has to run
> > with SMEP and then to implement a function to modify the PTEs for a
> > range of addresses, using gcc's double-ampersand syntax to get the
> > addresses of the labels.
> >
>
> Would it be safer to just mark the start and end of the text section,
> then I could remove the PT_USER_MASK flags from that whole range?
That sounds ideal. There's already an stext, so you would just have to
add etext.
> > > > + assert(pte);
> > > > +
> > > > if (smep)
> > > > - ptl2[2] &= ~PT_USER_MASK;
> > > > + *pte &= ~PT_USER_MASK;
> > > > r = write_cr4_checking(cr4);
> > > > if (r || !smep) {
> > > > - ptl2[2] |= PT_USER_MASK;
> > > > + *pte |= PT_USER_MASK;
> > > >
> > > > /* Flush to avoid spurious #PF */
> > > > invlpg((void *)(2 << 21));
> > >
> > > This invlpg() should be updated as well.
>
> Good catch. I'll update this.
>
> > >
> > > > diff --git a/x86/cstart64.S b/x86/cstart64.S
> > > > index 5c6ad38..4ba9943 100644
> > > > --- a/x86/cstart64.S
> > > > +++ b/x86/cstart64.S
> > > > @@ -26,7 +26,6 @@ ring0stacktop:
> > > > .data
> > > >
> > > > .align 4096
> > > > -.globl ptl2
> > > > ptl2:
> > > > i = 0
> > > > .rept 512 * 4
> > > > --
> > > > 2.33.1.1089.g2158813163f-goog
> > > >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-11-01 17:10 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-29 21:48 [kvm-unit-tests PATCH] x86: Look up the PTE rather than assuming it Aaron Lewis
2021-10-29 22:48 ` Sean Christopherson
2021-10-29 22:59 ` Jim Mattson
2021-11-01 15:00 ` Aaron Lewis
2021-11-01 17:10 ` Jim Mattson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).