| Message ID | 1460457796-1779-3-git-send-email-wei.liu2@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/04/16 11:43, Wei Liu wrote: > It should be placed before first time producer and consumer are used. This change isn't necessary and is confusing as this is not what this barrier is for. The barrier needs to be between the load of prod and the load of the ring contents (there's even a comment that says this). This pairs with the corresponding write barrier between the store of the ring contents and the store of prod (in the other end). David > > Signed-off-by: Wei Liu <wei.liu2@citrix.com> > --- > Cc: Stefano Stabellini <sstabellini@kernel.org> > Cc: Anthony Perard <anthony.perard@citrix.com> > > Backport candidate to our own tree. > --- > hw/display/xenfb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c > index 9866dfd..7f4fad7 100644 > --- a/hw/display/xenfb.c > +++ b/hw/display/xenfb.c > @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb) > > prod = page->out_prod; > out_cons = page->out_cons; > + xen_rmb(); > if (prod - out_cons > XENFB_OUT_RING_LEN) { > return; > } > - xen_rmb(); /* ensure we see ring contents up to prod */ > for (cons = out_cons; cons != prod; cons++) { > union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons); > uint8_t type = event->type; >
On 12/04/16 13:57, David Vrabel wrote: > On 12/04/16 11:43, Wei Liu wrote: >> It should be placed before first time producer and consumer are used. > This change isn't necessary and is confusing as this is not what this > barrier is for. > > The barrier needs to be between the load of prod and the load of the > ring contents (there's even a comment that says this). This pairs with > the corresponding write barrier between the store of the ring contents > and the store of prod (in the other end). Looking further, this code will compile to multiple reads of the page, because there is no ACCESS_ONCE(). This code is still vulnerable to XSA-155. ~Andrew > > David > >> Signed-off-by: Wei Liu <wei.liu2@citrix.com> >> --- >> Cc: Stefano Stabellini <sstabellini@kernel.org> >> Cc: Anthony Perard <anthony.perard@citrix.com> >> >> Backport candidate to our own tree. >> --- >> hw/display/xenfb.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c >> index 9866dfd..7f4fad7 100644 >> --- a/hw/display/xenfb.c >> +++ b/hw/display/xenfb.c >> @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb) >> >> prod = page->out_prod; >> out_cons = page->out_cons; >> + xen_rmb(); >> if (prod - out_cons > XENFB_OUT_RING_LEN) { >> return; >> } >> - xen_rmb(); /* ensure we see ring contents up to prod */ >> for (cons = out_cons; cons != prod; cons++) { >> union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons); >> uint8_t type = event->type; >> > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
On Tue, Apr 12, 2016 at 02:38:13PM +0100, Andrew Cooper wrote: > On 12/04/16 13:57, David Vrabel wrote: > > On 12/04/16 11:43, Wei Liu wrote: > >> It should be placed before first time producer and consumer are used. > > This change isn't necessary and is confusing as this is not what this > > barrier is for. > > > > The barrier needs to be between the load of prod and the load of the > > ring contents (there's even a comment that says this). This pairs with > > the corresponding write barrier between the store of the ring contents > > and the store of prod (in the other end). > > Looking further, this code will compile to multiple reads of the page, > because there is no ACCESS_ONCE(). This code is still vulnerable to > XSA-155. > Oops, accidentally kicked over a can of worms. Should have just sent patch 1. :-) Jokes aside, more time is needed to fix this properly. So maybe we should just upstream patch #1 first. Stefano? Anthony? Wei.
On Tue, 12 Apr 2016, Wei Liu wrote: > On Tue, Apr 12, 2016 at 02:38:13PM +0100, Andrew Cooper wrote: > > On 12/04/16 13:57, David Vrabel wrote: > > > On 12/04/16 11:43, Wei Liu wrote: > > >> It should be placed before first time producer and consumer are used. > > > This change isn't necessary and is confusing as this is not what this > > > barrier is for. > > > > > > The barrier needs to be between the load of prod and the load of the > > > ring contents (there's even a comment that says this). This pairs with > > > the corresponding write barrier between the store of the ring contents > > > and the store of prod (in the other end). > > > > Looking further, this code will compile to multiple reads of the page, > > because there is no ACCESS_ONCE(). This code is still vulnerable to > > XSA-155. There is no ACCESS_ONCE in QEMU, the closest thing to it is atomic_read. > Oops, accidentally kicked over a can of worms. Should have just sent > patch 1. :-) > > Jokes aside, more time is needed to fix this properly. So maybe we > should just upstream patch #1 first. Stefano? Anthony? Sure
diff --git a/hw/display/xenfb.c b/hw/display/xenfb.c index 9866dfd..7f4fad7 100644 --- a/hw/display/xenfb.c +++ b/hw/display/xenfb.c @@ -775,10 +775,10 @@ static void xenfb_handle_events(struct XenFB *xenfb) prod = page->out_prod; out_cons = page->out_cons; + xen_rmb(); if (prod - out_cons > XENFB_OUT_RING_LEN) { return; } - xen_rmb(); /* ensure we see ring contents up to prod */ for (cons = out_cons; cons != prod; cons++) { union xenfb_out_event *event = &XENFB_OUT_RING_REF(page, cons); uint8_t type = event->type;
It should be placed before first time producer and consumer are used. Signed-off-by: Wei Liu <wei.liu2@citrix.com> --- Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Anthony Perard <anthony.perard@citrix.com> Backport candidate to our own tree. --- hw/display/xenfb.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)