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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 22.11.2023 02:26, Andrew Cooper wrote:
> 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?

Master bit 2 (i.e. the value for bit 2 iff dealing with the master PIC).
I'm afraid I couldn't think of anything better that wouldn't be unduly
long.

> 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.

I can split it out, but I'm afraid it makes little sense when split. So
I'd first like to double check with you that with the naming choice
clarified, you still think you'd see this making sense as a change on
its own.

>> --- 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.

Indeed.

> 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.

Can do. Since there's no breakage from this, I wouldn't have thought of
backporting the change, though. Hence also why I folded it into here.

Jan