On 16/11/2023 1:48 pm, 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(). Make sure the value is consistent with the instance being > loaded. > > For ->int_output (which for whatever reason isn't a 1-bit bitfield), > besides bounds checking also take ->init_state into account. > > For ELCR follow what vpic_intercept_elcr_io()'s write path and > vpic_reset() do, i.e. don't insist on the internal view of the value to > be saved. Adjust vpic_elcr_mask() to allow using it easily for the new > case, but still also especially in the 2nd of the uses by > vpic_intercept_elcr_io()).
I'm afraid I'm totally lost trying to follow this change. What is mb2 and why is it variable? This does look like a logically unrelated change (the only overlap is using the new vpic_elcr_mask() form to audit the incoming data), so I think it needs breaking out simply for review-ability. > > Move the instance range check as well, leaving just an assertion in the > load handler. > > While there also correct vpic_domain() itself, to use its parameter in > both places. > > Signed-off-by: Jan Beulich <[email protected]> > --- > v2: Introduce separate checking function; switch to refusing to load > bogus values. Re-base. > > --- a/xen/arch/x86/hvm/vpic.c > +++ b/xen/arch/x86/hvm/vpic.c > @@ -35,13 +35,13 @@ > #include <asm/hvm/save.h> > > #define vpic_domain(v) (container_of((v), struct domain, \ > - arch.hvm.vpic[!vpic->is_master])) > + arch.hvm.vpic[!(v)->is_master])) This appears to have only compiled before because both callers have vpic as their parameter. I think this is worthy of breaking out into a separate change, because it wants backporting further than I expect you're likely to want to backport this patch in general. ~Andrew
