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

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 <[e-mail]>

Comments

Roger Pau Monne 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 <[e-mail]>

Reviewed-by: Roger Pau Monné <[e-mail]>

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 <[e-mail]>
> 
> Reviewed-by: Roger Pau Monné <[e-mail]>

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

--- 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 )