x86/HVM: fix PM timer I/O port range version check
diff mbox series

Message ID 4c53200e-f571-a3de-cb25-6548a40bbb94@suse.com
State New
Headers show
Series
  • x86/HVM: fix PM timer I/O port range version check
Related show

Commit Message

Jan Beulich Jan. 28, 2020, 1:17 p.m. UTC
XOR-ing two arbitrary non-equal values may produce 1 even if both values
are different from both 0 and 1 (2 and 3 would fit, for example). Use OR
instead, which together with the earlier bailing upon finding
"version == old_version" achieves the intended effect.

Fixes: f0ad21c499f7 ("x86 hvm: Introduce pmtimer_change_ioport and HVM_PARAM_ACPI_IOPORTS_LOCATION")
Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Roger Pau Monné Jan. 28, 2020, 1:56 p.m. UTC | #1
On Tue, Jan 28, 2020 at 02:17:51PM +0100, Jan Beulich wrote:
> XOR-ing two arbitrary non-equal values may produce 1 even if both values
> are different from both 0 and 1 (2 and 3 would fit, for example). Use OR
> instead, which together with the earlier bailing upon finding
> "version == old_version" achieves the intended effect.
> 
> Fixes: f0ad21c499f7 ("x86 hvm: Introduce pmtimer_change_ioport and HVM_PARAM_ACPI_IOPORTS_LOCATION")
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, Roger.
Jan Beulich Jan. 30, 2020, 7:58 a.m. UTC | #2
On 28.01.2020 14:56, Roger Pau Monné wrote:
> On Tue, Jan 28, 2020 at 02:17:51PM +0100, Jan Beulich wrote:
>> XOR-ing two arbitrary non-equal values may produce 1 even if both values
>> are different from both 0 and 1 (2 and 3 would fit, for example). Use OR
>> instead, which together with the earlier bailing upon finding
>> "version == old_version" achieves the intended effect.
>>
>> Fixes: f0ad21c499f7 ("x86 hvm: Introduce pmtimer_change_ioport and HVM_PARAM_ACPI_IOPORTS_LOCATION")
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Thanks, but I withdraw this patch. The code is correct as is, just
a little less obviously correct than with the use of |. As long as
there's no way for the value stored in Xen to be larger than 1,
there's also no way for a larger value to pass the check in
question.

Instead I notice the upper 32 bits of the value are silently
ignored. I'll submit a new patch addressing that instead.

Jan

Patch
diff mbox series

--- a/xen/arch/x86/hvm/pmtimer.c
+++ b/xen/arch/x86/hvm/pmtimer.c
@@ -326,7 +326,7 @@  int pmtimer_change_ioport(struct domain
         return 0;
 
     /* Only allow changes between versions 0 and 1. */
-    if ( (version ^ old_version) != 1 )
+    if ( (version | old_version) != 1 )
         return -EINVAL;
 
     if ( version == 1 )