Re: [PATCH v3 4/6] x86/vPIT: check values loaded from state save record

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

On Tue, Nov 28, 2023 at 11:35:18AM +0100, 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. Refuse to load values which
> cannot result from normal operation, except mode, the init state of
> which (see also below) cannot otherwise be reached.
> 
> 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. Note
> that with or without this adjustment effectively all switch() statements
> using mode as the control expression aren't quite right when the PIT is
> still in that init state; there is an apparent assumption that before
> these can sensibly be invoked, the guest would init the PIT (i.e. in
> particular set the mode).
> 
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> For mode we could refuse to load values in the [0x08,0xfe] range; I'm

I'm missing something, why should we accept a 0xff mode?  Don't modes
go up to 7 at most (0b111, mode 3).

> not certain that's going to be overly helpful.

I don't have a strong opinion.  Could be done in a separate change
anyway.  I guess since we are at it it might be worth to check for as
much as we can, even if it's not going to affect the logic.

> For count I was considering to clip the saved value to 16 bits (i.e. to
> convert the internally used 0x10000 back to the architectural 0x0000),
> but pit_save() doesn't easily lend itself to such a "fixup". If desired
> perhaps better a separate change anyway.

I would prefer a separate change iff you want to implement this.

> ---
> v3: Slightly adjust two comments. Re-base over rename in earlier patch.
> v2: Introduce separate checking function; switch to refusing to load
>     bogus values. Re-base.
> 
> --- 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
>  
>  #define get_guest_time(v) \
>     (is_hvm_vcpu(v) ? hvm_get_guest_time(v) : (u64)get_s_time())
> @@ -427,6 +428,47 @@ static int cf_check pit_save(struct vcpu
>      return rc;
>  }
>  
> +static int cf_check pit_check(const struct domain *d, hvm_domain_context_t 
> *h)
> +{
> +    const struct hvm_hw_pit *hw;
> +    unsigned int i;
> +
> +    if ( !has_vpit(d) )
> +        return -ENODEV;
> +
> +    hw = hvm_get_entry(PIT, h);
> +    if ( !hw )
> +        return -ENODATA;
> +
> +    /*
> +     * Check to-be-loaded values are within valid range, for them to 
> represent
> +     * actually reachable state.  Uses of some of the values elsewhere assume
> +     * this is the case.  Note that the channels' mode fields aren't checked;
> +     * Xen prior to 4.19 might save them as 0xff.

Oh, OK, so that explains the weird 0xff mode.

Thanks, Roger.