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

[Jason Andryuk <jandryuk@xxxxxxxxx>]

On Thu, May 11, 2023 at 7:50 AM Jan Beulich <jbeulich@xxxxxxxx> 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.
>
> --- 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