[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 2/3] xen/blkfront: don't take local copy of a request from the ring page



On Fri, Jul 30, 2021 at 12:38:53PM +0200, Juergen Gross wrote:
> In order to avoid a malicious backend being able to influence the local
> copy of a request build the request locally first and then copy it to
> the ring page instead of doing it the other way round as today.
> 
> Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
> V2:
> - init variable to avoid potential compiler warning (Jan Beulich)
> ---
>  drivers/block/xen-blkfront.c | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 15e840287734..b7301006fb28 100644

(...)

> @@ -827,10 +832,10 @@ static int blkif_queue_rw_req(struct request *req, 
> struct blkfront_ring_info *ri
>       if (setup.segments)
>               kunmap_atomic(setup.segments);
>  
> -     /* Keep a private copy so we can reissue requests when recovering. */
> -     rinfo->shadow[id].req = *ring_req;
> +     /* Copy request(s) to the ring page. */
> +     *final_ring_req = *ring_req;

Is this guaranteed to not be optimized by the compiler in an unsafe way
(like, do the operation the other way around)?
My version of the patch had "wmb()" just before, maybe a good idea to
add it here too?

>       if (unlikely(require_extra_req))
> -             rinfo->shadow[extra_id].req = *extra_ring_req;
> +             *final_extra_ring_req = *extra_ring_req;
>  
>       if (new_persistent_gnts)
>               gnttab_free_grant_references(setup.gref_head);
> -- 
> 2.26.2
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.