In particular pit_latch_status() and speaker_ioport_read() perform
calculations which assume in-bounds values. Several of the state save
record fields can hold wider ranges, though.

Note that ->gate should only be possible to be zero for channel 2;
enforce that as well.

Adjust pit_reset()'s writing of ->mode as well, to not unduly affect
the value pit_latch_status() may calculate. The chosen mode of 7 is
still one which cannot be established by writing the control word.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
Of course an alternative would be to simply reject state save records
with out of bounds values.

--- a/xen/arch/x86/emul-i8254.c
+++ b/xen/arch/x86/emul-i8254.c
@@ -47,6 +47,7 @@
 #define RW_STATE_MSB 2
 #define RW_STATE_WORD0 3
 #define RW_STATE_WORD1 4
+#define RW_STATE_NUM 5
 
 static int cf_check handle_pit_io(
     int dir, unsigned int port, unsigned int bytes, uint32_t *val);
@@ -426,6 +427,33 @@ static int cf_check pit_load(struct doma
     }
     
     /*
+     * Convert loaded values to be within valid range, for them to represent
+     * actually reachable state.  Uses of some of the values elsewhere assume
+     * this is the case.
+     */
+    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
+    {
+        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
+
+        /* pit_load_count() will convert 0 suitably back to 0x10000. */
+        ch->count &= 0xffff;
+        if ( ch->count_latched >= RW_STATE_NUM )
+            ch->count_latched = 0;
+        if ( ch->read_state >= RW_STATE_NUM )
+            ch->read_state = 0;
+        if ( ch->read_state >= RW_STATE_NUM )
+            ch->write_state = 0;
+        if ( ch->rw_mode > RW_STATE_WORD0 )
+            ch->rw_mode = 0;
+        if ( (ch->mode &= 7) > 5 )
+            ch->mode -= 4;
+        ch->bcd = !!ch->bcd;
+        ch->gate = i != 2 || ch->gate;
+    }
+
+    pit->hw.speaker_data_on = !!pit->hw.speaker_data_on;
+
+    /*
      * Recreate platform timers from hardware state.  There will be some 
      * time jitter here, but the wall-clock will have jumped massively, so 
      * we hope the guest can handle it.
@@ -464,7 +492,7 @@ void pit_reset(struct domain *d)
     for ( i = 0; i < 3; i++ )
     {
         s = &pit->hw.channels[i];
-        s->mode = 0xff; /* the init mode */
+        s->mode = 7; /* the init mode */
         s->gate = (i != 2);
         pit_load_count(pit, i, 0);
     }

On Thu, May 11, 2023 at 01:50:05PM +0200, Jan Beulich wrote:
> In particular pit_latch_status() and speaker_ioport_read() perform
> calculations which assume in-bounds values. Several of the state save
> record fields can hold wider ranges, though.
> 
> Note that ->gate should only be possible to be zero for channel 2;
> enforce that as well.
> 
> Adjust pit_reset()'s writing of ->mode as well, to not unduly affect
> the value pit_latch_status() may calculate. The chosen mode of 7 is
> still one which cannot be established by writing the control word.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course an alternative would be to simply reject state save records
> with out of bounds values.

I've been taking a look at how we load other records, and
lapic_load_hidden() for example will return error when invalid values
are found.

IMO that seems safer, I think there's a risk in the adjustments
done below to also lead to not safe combinations of fields.

So we either reject the state and return an error, or we silently
reject and leave the PIT in the reset state.

Unless there's a reason we need to handle such bogus state.

> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -47,6 +47,7 @@
>  #define RW_STATE_MSB 2
>  #define RW_STATE_WORD0 3
>  #define RW_STATE_WORD1 4
> +#define RW_STATE_NUM 5
>  
>  static int cf_check handle_pit_io(
>      int dir, unsigned int port, unsigned int bytes, uint32_t *val);
> @@ -426,6 +427,33 @@ static int cf_check pit_load(struct doma
>      }
>      
>      /*
> +     * Convert loaded values to be within valid range, for them to represent
> +     * actually reachable state.  Uses of some of the values elsewhere assume
> +     * this is the case.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
> +    {
> +        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
> +
> +        /* pit_load_count() will convert 0 suitably back to 0x10000. */
> +        ch->count &= 0xffff;

Might be helpful to have this in a define, as it's also used by
pit_get_count().

Thanks, Roger.

On 25.10.2023 11:11, Roger Pau Monné wrote:
> On Thu, May 11, 2023 at 01:50:05PM +0200, Jan Beulich wrote:
>> In particular pit_latch_status() and speaker_ioport_read() perform
>> calculations which assume in-bounds values. Several of the state save
>> record fields can hold wider ranges, though.
>>
>> Note that ->gate should only be possible to be zero for channel 2;
>> enforce that as well.
>>
>> Adjust pit_reset()'s writing of ->mode as well, to not unduly affect
>> the value pit_latch_status() may calculate. The chosen mode of 7 is
>> still one which cannot be established by writing the control word.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> Of course an alternative would be to simply reject state save records
>> with out of bounds values.
> 
> I've been taking a look at how we load other records, and
> lapic_load_hidden() for example will return error when invalid values
> are found.
> 
> IMO that seems safer, I think there's a risk in the adjustments
> done below to also lead to not safe combinations of fields.
> 
> So we either reject the state and return an error, or we silently
> reject and leave the PIT in the reset state.
> 
> Unless there's a reason we need to handle such bogus state.

