nestedsvm: Don't set VMCB(1-2)'s NP_ENABLE and N_CR3 during VMEXIT to L1

[PATCH] nestedsvm: Don't set VMCB(1-2)'s NP_ENABLE and N_CR3 during VMEXIT to L1

Posted by Ross Lagerwall 6 days, 5 hours ago

As per the VMRUN pseudocode in APM Vol 3 3.38, the VMCB's NP_ENABLE and
N_CR3 fields are not set during a VMEXIT so don't do this when updating
VMCB(1-2). At the same time, cleanup the somewhat bogus and irrelevant
comments. Not clearing N_CR3 does not introduce a security hole as
stated since L1 can set it regardless and it is never used directly when
running L2.

Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
---
 xen/arch/x86/hvm/svm/nestedsvm.c | 32 +-------------------------------
 1 file changed, 1 insertion(+), 31 deletions(-)

diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
index a8b15d6eae05..c62fca571d75 100644
--- a/xen/arch/x86/hvm/svm/nestedsvm.c
+++ b/xen/arch/x86/hvm/svm/nestedsvm.c
@@ -1023,37 +1023,6 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
 
     ns_vmcb->event_inj.raw = 0;
 
-    /* Nested paging mode */
-    if ( nestedhvm_paging_mode_hap(v) )
-    {
-        /* host nested paging + guest nested paging. */
-        vmcb_set_np(ns_vmcb, vmcb_get_np(n2vmcb));
-        ns_vmcb->_cr3 = n2vmcb->_cr3;
-        /* The vmcb->h_cr3 is the shadowed h_cr3. The original
-         * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
-         * hence we keep the ns_vmcb->h_cr3 value. */
-    }
-    else if ( paging_mode_hap(v->domain) )
-    {
-        /* host nested paging + guest shadow paging. */
-        vmcb_set_np(ns_vmcb, false);
-        /* Throw h_cr3 away. Guest is not allowed to set it or
-         * it can break out, otherwise (security hole!) */
-        ns_vmcb->_h_cr3 = 0x0;
-        /* Stop intercepting #PF (already done above
-         * by restoring cached intercepts). */
-        ns_vmcb->_cr3 = n2vmcb->_cr3;
-    }
-    else
-    {
-        /* host shadow paging + guest shadow paging. */
-        vmcb_set_np(ns_vmcb, false);
-        ns_vmcb->_h_cr3 = 0x0;
-        /* The vmcb->_cr3 is the shadowed cr3. The original
-         * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
-         * hence we keep the ns_vmcb->_cr3 value. */
-    }
-
     /* LBR virtualization - keep lbr control as is */
 
     /* NextRIP */
@@ -1083,6 +1052,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
 
     /* CRn */
     ns_vmcb->_cr4 = n2vmcb->_cr4;
+    ns_vmcb->_cr3 = n2vmcb->_cr3;
     ns_vmcb->_cr0 = n2vmcb->_cr0;
 
     /* DRn */
-- 
2.55.0

Re: [PATCH] nestedsvm: Don't set VMCB(1-2)'s NP_ENABLE and N_CR3 during VMEXIT to L1

Posted by Teddy Astie 3 days, 23 hours ago

Le 18/09/2026 à 14:54, Ross Lagerwall a écrit :
> As per the VMRUN pseudocode in APM Vol 3 3.38, the VMCB's NP_ENABLE and
> N_CR3 fields are not set during a VMEXIT so don't do this when updating
> VMCB(1-2). At the same time, cleanup the somewhat bogus and irrelevant
> comments. Not clearing N_CR3 does not introduce a security hole as
> stated since L1 can set it regardless and it is never used directly when
> running L2.
> 
> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
> ---
>   xen/arch/x86/hvm/svm/nestedsvm.c | 32 +-------------------------------
>   1 file changed, 1 insertion(+), 31 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
> index a8b15d6eae05..c62fca571d75 100644
> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
> @@ -1023,37 +1023,6 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>   
>       ns_vmcb->event_inj.raw = 0;
>   
> -    /* Nested paging mode */
> -    if ( nestedhvm_paging_mode_hap(v) )
> -    {
> -        /* host nested paging + guest nested paging. */
> -        vmcb_set_np(ns_vmcb, vmcb_get_np(n2vmcb));
> -        ns_vmcb->_cr3 = n2vmcb->_cr3;
> -        /* The vmcb->h_cr3 is the shadowed h_cr3. The original
> -         * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
> -         * hence we keep the ns_vmcb->h_cr3 value. */
> -    }
> -    else if ( paging_mode_hap(v->domain) )
> -    {
> -        /* host nested paging + guest shadow paging. */
> -        vmcb_set_np(ns_vmcb, false);
> -        /* Throw h_cr3 away. Guest is not allowed to set it or
> -         * it can break out, otherwise (security hole!) */
> -        ns_vmcb->_h_cr3 = 0x0;
> -        /* Stop intercepting #PF (already done above
> -         * by restoring cached intercepts). */
> -        ns_vmcb->_cr3 = n2vmcb->_cr3;
> -    }
> -    else
> -    {
> -        /* host shadow paging + guest shadow paging. */
> -        vmcb_set_np(ns_vmcb, false);
> -        ns_vmcb->_h_cr3 = 0x0;
> -        /* The vmcb->_cr3 is the shadowed cr3. The original
> -         * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
> -         * hence we keep the ns_vmcb->_cr3 value. */
> -    }
> ->       /* LBR virtualization - keep lbr control as is */
>   
>       /* NextRIP */
> @@ -1083,6 +1052,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>   
>       /* CRn */
>       ns_vmcb->_cr4 = n2vmcb->_cr4;
> +    ns_vmcb->_cr3 = n2vmcb->_cr3;
>       ns_vmcb->_cr0 = n2vmcb->_cr0;
>   

