Re: [PATCH] x86/vPIT: check/bound values loaded from state save record

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxx>
>> ---
>> 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