Well, I've tried to be conservative (similarly in the vPIC equivalent
change) as to affecting guests with potentially bogus incoming
migration streams. Such guests may have been migrated multiple times,
from pretty old Xen, and I don't consider it reasonable to check each
and every Xen version as to possibly permitting out-of-range values
to be reached and then saved when migrating the guest away. Imo if we
were to reject bad input, we'd need to have a way to override that.
Requiring people to "fix" the data in the migration stream seems to
me like demanding too much in such a situation.

What we could also consider is have the tool stack do the adjustment
(perhaps optionally, e.g. driven by a command line option), while
unconditionally refusing out of range input in the hypervisor. But
of course that's more involved, overall (and could still end in
perceived regressions, even if those are then easier to overcome).

>> @@ -426,6 +427,33 @@ static int cf_check pit_load(struct doma
>>      }
>>      
>>      /*
>> +     * Convert loaded values to be within valid range, for them to represent
>> +     * actually reachable state.  Uses of some of the values elsewhere assume
>> +     * this is the case.
>> +     */
>> +    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
>> +    {
>> +        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
>> +
>> +        /* pit_load_count() will convert 0 suitably back to 0x10000. */
>> +        ch->count &= 0xffff;
> 
> Might be helpful to have this in a define, as it's also used by
> pit_get_count().

Hmm, the two uses of 0xffff aren't exactly for the same purpose.
Here we're bounding the input range, whereas there we simply need to
deal with overflow that unavoidably can happen when the counter has
wrapped at least once. Really the comment is here to avoid putting
in place the more precise but also more involved

        if ( ch->count > 0x10000 )
            ch->count &= 0xffff;

If we used this, divergence from pit_get_count() would be yet larger.

Jan

On Thu, May 11, 2023 at 7:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> In particular pit_latch_status() and speaker_ioport_read() perform
> calculations which assume in-bounds values. Several of the state save
> record fields can hold wider ranges, though.
>
> Note that ->gate should only be possible to be zero for channel 2;
> enforce that as well.
>
> Adjust pit_reset()'s writing of ->mode as well, to not unduly affect
> the value pit_latch_status() may calculate. The chosen mode of 7 is
> still one which cannot be established by writing the control word.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> Of course an alternative would be to simply reject state save records
> with out of bounds values.
>
> --- a/xen/arch/x86/emul-i8254.c
> +++ b/xen/arch/x86/emul-i8254.c
> @@ -47,6 +47,7 @@
>  #define RW_STATE_MSB 2
>  #define RW_STATE_WORD0 3
>  #define RW_STATE_WORD1 4
> +#define RW_STATE_NUM 5
>
>  static int cf_check handle_pit_io(
>      int dir, unsigned int port, unsigned int bytes, uint32_t *val);
> @@ -426,6 +427,33 @@ static int cf_check pit_load(struct doma
>      }
>
>      /*
> +     * Convert loaded values to be within valid range, for them to represent
> +     * actually reachable state.  Uses of some of the values elsewhere assume
> +     * this is the case.
> +     */
> +    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
> +    {
> +        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
> +
> +        /* pit_load_count() will convert 0 suitably back to 0x10000. */
> +        ch->count &= 0xffff;
> +        if ( ch->count_latched >= RW_STATE_NUM )
> +            ch->count_latched = 0;
> +        if ( ch->read_state >= RW_STATE_NUM )
> +            ch->read_state = 0;
> +        if ( ch->read_state >= RW_STATE_NUM )
> +            ch->write_state = 0;

Should these both be write_state?

Regards,
Jason

On 11.05.2023 19:51, Jason Andryuk wrote:
> On Thu, May 11, 2023 at 7:50 AM Jan Beulich <jbeulich@suse.com> wrote:
>> @@ -426,6 +427,33 @@ static int cf_check pit_load(struct doma
>>      }
>>
>>      /*
>> +     * Convert loaded values to be within valid range, for them to represent
>> +     * actually reachable state.  Uses of some of the values elsewhere assume
>> +     * this is the case.
>> +     */
>> +    for ( i = 0; i < ARRAY_SIZE(pit->hw.channels); ++i )
>> +    {
>> +        struct hvm_hw_pit_channel *ch = &pit->hw.channels[i];
>> +
>> +        /* pit_load_count() will convert 0 suitably back to 0x10000. */
>> +        ch->count &= 0xffff;
>> +        if ( ch->count_latched >= RW_STATE_NUM )
>> +            ch->count_latched = 0;
>> +        if ( ch->read_state >= RW_STATE_NUM )
>> +            ch->read_state = 0;
>> +        if ( ch->read_state >= RW_STATE_NUM )
>> +            ch->write_state = 0;
> 
> Should these both be write_state?

Definitely. Thanks for noticing!

Jan