On 04/30/2018 05:01 PM, Marek Marczykowski-Górecki wrote:
Using RING_GET_RESPONSE() on a shared ring is easy to use incorrectly (i.e., by not considering that the other end may alter the data in the shared ring while it is being inspected). Safe usage of a response generally requires taking a local copy.
Provide a RING_COPY_RESPONSE() macro to use instead of RING_GET_RESPONSE() and an open-coded memcpy(). This takes care of ensuring that the copy is done correctly regardless of any possible compiler optimizations.
Use a volatile source to prevent the compiler from reordering or omitting the copy.
This is complementary to XSA155.
CC: stable@vger.kernel.org Signed-off-by: Marek Marczykowski-Górecki marmarek@invisiblethingslab.com
include/xen/interface/io/ring.h | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
diff --git a/include/xen/interface/io/ring.h b/include/xen/interface/io/ring.h index 3f40501..03702f6 100644 --- a/include/xen/interface/io/ring.h +++ b/include/xen/interface/io/ring.h @@ -201,6 +201,20 @@ struct __name##_back_ring { \ #define RING_GET_RESPONSE(_r, _idx) \ (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp)) +/*
- Get a local copy of a response.
- Use this in preference to RING_GET_RESPONSE() so all processing is
- done on a local copy that cannot be modified by the other end.
- Note that https://gcc.gnu.org/bugzilla/show_bug.cgi?id=58145 may cause this
- to be ineffective where _rsp is a struct which consists of only bitfields.
- */
+#define RING_COPY_RESPONSE(_r, _idx, _rsp) do { \
- /* Use volatile to force the copy into _rsp. */ \
- *(_rsp) = *(volatile typeof(_rsp))RING_GET_RESPONSE(_r, _idx); \
+} while (0)
To avoid repeating essentially the same comment, can you move RING_COPY_RESPONSE definition next to RING_COPY_REQUEST and adjust the existing comment? And probably move RING_GET_RESPONSE next to RING_GET_REQUEST? In other words
#define RING_GET_REQUEST #define RING_GET_RESPONSE
/* comment */ #define RING_COPY_REQUEST #define RING_COPY_RESPONSE
Also, perhaps the two can be collapsed together, along the lines of
#define RING_COPY_(action, _r, _idx, _msg) do { \ /* Use volatile to force the copy into _msg. */ \ *(_msg) = *(volatile typeof(_msg))RING_GET_##action(_r, _idx); \ } while (0)
#define RING_COPY_REQUEST(_r, _idx, _req) RING_COPY_(REQUEST, _r, _idx, _req) #define RING_COPY_RESPONSE(_r, _idx, _rsp) RING_COPY_(RESPONSE, _r, _idx, _rsp)
(I have not tried to compile this so it may well be wrong)
-boris
/* Loop termination condition: Would the specified index overflow the ring? */ #define RING_REQUEST_CONS_OVERFLOW(_r, _cons) \ (((_cons) - (_r)->rsp_prod_pvt) >= RING_SIZE(_r))