* [PATCH] x86/HVM: don't leak PFEC_implict to guests
@ 2017-04-06 14:27 Jan Beulich
2017-04-06 14:33 ` Andrew Cooper
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 14:27 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Julien Grall,
Jun Nakajima, Boris Ostrovsky
[-- Attachment #1: Type: text/plain, Size: 3727 bytes --]
Doing so may not only confuse them, but will - on VMX - lead to
VMRESUME failures. Add respective ASSERT()s where the fields get set
to guard against future similar issues (or - in the restore case -
fail the operation). In that latter code at once convert the mis-used
gdprintk() to dprintk(), as the vCPU of interest is not "current".
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3111,7 +3111,7 @@ static enum hvm_copy_result __hvm_copy(
if ( pfinfo )
{
pfinfo->linear = addr;
- pfinfo->ec = pfec;
+ pfinfo->ec = pfec & ~PFEC_implicit;
}
return HVMCOPY_bad_gva_to_gfn;
}
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
- if ( c->pending_valid &&
- ((c->pending_type == 1) || (c->pending_type > 6) ||
- (c->pending_reserved != 0)) )
+ if ( c->pending_valid )
{
- gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
- c->pending_event);
- return -EINVAL;
+ if ( (c->pending_type == 1) || (c->pending_type > 6) ||
+ (c->pending_reserved != 0) )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n",
+ v, c->pending_event);
+ return -EINVAL;
+ }
+
+ if ( c->pending_error_valid &&
+ c->error_code != (uint16_t)c->error_code )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n",
+ v, c->error_code);
+ return -EINVAL;
+ }
}
if ( !paging_mode_hap(v->domain) )
@@ -1288,6 +1298,8 @@ static void svm_inject_event(const struc
vmcb->nextrip = (uint32_t)vmcb->nextrip;
}
+ ASSERT(!eventinj.fields.ev ||
+ eventinj.fields.errorcode == (uint16_t)eventinj.fields.errorcode);
vmcb->eventinj = eventinj;
if ( _event.vector == TRAP_page_fault )
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -731,13 +731,23 @@ static int vmx_vmcs_restore(struct vcpu
{
int rc;
- if ( c->pending_valid &&
- ((c->pending_type == 1) || (c->pending_type > 6) ||
- (c->pending_reserved != 0)) )
+ if ( c->pending_valid )
{
- gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
- c->pending_event);
- return -EINVAL;
+ if ( (c->pending_type == 1) || (c->pending_type > 6) ||
+ (c->pending_reserved != 0) )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n",
+ v, c->pending_event);
+ return -EINVAL;
+ }
+
+ if ( c->pending_error_valid &&
+ c->error_code != (uint16_t)c->error_code )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n",
+ v, c->error_code);
+ return -EINVAL;
+ }
}
rc = vmx_restore_cr0_cr3(v, c->cr0, c->cr3);
@@ -1660,6 +1670,7 @@ static void __vmx_inject_exception(int t
MASK_INSR(trap, INTR_INFO_VECTOR_MASK);
if ( error_code != X86_EVENT_NO_EC )
{
+ ASSERT(error_code == (uint16_t)error_code);
__vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
}
[-- Attachment #2: x86-HVM-filter-PFEC_implicit.patch --]
[-- Type: text/plain, Size: 3767 bytes --]
x86/HVM: don't leak PFEC_implict to guests
Doing so may not only confuse them, but will - on VMX - lead to
VMRESUME failures. Add respective ASSERT()s where the fields get set
to guard against future similar issues (or - in the restore case -
fail the operation). In that latter code at once convert the mis-used
gdprintk() to dprintk(), as the vCPU of interest is not "current".
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -3111,7 +3111,7 @@ static enum hvm_copy_result __hvm_copy(
if ( pfinfo )
{
pfinfo->linear = addr;
- pfinfo->ec = pfec;
+ pfinfo->ec = pfec & ~PFEC_implicit;
}
return HVMCOPY_bad_gva_to_gfn;
}
--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
- if ( c->pending_valid &&
- ((c->pending_type == 1) || (c->pending_type > 6) ||
- (c->pending_reserved != 0)) )
+ if ( c->pending_valid )
{
- gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
- c->pending_event);
- return -EINVAL;
+ if ( (c->pending_type == 1) || (c->pending_type > 6) ||
+ (c->pending_reserved != 0) )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n",
+ v, c->pending_event);
+ return -EINVAL;
+ }
+
+ if ( c->pending_error_valid &&
+ c->error_code != (uint16_t)c->error_code )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n",
+ v, c->error_code);
+ return -EINVAL;
+ }
}
if ( !paging_mode_hap(v->domain) )
@@ -1288,6 +1298,8 @@ static void svm_inject_event(const struc
vmcb->nextrip = (uint32_t)vmcb->nextrip;
}
+ ASSERT(!eventinj.fields.ev ||
+ eventinj.fields.errorcode == (uint16_t)eventinj.fields.errorcode);
vmcb->eventinj = eventinj;
if ( _event.vector == TRAP_page_fault )
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -731,13 +731,23 @@ static int vmx_vmcs_restore(struct vcpu
{
int rc;
- if ( c->pending_valid &&
- ((c->pending_type == 1) || (c->pending_type > 6) ||
- (c->pending_reserved != 0)) )
+ if ( c->pending_valid )
{
- gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
- c->pending_event);
- return -EINVAL;
+ if ( (c->pending_type == 1) || (c->pending_type > 6) ||
+ (c->pending_reserved != 0) )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid pending event %#"PRIx32".\n",
+ v, c->pending_event);
+ return -EINVAL;
+ }
+
+ if ( c->pending_error_valid &&
+ c->error_code != (uint16_t)c->error_code )
+ {
+ dprintk(XENLOG_ERR, "%pv: Invalid error code %#"PRIx32".\n",
+ v, c->error_code);
+ return -EINVAL;
+ }
}
rc = vmx_restore_cr0_cr3(v, c->cr0, c->cr3);
@@ -1660,6 +1670,7 @@ static void __vmx_inject_exception(int t
MASK_INSR(trap, INTR_INFO_VECTOR_MASK);
if ( error_code != X86_EVENT_NO_EC )
{
+ ASSERT(error_code == (uint16_t)error_code);
__vmwrite(VM_ENTRY_EXCEPTION_ERROR_CODE, error_code);
intr_fields |= INTR_INFO_DELIVER_CODE_MASK;
}
[-- Attachment #3: Type: text/plain, Size: 127 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:27 [PATCH] x86/HVM: don't leak PFEC_implict to guests Jan Beulich
@ 2017-04-06 14:33 ` Andrew Cooper
2017-04-06 14:47 ` Jan Beulich
2017-04-06 14:59 ` Boris Ostrovsky
2017-04-07 7:36 ` Tian, Kevin
2 siblings, 1 reply; 8+ messages in thread
From: Andrew Cooper @ 2017-04-06 14:33 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Kevin Tian, Julien Grall, Boris Ostrovsky, Jun Nakajima,
Suravee Suthikulpanit
On 06/04/17 15:27, Jan Beulich wrote:
> Doing so may not only confuse them, but will - on VMX - lead to
> VMRESUME failures. Add respective ASSERT()s where the fields get set
> to guard against future similar issues (or - in the restore case -
> fail the operation). In that latter code at once convert the mis-used
> gdprintk() to dprintk(), as the vCPU of interest is not "current".
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
I'd drop the unnecessary full stops from the printed text.
Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:33 ` Andrew Cooper
@ 2017-04-06 14:47 ` Jan Beulich
0 siblings, 0 replies; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 14:47 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Suravee Suthikulpanit, Julien Grall, Jun Nakajima,
xen-devel, Boris Ostrovsky
>>> On 06.04.17 at 16:33, <andrew.cooper3@citrix.com> wrote:
> On 06/04/17 15:27, Jan Beulich wrote:
>> Doing so may not only confuse them, but will - on VMX - lead to
>> VMRESUME failures. Add respective ASSERT()s where the fields get set
>> to guard against future similar issues (or - in the restore case -
>> fail the operation). In that latter code at once convert the mis-used
>> gdprintk() to dprintk(), as the vCPU of interest is not "current".
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> I'd drop the unnecessary full stops from the printed text.
Oops - I didn't pay attention (and even copied them). Thanks for
spotting.
> Either way, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
Thanks, Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:27 [PATCH] x86/HVM: don't leak PFEC_implict to guests Jan Beulich
2017-04-06 14:33 ` Andrew Cooper
@ 2017-04-06 14:59 ` Boris Ostrovsky
2017-04-06 15:03 ` Andrew Cooper
2017-04-06 15:11 ` Jan Beulich
2017-04-07 7:36 ` Tian, Kevin
2 siblings, 2 replies; 8+ messages in thread
From: Boris Ostrovsky @ 2017-04-06 14:59 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Kevin Tian, Andrew Cooper, Julien Grall, Jun Nakajima,
Suravee Suthikulpanit
> --- a/xen/arch/x86/hvm/svm/svm.c
> +++ b/xen/arch/x86/hvm/svm/svm.c
> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>
> - if ( c->pending_valid &&
> - ((c->pending_type == 1) || (c->pending_type > 6) ||
> - (c->pending_reserved != 0)) )
> + if ( c->pending_valid )
> {
> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
> - c->pending_event);
> - return -EINVAL;
> + if ( (c->pending_type == 1) || (c->pending_type > 6) ||
Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:59 ` Boris Ostrovsky
@ 2017-04-06 15:03 ` Andrew Cooper
2017-04-06 15:11 ` Jan Beulich
1 sibling, 0 replies; 8+ messages in thread
From: Andrew Cooper @ 2017-04-06 15:03 UTC (permalink / raw)
To: Boris Ostrovsky, Jan Beulich, xen-devel
Cc: Kevin Tian, Julien Grall, Jun Nakajima, Suravee Suthikulpanit
On 06/04/17 15:59, Boris Ostrovsky wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
>> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>>
>> - if ( c->pending_valid &&
>> - ((c->pending_type == 1) || (c->pending_type > 6) ||
>> - (c->pending_reserved != 0)) )
>> + if ( c->pending_valid )
>> {
>> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
>> - c->pending_event);
>> - return -EINVAL;
>> + if ( (c->pending_type == 1) || (c->pending_type > 6) ||
> Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4.
That is a good point. SVM has fewer valid types than VT-x. I guess
this was a copy&paste mistake in the past.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:59 ` Boris Ostrovsky
2017-04-06 15:03 ` Andrew Cooper
@ 2017-04-06 15:11 ` Jan Beulich
2017-04-06 15:22 ` Boris Ostrovsky
1 sibling, 1 reply; 8+ messages in thread
From: Jan Beulich @ 2017-04-06 15:11 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Julien Grall,
Jun Nakajima, xen-devel
>>> On 06.04.17 at 16:59, <boris.ostrovsky@oracle.com> wrote:
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
>> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>>
>> - if ( c->pending_valid &&
>> - ((c->pending_type == 1) || (c->pending_type > 6) ||
>> - (c->pending_reserved != 0)) )
>> + if ( c->pending_valid )
>> {
>> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
>> - c->pending_event);
>> - return -EINVAL;
>> + if ( (c->pending_type == 1) || (c->pending_type > 6) ||
>
> Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4.
I don't know, I'm only moving this code. Changes to the values
should be a separate patch.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 15:11 ` Jan Beulich
@ 2017-04-06 15:22 ` Boris Ostrovsky
0 siblings, 0 replies; 8+ messages in thread
From: Boris Ostrovsky @ 2017-04-06 15:22 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Suravee Suthikulpanit, Andrew Cooper, Julien Grall,
Jun Nakajima, xen-devel
On 04/06/2017 11:11 AM, Jan Beulich wrote:
>>>> On 06.04.17 at 16:59, <boris.ostrovsky@oracle.com> wrote:
>>> --- a/xen/arch/x86/hvm/svm/svm.c
>>> +++ b/xen/arch/x86/hvm/svm/svm.c
>>> @@ -269,13 +269,23 @@ static int svm_vmcb_restore(struct vcpu
>>> struct vmcb_struct *vmcb = v->arch.hvm_svm.vmcb;
>>> struct p2m_domain *p2m = p2m_get_hostp2m(v->domain);
>>>
>>> - if ( c->pending_valid &&
>>> - ((c->pending_type == 1) || (c->pending_type > 6) ||
>>> - (c->pending_reserved != 0)) )
>>> + if ( c->pending_valid )
>>> {
>>> - gdprintk(XENLOG_ERR, "Invalid pending event %#"PRIx32".\n",
>>> - c->pending_event);
>>> - return -EINVAL;
>>> + if ( (c->pending_type == 1) || (c->pending_type > 6) ||
>> Shouldn't this be >=5? The only valid types are 0, 2, 3 and 4.
> I don't know, I'm only moving this code. Changes to the values
> should be a separate patch.
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
I can send a patch to adjust type check once this is committed to
staging unless you want to do it yourself.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] x86/HVM: don't leak PFEC_implict to guests
2017-04-06 14:27 [PATCH] x86/HVM: don't leak PFEC_implict to guests Jan Beulich
2017-04-06 14:33 ` Andrew Cooper
2017-04-06 14:59 ` Boris Ostrovsky
@ 2017-04-07 7:36 ` Tian, Kevin
2 siblings, 0 replies; 8+ messages in thread
From: Tian, Kevin @ 2017-04-07 7:36 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Boris Ostrovsky, Andrew Cooper, Julien Grall, Nakajima, Jun,
Suravee Suthikulpanit
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, April 6, 2017 10:28 PM
>
> Doing so may not only confuse them, but will - on VMX - lead to
> VMRESUME failures. Add respective ASSERT()s where the fields get set
> to guard against future similar issues (or - in the restore case -
> fail the operation). In that latter code at once convert the mis-used
> gdprintk() to dprintk(), as the vCPU of interest is not "current".
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
Reviewed-by: Kevin Tian <kevin.tian@intel.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-04-07 7:36 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-06 14:27 [PATCH] x86/HVM: don't leak PFEC_implict to guests Jan Beulich
2017-04-06 14:33 ` Andrew Cooper
2017-04-06 14:47 ` Jan Beulich
2017-04-06 14:59 ` Boris Ostrovsky
2017-04-06 15:03 ` Andrew Cooper
2017-04-06 15:11 ` Jan Beulich
2017-04-06 15:22 ` Boris Ostrovsky
2017-04-07 7:36 ` Tian, Kevin
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.