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

["Roger Pau Monné" <roger.pau@citrix.com>]

On Wed, Oct 25, 2023 at 01:51:05PM +0200, Jan Beulich wrote:
> On 25.10.2023 12:12, Roger Pau Monné wrote:
> > On Thu, May 11, 2023 at 01:50:33PM +0200, Jan Beulich wrote:
> >> Loading is_master from the state save record can lead to out-of-bounds
> >> accesses via at least the two container_of() uses by vpic_domain() and
> >> __vpic_lock(). Calculate the field from the supplied instance number
> >> instead. Adjust the public header comment accordingly.
> >>
> >> For ELCR follow what vpic_intercept_elcr_io()'s write path and
> >> vpic_reset() do.
> >>
> >> Convert ->int_output (which for whatever reason isn't a 1-bit bitfield)
> >> to boolean, also taking ->init_state into account.
> >>
> >> While there also correct vpic_domain() itself, to use its parameter in
> >> both places.
> >>
> >> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> >> ---
> >> Of course an alternative would be to simply reject state save records
> >> with bogus values.
> > 
> > Likewise on the vPIC one, I feel it might be better to just reject
> > such bogus entries, instead of attempting to amend them.
> 
> Perhaps we should discuss which route to take on the next x86 meeting?
> Then also Andrew would have a chance to voice concerns; not sure if
> he's following the thread.

I don't have a strong opinion.  It seems more prone to errors to try
to adjust state that we know it's wrong.  The adjustments could have
bad interactions, or we might miss other fields that also need
adjusting.  Plus any such 'bogus' state is a sign of something going
wrong.

Thanks, Roger.