Message ID | 8be5ebf6-c710-e1de-12af-f87137b69c44@suse.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | x86/mm: XSA-299 / 309 / 310 follow-up | expand |
On 20/12/2019 14:19, Jan Beulich wrote: > mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: > Fold page_info lock into type_info"), and the other three never had such > a need, at least going back as far as 3.2.0. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> These presumably want ACCESS_ONCE() to avoid introducing repeated read vulnerabilities? While the mappings might be safe, they still point to live guest data. ~Andrew
On 12/20/19 2:42 PM, Andrew Cooper wrote: > On 20/12/2019 14:19, Jan Beulich wrote: >> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: >> Fold page_info lock into type_info"), and the other three never had such >> a need, at least going back as far as 3.2.0. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > These presumably want ACCESS_ONCE() to avoid introducing repeated read > vulnerabilities? While the mappings might be safe, they still point to > live guest data. The L1 itself should be mapped read-only by the guest, and locked at this point, no? -George
On 20.12.2019 15:42, Andrew Cooper wrote: > On 20/12/2019 14:19, Jan Beulich wrote: >> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: >> Fold page_info lock into type_info"), and the other three never had such >> a need, at least going back as far as 3.2.0. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > These presumably want ACCESS_ONCE() to avoid introducing repeated read > vulnerabilities? While the mappings might be safe, they still point to > live guest data. Oh, yes - will do. Jan
On 20.12.2019 15:47, George Dunlap wrote: > On 12/20/19 2:42 PM, Andrew Cooper wrote: >> On 20/12/2019 14:19, Jan Beulich wrote: >>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: >>> Fold page_info lock into type_info"), and the other three never had such >>> a need, at least going back as far as 3.2.0. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> These presumably want ACCESS_ONCE() to avoid introducing repeated read >> vulnerabilities? While the mappings might be safe, they still point to >> live guest data. > > The L1 itself should be mapped read-only by the guest, and locked at > this point, no? True, but I think it won't hurt to use ACCESS_ONCE() nevertheless - it makes the code more obviously safe independent of any locking knowledge. Jan
On 12/20/19 2:52 PM, Jan Beulich wrote: > On 20.12.2019 15:47, George Dunlap wrote: >> On 12/20/19 2:42 PM, Andrew Cooper wrote: >>> On 20/12/2019 14:19, Jan Beulich wrote: >>>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: >>>> Fold page_info lock into type_info"), and the other three never had such >>>> a need, at least going back as far as 3.2.0. >>>> >>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>> >>> These presumably want ACCESS_ONCE() to avoid introducing repeated read >>> vulnerabilities? While the mappings might be safe, they still point to >>> live guest data. >> >> The L1 itself should be mapped read-only by the guest, and locked at >> this point, no? > > True, but I think it won't hurt to use ACCESS_ONCE() nevertheless > - it makes the code more obviously safe independent of any locking > knowledge. But that might give someone the idea that that it *was* safe to do the type adjustments without the page being locked, which it certainly isn't. -George
On 20.12.2019 15:54, George Dunlap wrote: > On 12/20/19 2:52 PM, Jan Beulich wrote: >> On 20.12.2019 15:47, George Dunlap wrote: >>> On 12/20/19 2:42 PM, Andrew Cooper wrote: >>>> On 20/12/2019 14:19, Jan Beulich wrote: >>>>> mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: >>>>> Fold page_info lock into type_info"), and the other three never had such >>>>> a need, at least going back as far as 3.2.0. >>>>> >>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >>>> >>>> These presumably want ACCESS_ONCE() to avoid introducing repeated read >>>> vulnerabilities? While the mappings might be safe, they still point to >>>> live guest data. >>> >>> The L1 itself should be mapped read-only by the guest, and locked at >>> this point, no? >> >> True, but I think it won't hurt to use ACCESS_ONCE() nevertheless >> - it makes the code more obviously safe independent of any locking >> knowledge. > > But that might give someone the idea that that it *was* safe to do the > type adjustments without the page being locked, which it certainly isn't. It isn't in the common case, but we couldn't, for example, put in ASSERT()s in place of using ACCESS_ONCE(), as new_guest_cr3()'s use of mod_l4_entry() is without lock, yet still safe (because of acting on the otherwise immutable L4 table of a 32-bit guest). FAOD this code path also doesn't need the ACCESS_ONCE() (for the same reason). Jan
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2124,13 +2124,10 @@ static int mod_l1_entry(l1_pgentry_t *pl struct vcpu *pt_vcpu, struct domain *pg_dom) { bool preserve_ad = (cmd == MMU_PT_UPDATE_PRESERVE_AD); - l1_pgentry_t ol1e; + l1_pgentry_t ol1e = *pl1e; struct domain *pt_dom = pt_vcpu->domain; int rc = 0; - if ( unlikely(__copy_from_user(&ol1e, pl1e, sizeof(ol1e)) != 0) ) - return -EFAULT; - ASSERT(!paging_mode_refcounts(pt_dom)); if ( l1e_get_flags(nl1e) & _PAGE_PRESENT ) @@ -2248,8 +2245,7 @@ static int mod_l2_entry(l2_pgentry_t *pl return -EPERM; } - if ( unlikely(__copy_from_user(&ol2e, pl2e, sizeof(ol2e)) != 0) ) - return -EFAULT; + ol2e = *pl2e; if ( l2e_get_flags(nl2e) & _PAGE_PRESENT ) { @@ -2311,8 +2307,7 @@ static int mod_l3_entry(l3_pgentry_t *pl if ( is_pv_32bit_domain(d) && (pgentry_ptr_to_slot(pl3e) >= 3) ) return -EINVAL; - if ( unlikely(__copy_from_user(&ol3e, pl3e, sizeof(ol3e)) != 0) ) - return -EFAULT; + ol3e = *pl3e; if ( l3e_get_flags(nl3e) & _PAGE_PRESENT ) { @@ -2378,8 +2373,7 @@ static int mod_l4_entry(l4_pgentry_t *pl return -EINVAL; } - if ( unlikely(__copy_from_user(&ol4e, pl4e, sizeof(ol4e)) != 0) ) - return -EFAULT; + ol4e = *pl4e; if ( l4e_get_flags(nl4e) & _PAGE_PRESENT ) {
mod_l1_entry()'s need to do so went away with commit 2d0557c5cb ("x86: Fold page_info lock into type_info"), and the other three never had such a need, at least going back as far as 3.2.0. Signed-off-by: Jan Beulich <jbeulich@suse.com>