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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

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