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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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 <jbeulich@xxxxxxxx>
> ---
> 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