| Message ID | 272862bd-0b05-9a66-c79e-76502b89dd38@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:20, Jan Beulich wrote: > get_page_light()'s use of cmpxchg() is a full barrier already anyway. > > Signed-off-by: Jan Beulich <jbeulich@suse.com> While true, is this actually a clever change to make? The implementation of get_page_light() could plausibly change and no longer be a full barrier, introducing a vulnerability here. OTOH, smp_wmb() is free.
On 20.12.2019 15:51, Andrew Cooper wrote: > On 20/12/2019 14:20, Jan Beulich wrote: >> get_page_light()'s use of cmpxchg() is a full barrier already anyway. >> >> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > While true, is this actually a clever change to make? > > The implementation of get_page_light() could plausibly change and no > longer be a full barrier, introducing a vulnerability here. OTOH, > smp_wmb() is free. It's free at the CPU level, but not at the compiler one (where it still is a barrier). I also don't think get_page_light() could change as drastically as losing its LOCK-ed operation, or if it did the author (and reviewer) would be well advised to at least briefly audit use sites (one of the reasons I'm leaving a comment). Jan
On 20/12/2019 14:55, Jan Beulich wrote: > On 20.12.2019 15:51, Andrew Cooper wrote: >> On 20/12/2019 14:20, Jan Beulich wrote: >>> get_page_light()'s use of cmpxchg() is a full barrier already anyway. >>> >>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> While true, is this actually a clever change to make? >> >> The implementation of get_page_light() could plausibly change and no >> longer be a full barrier, introducing a vulnerability here. OTOH, >> smp_wmb() is free. > It's free at the CPU level, but not at the compiler one (where > it still is a barrier). I also don't think get_page_light() > could change as drastically as losing its LOCK-ed operation, > or if it did the author (and reviewer) would be well advised > to at least briefly audit use sites (one of the reasons I'm > leaving a comment). The comment should be at get_page_light() then, saying "some callers depend on this function being a full memory barrier", which will be far more obvious for anyone who changes the behaviour. With an adjustment along those lines, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -2746,7 +2746,7 @@ static int _put_final_page_type(struct p else { BUG_ON(rc != -ERESTART); - smp_wmb(); + /* get_page_light() includes a full barrier. */ get_page_light(page); page->u.inuse.type_info |= PGT_partial; }
get_page_light()'s use of cmpxchg() is a full barrier already anyway. Signed-off-by: Jan Beulich <jbeulich@suse.com>