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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <72cb5265-aca2-8717-6860-19b66ccf9191@suse.com>
Date:   Mon, 17 May 2021 17:08:13 +0200
From:   Jan Beulich <jbeulich@...e.com>
To:     Juergen Gross <jgross@...e.com>
Cc:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Stefano Stabellini <sstabellini@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        xen-devel@...ts.xenproject.org, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 6/8] xen/netfront: don't read data from request on the
 ring page

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