netdev - RE: [PATCH 2/3] xen-netback: validate queue numbers in xenvif_set_hash

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]

Date:   Mon, 3 Sep 2018 09:23:53 +0000
From:   Paul Durrant <Paul.Durrant@...rix.com>
To:     'Jan Beulich' <JBeulich@...e.com>, Wei Liu <wei.liu2@...rix.com>
CC:     "davem@...emloft.net" <davem@...emloft.net>,
        xen-devel <xen-devel@...ts.xenproject.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH 2/3] xen-netback: validate queue numbers in
 xenvif_set_hash_mapping()

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@...e.com]
> Sent: 28 August 2018 16:00
> To: Paul Durrant <Paul.Durrant@...rix.com>; Wei Liu <wei.liu2@...rix.com>
> Cc: davem@...emloft.net; xen-devel <xen-devel@...ts.xenproject.org>;
> netdev@...r.kernel.org
> Subject: [PATCH 2/3] xen-netback: validate queue numbers in
> xenvif_set_hash_mapping()
> 
> Checking them before the grant copy means nothing as to the validity of
> the incoming request. As we shouldn't make the new data live before
> having validated it, introduce a second instance of the mapping array.
> 
> Signed-off-by: Jan Beulich <jbeulich@...e.com>

Reviewed-by: Paul Durrant <paul.durrant@...rix.com>

> 
> ---
>  drivers/net/xen-netback/common.h    |    3 ++-
>  drivers/net/xen-netback/hash.c      |   20 ++++++++++++++------
>  drivers/net/xen-netback/interface.c |    3 ++-
>  3 files changed, 18 insertions(+), 8 deletions(-)
> 
> --- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-
> netback/common.h
> +++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-
> netback/common.h
> @@ -241,8 +241,9 @@ struct xenvif_hash_cache {
>  struct xenvif_hash {
>  	unsigned int alg;
>  	u32 flags;
> +	bool mapping_sel;
>  	u8 key[XEN_NETBK_MAX_HASH_KEY_SIZE];
> -	u32 mapping[XEN_NETBK_MAX_HASH_MAPPING_SIZE];
> +	u32 mapping[2][XEN_NETBK_MAX_HASH_MAPPING_SIZE];
>  	unsigned int size;
>  	struct xenvif_hash_cache cache;
>  };
> --- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-
> netback/hash.c
> +++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-
> netback/hash.c
> @@ -324,7 +324,8 @@ u32 xenvif_set_hash_mapping_size(struct
>  		return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
> 
>  	vif->hash.size = size;
> -	memset(vif->hash.mapping, 0, sizeof(u32) * size);
> +	memset(vif->hash.mapping[vif->hash.mapping_sel], 0,
> +	       sizeof(u32) * size);
> 
>  	return XEN_NETIF_CTRL_STATUS_SUCCESS;
>  }
> @@ -332,7 +333,7 @@ u32 xenvif_set_hash_mapping_size(struct
>  u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
>  			    u32 off)
>  {
> -	u32 *mapping = vif->hash.mapping;
> +	u32 *mapping = vif->hash.mapping[!vif->hash.mapping_sel];
>  	struct gnttab_copy copy_op = {
>  		.source.u.ref = gref,
>  		.source.domid = vif->domid,
> @@ -348,9 +349,8 @@ u32 xenvif_set_hash_mapping(struct xenvi
>  	copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
>  	copy_op.dest.offset = xen_offset_in_page(mapping + off);
> 
> -	while (len-- != 0)
> -		if (mapping[off++] >= vif->num_queues)
> -			return
> XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
> +	memcpy(mapping, vif->hash.mapping[vif->hash.mapping_sel],
> +	       vif->hash.size * sizeof(*mapping));
> 
>  	if (copy_op.len != 0) {
>  		gnttab_batch_copy(&copy_op, 1);
> @@ -359,6 +359,12 @@ u32 xenvif_set_hash_mapping(struct xenvi
>  			return
> XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
>  	}
> 
> +	while (len-- != 0)
> +		if (mapping[off++] >= vif->num_queues)
> +			return
> XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
> +
> +	vif->hash.mapping_sel = !vif->hash.mapping_sel;
> +
>  	return XEN_NETIF_CTRL_STATUS_SUCCESS;
>  }
> 
> @@ -410,6 +416,8 @@ void xenvif_dump_hash_info(struct xenvif
>  	}
> 
>  	if (vif->hash.size != 0) {
> +		const u32 *mapping = vif->hash.mapping[vif-
> >hash.mapping_sel];
> +
>  		seq_puts(m, "\nHash Mapping:\n");
> 
>  		for (i = 0; i < vif->hash.size; ) {
> @@ -422,7 +430,7 @@ void xenvif_dump_hash_info(struct xenvif
>  			seq_printf(m, "[%4u - %4u]: ", i, i + n - 1);
> 
>  			for (j = 0; j < n; j++, i++)
> -				seq_printf(m, "%4u ", vif->hash.mapping[i]);
> +				seq_printf(m, "%4u ", mapping[i]);
> 
>  			seq_puts(m, "\n");
>  		}
> --- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-
> netback/interface.c
> +++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-
> netback/interface.c
> @@ -162,7 +162,8 @@ static u16 xenvif_select_queue(struct ne
>  	if (size == 0)
>  		return skb_get_hash_raw(skb) % dev-
> >real_num_tx_queues;
> 
> -	return vif->hash.mapping[skb_get_hash_raw(skb) % size];
> +	return vif->hash.mapping[vif->hash.mapping_sel]
> +				[skb_get_hash_raw(skb) % size];
>  }
> 
>  static int xenvif_start_xmit(struct sk_buff *skb, struct net_device *dev)
> 
> 
>