I would suggest to group all the CRn (the CR2 part is currently 
separated). That can be done separately.

>       /* DRn */

Reviewed-by: Teddy Astie <teddy.astie@vates.tech>

Teddy

Re: [PATCH] nestedsvm: Don't set VMCB(1-2)'s NP_ENABLE and N_CR3 during VMEXIT to L1

Posted by Ross Lagerwall 3 days, 10 hours ago

On 9/20/26 8:10 PM, Teddy Astie wrote:
> Le 18/09/2026 à 14:54, Ross Lagerwall a écrit :
>> As per the VMRUN pseudocode in APM Vol 3 3.38, the VMCB's NP_ENABLE and
>> N_CR3 fields are not set during a VMEXIT so don't do this when updating
>> VMCB(1-2). At the same time, cleanup the somewhat bogus and irrelevant
>> comments. Not clearing N_CR3 does not introduce a security hole as
>> stated since L1 can set it regardless and it is never used directly when
>> running L2.
>>
>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com>
>> ---
>>   xen/arch/x86/hvm/svm/nestedsvm.c | 32 +-------------------------------
>>   1 file changed, 1 insertion(+), 31 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/svm/nestedsvm.c b/xen/arch/x86/hvm/svm/nestedsvm.c
>> index a8b15d6eae05..c62fca571d75 100644
>> --- a/xen/arch/x86/hvm/svm/nestedsvm.c
>> +++ b/xen/arch/x86/hvm/svm/nestedsvm.c
>> @@ -1023,37 +1023,6 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       ns_vmcb->event_inj.raw = 0;
>> -    /* Nested paging mode */
>> -    if ( nestedhvm_paging_mode_hap(v) )
>> -    {
>> -        /* host nested paging + guest nested paging. */
>> -        vmcb_set_np(ns_vmcb, vmcb_get_np(n2vmcb));
>> -        ns_vmcb->_cr3 = n2vmcb->_cr3;
>> -        /* The vmcb->h_cr3 is the shadowed h_cr3. The original
>> -         * unshadowed guest h_cr3 is kept in ns_vmcb->h_cr3,
>> -         * hence we keep the ns_vmcb->h_cr3 value. */
>> -    }
>> -    else if ( paging_mode_hap(v->domain) )
>> -    {
>> -        /* host nested paging + guest shadow paging. */
>> -        vmcb_set_np(ns_vmcb, false);
>> -        /* Throw h_cr3 away. Guest is not allowed to set it or
>> -         * it can break out, otherwise (security hole!) */
>> -        ns_vmcb->_h_cr3 = 0x0;
>> -        /* Stop intercepting #PF (already done above
>> -         * by restoring cached intercepts). */
>> -        ns_vmcb->_cr3 = n2vmcb->_cr3;
>> -    }
>> -    else
>> -    {
>> -        /* host shadow paging + guest shadow paging. */
>> -        vmcb_set_np(ns_vmcb, false);
>> -        ns_vmcb->_h_cr3 = 0x0;
>> -        /* The vmcb->_cr3 is the shadowed cr3. The original
>> -         * unshadowed guest cr3 is kept in ns_vmcb->_cr3,
>> -         * hence we keep the ns_vmcb->_cr3 value. */
>> -    }
>> ->       /* LBR virtualization - keep lbr control as is */
>>       /* NextRIP */
>> @@ -1083,6 +1052,7 @@ nsvm_vmcb_prepare4vmexit(struct vcpu *v, struct cpu_user_regs *regs)
>>       /* CRn */
>>       ns_vmcb->_cr4 = n2vmcb->_cr4;
>> +    ns_vmcb->_cr3 = n2vmcb->_cr3;
>>       ns_vmcb->_cr0 = n2vmcb->_cr0;
> 
> I would suggest to group all the CRn (the CR2 part is currently separated). That can be done separately.
> 

Indeed. The APM says...

"Upon #VMEXIT, the processor performs the following actions in order to return
to the host execution context:"

... so ideally we would rearrange this function to match the order specified
(though it shouldn't make a functional difference).

Ross