| Message ID | 976532e6-9514-4a46-8ca1-edf66f22659b@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/HVM: load state checking | expand |
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@suse.com> > --- > 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
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
--- 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])) #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)) #define vpic_unlock(v) spin_unlock(__vpic_lock(v)) #define vpic_is_locked(v) spin_is_locked(__vpic_lock(v)) -#define vpic_elcr_mask(v) ((v)->is_master ? 0xf8 : 0xde) +#define vpic_elcr_mask(v, mb2) ((v)->is_master ? 0xf8 | ((mb2) << 2) : 0xde) /* Return the highest priority found in mask. Return 8 if none. */ #define VPIC_PRIO_NONE 8 @@ -387,7 +387,7 @@ static int cf_check vpic_intercept_elcr_ if ( dir == IOREQ_WRITE ) { /* Some IRs are always edge trig. Slave IR is always level trig. */ - data = (*val >> shift) & vpic_elcr_mask(vpic); + data = (*val >> shift) & vpic_elcr_mask(vpic, 1); if ( vpic->is_master ) data |= 1 << 2; vpic->elcr = data; @@ -395,7 +395,7 @@ static int cf_check vpic_intercept_elcr_ else { /* Reader should not see hardcoded level-triggered slave IR. */ - data = vpic->elcr & vpic_elcr_mask(vpic); + data = vpic->elcr & vpic_elcr_mask(vpic, 0); if ( !shift ) *val = data; else @@ -429,6 +429,38 @@ static int cf_check vpic_save(struct vcp return 0; } +static int cf_check vpic_check(const struct domain *d, hvm_domain_context_t *h) +{ + unsigned int inst = hvm_load_instance(h); + const struct hvm_hw_vpic *s; + + if ( !has_vpic(d) ) + return -ENODEV; + + /* Which PIC is this? */ + if ( inst >= ARRAY_SIZE(d->arch.hvm.vpic) ) + return -ENOENT; + + s = hvm_point_entry(PIC, h); + if ( !s ) + return -ENODATA; + + /* + * Check to-be-loaded values are within valid range, for them to represent + * actually reachable state. Uses of some of the values elsewhere assume + * this is the case. + */ + if ( s->int_output > 1 ) + return -EDOM; + + if ( s->is_master != !inst || + (s->int_output && s->init_state) || + (s->elcr & ~vpic_elcr_mask(s, 1)) ) + return -EINVAL; + + return 0; +} + static int cf_check vpic_load(struct domain *d, hvm_domain_context_t *h) { struct hvm_hw_vpic *s; @@ -438,18 +470,21 @@ static int cf_check vpic_load(struct dom return -ENODEV; /* Which PIC is this? */ - if ( inst > 1 ) - return -ENOENT; + ASSERT(inst < ARRAY_SIZE(d->arch.hvm.vpic)); s = &d->arch.hvm.vpic[inst]; /* Load the state */ if ( hvm_load_entry(PIC, h, s) != 0 ) return -EINVAL; + if ( s->is_master ) + s->elcr |= 1 << 2; + return 0; } -HVM_REGISTER_SAVE_RESTORE(PIC, vpic_save, NULL, vpic_load, 2, HVMSR_PER_DOM); +HVM_REGISTER_SAVE_RESTORE(PIC, vpic_save, vpic_check, vpic_load, 2, + HVMSR_PER_DOM); void vpic_reset(struct domain *d) {
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()). 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@suse.com> --- v2: Introduce separate checking function; switch to refusing to load bogus values. Re-base.