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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

> This one however just unconditionally reset some values, but might be
> simpler to just test if is_master == !inst and if it's master than bit
> 2 in s->elcr is set?

vpic_elcr_mask() also wants applying (or a respective check carrying
out). Plus int_output can't be left alone, I thunk.

> Also if we are serious about doing some sanity checks in the loaded
> records, we could introduce a checker function for the load machinery.

If we were to check rather than override, then yes, perhaps running
through all check functions first would be desirable.

Jan