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 <[email protected]>
Reviewed-by: Roger Pau Monné <[email protected]> > --- > 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.
