Re: [PATCH 6/8] xen/netfront: don't read data from request on the ring page

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2021 12:03, Juergen Gross wrote:
> In order to avoid a malicious backend being able to influence the local
> processing of a request build the request locally first and then copy
> it to the ring page. Any reading from the request needs to be done on
> the local instance.

"Any reading" isn't really true - you don't change xennet_make_one_txreq(),
yet that has a read-modify-write operation. Without that I would have
been inclined to ask whether ...

> --- a/drivers/net/xen-netfront.c
> +++ b/drivers/net/xen-netfront.c
> @@ -435,7 +435,8 @@ struct xennet_gnttab_make_txreq {
>       struct netfront_queue *queue;
>       struct sk_buff *skb;
>       struct page *page;
> -     struct xen_netif_tx_request *tx; /* Last request */
> +     struct xen_netif_tx_request *tx;      /* Last request on ring page */
> +     struct xen_netif_tx_request tx_local; /* Last request local copy*/

... retaining the tx field here is a good idea.

> @@ -463,30 +464,27 @@ static void xennet_tx_setup_grant(unsigned long gfn, 
> unsigned int offset,
>       queue->grant_tx_page[id] = page;
>       queue->grant_tx_ref[id] = ref;
>  
> -     tx->id = id;
> -     tx->gref = ref;
> -     tx->offset = offset;
> -     tx->size = len;
> -     tx->flags = 0;
> +     info->tx_local.id = id;
> +     info->tx_local.gref = ref;
> +     info->tx_local.offset = offset;
> +     info->tx_local.size = len;
> +     info->tx_local.flags = 0;
> +
> +     *tx = info->tx_local;
>  
>       info->tx = tx;
> -     info->size += tx->size;
> +     info->size += info->tx_local.size;
>  }
>  
>  static struct xen_netif_tx_request *xennet_make_first_txreq(
> -     struct netfront_queue *queue, struct sk_buff *skb,
> -     struct page *page, unsigned int offset, unsigned int len)
> +     struct xennet_gnttab_make_txreq *info,
> +     unsigned int offset, unsigned int len)
>  {
> -     struct xennet_gnttab_make_txreq info = {
> -             .queue = queue,
> -             .skb = skb,
> -             .page = page,
> -             .size = 0,
> -     };
> +     info->size = 0;
>  
> -     gnttab_for_one_grant(page, offset, len, xennet_tx_setup_grant, &info);
> +     gnttab_for_one_grant(info->page, offset, len, xennet_tx_setup_grant, 
> info);
>  
> -     return info.tx;
> +     return info->tx;
>  }

Similarly this returning of a pointer into the ring looks at least
risky to me. At the very least it looks as if ...

> @@ -704,14 +699,16 @@ static netdev_tx_t xennet_start_xmit(struct sk_buff 
> *skb, struct net_device *dev
>       }
>  
>       /* First request for the linear area. */
> -     first_tx = tx = xennet_make_first_txreq(queue, skb,
> -                                             page, offset, len);
> +     info.queue = queue;
> +     info.skb = skb;
> +     info.page = page;
> +     first_tx = tx = xennet_make_first_txreq(&info, offset, len);

... you could avoid setting tx here; perhaps the local variable
could go away altogether, showing it's really just first_rx that
is still needed. It's odd that ...

>       offset += tx->size;

... you don't change this one, when ...

>       if (offset == PAGE_SIZE) {
>               page++;
>               offset = 0;
>       }
> -     len -= tx->size;
> +     len -= info.tx_local.size;

... you do so here. Likely just an oversight.

Jan