Re: [Xen-devel] [PATCH] xen: Add RING_COPY_RESPONSE()

[Oleksandr Andrushchenko <andr2000@xxxxxxxxx>]

On 05/01/2018 12:54 AM, 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.
I do not agree with that. Copying still doesn't make all the above safe
as nothing prevents the backend from overwriting the response while
frontend makes its 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.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> ---
>   xen/include/public/io/ring.h | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
>
> diff --git a/xen/include/public/io/ring.h b/xen/include/public/io/ring.h
> index 30342fc8c0..230fc34cba 100644
> --- a/xen/include/public/io/ring.h
> +++ b/xen/include/public/io/ring.h
> @@ -227,22 +227,25 @@ typedef struct __name##_back_ring __name##_back_ring_t
>   #define RING_GET_REQUEST(_r, _idx)                                      \
>       (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].req))
>   
> +#define RING_GET_RESPONSE(_r, _idx)                                     \
> +    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
> +
>   /*
> - * Get a local copy of a request.
> + * Get a local copy of a request/response.
>    *
> - * Use this in preference to RING_GET_REQUEST() so all processing is
> - * done on a local copy that cannot be modified by the other end.
> + * Use this in preference to RING_GET_REQUEST()/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 _req is a struct which consists of only bitfields.
>    */
> -#define RING_COPY_REQUEST(_r, _idx, _req) do {                         \
> +#define RING_COPY_(action, _r, _idx, _req) do {                                
> \
>         /* Use volatile to force the copy into _req. */                 \
> -       *(_req) = *(volatile typeof(_req))RING_GET_REQUEST(_r, _idx);   \
> +       *(_req) = *(volatile typeof(_req))RING_GET_##action(_r, _idx);  \
>   } while (0)
>   
> -#define RING_GET_RESPONSE(_r, _idx)                                     \
> -    (&((_r)->sring->ring[((_idx) & (RING_SIZE(_r) - 1))].rsp))
> +#define RING_COPY_REQUEST(_r, _idx, _req) RING_COPY_(REQUEST, _r, _idx, _req)
> +#define RING_COPY_RESPONSE(_r, _idx, _req) RING_COPY_(RESPONSE, _r, _idx, _req)
>   
>   /* Loop termination condition: Would the specified index overflow the ring? */
>   #define RING_REQUEST_CONS_OVERFLOW(_r, _cons)                           \

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel