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

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

[Jan Beulich]

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.

--- a/xen/arch/x86/hvm/vpic.c
+++ b/xen/arch/x86/hvm/vpic.c
@@ -35,7 +35,7 @@
 #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]))
 #define __vpic_lock(v) &container_of((v), struct hvm_domain, \
                                         vpic[!(v)->is_master])->irq_lock
 #define vpic_lock(v)   spin_lock(__vpic_lock(v))
@@ -437,6 +437,14 @@ static int cf_check vpic_load(struct dom
     if ( hvm_load_entry(PIC, h, s) != 0 )
         return -EINVAL;
 
+    s->is_master = !inst;
+
+    s->elcr &= vpic_elcr_mask(s);
+    if ( s->is_master )
+        s->elcr |= 1 << 2;
+
+    s->int_output = !s->init_state && s->int_output;
+
     return 0;
 }
 
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -329,7 +329,10 @@ struct hvm_hw_vpic {
     /* Special mask mode excludes masked IRs from AEOI and priority checks. */
     uint8_t special_mask_mode:1;
 
-    /* Is this a master PIC or slave PIC? (NB. This is not programmable.) */
+    /*
+     * Is this the master PIC or a slave one? (NB. This is not programmable,
+     * and hence is ignored upon loading.)
+     */
     uint8_t is_master:1;
 
     /* Edge/trigger selection. */

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

[Roger Pau Monné]

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.

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?

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

Thanks, Roger.

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

[Jan Beulich]

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.

> 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

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

[Roger Pau Monné]

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.