netdev - Re: [Xen-devel] [PATCH 6/7] xen-netback: coalesce slots and fix regressions

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: <5164221302000078000CBBF2@nat28.tlf.novell.com>
Date:	Tue, 09 Apr 2013 13:13:39 +0100
From:	"Jan Beulich" <JBeulich@...e.com>
To:	"Wei Liu" <wei.liu2@...rix.com>
Cc:	<david.vrabel@...rix.com>, <ian.campbell@...rix.com>,
	<wdauchy@...il.com>, <xen-devel@...ts.xen.org>,
	<annie.li@...cle.com>, <konrad.wilk@...cle.com>,
	<netdev@...r.kernel.org>
Subject: Re: [Xen-devel] [PATCH 6/7] xen-netback: coalesce slots and
 fix regressions

>>> On 09.04.13 at 13:07, Wei Liu <wei.liu2@...rix.com> wrote:
> --- a/drivers/net/xen-netback/netback.c
> +++ b/drivers/net/xen-netback/netback.c
> @@ -47,11 +47,47 @@
>  #include <asm/xen/hypercall.h>
>  #include <asm/xen/page.h>
>  
> +/*
> + * This is the maximum slots a skb can have. If a guest sends a skb
> + * which exceeds this limit it is considered malicious.
> + */
> +#define MAX_SKB_SLOTS_DEFAULT 20
> +static unsigned int max_skb_slots = MAX_SKB_SLOTS_DEFAULT;
> +
> +static int max_skb_slots_set(const char *val, const struct kernel_param *kp)
> +{
> +	int ret;
> +	unsigned long param = 0;
> +
> +	ret = kstrtoul(val, 10, &param);
> +
> +	if (ret < 0 || param < XEN_NETIF_NR_SLOTS_MIN)
> +		return -EINVAL;
> +
> +	max_skb_slots = param;
> +
> +	return 0;
> +}
> +
> +static struct kernel_param_ops max_skb_slots_param_ops = {

__moduleparam_const

> +	.set = max_skb_slots_set,
> +	.get = param_get_ulong,

param_get_uint

> @@ -251,7 +291,7 @@ static int max_required_rx_slots(struct xenvif *vif)
>  	int max = DIV_ROUND_UP(vif->dev->mtu, PAGE_SIZE);
>  
>  	if (vif->can_sg || vif->gso || vif->gso_prefix)
> -		max += MAX_SKB_FRAGS + 1; /* extra_info + frags */
> +		max += XEN_NETIF_NR_SLOTS_MIN + 1; /* extra_info + frags */
>  
>  	return max;
>  }
> @@ -657,7 +697,7 @@ static void xen_netbk_rx_action(struct xen_netbk *netbk)
>  		__skb_queue_tail(&rxq, skb);
>  
>  		/* Filled the batch queue? */
> -		if (count + MAX_SKB_FRAGS >= XEN_NETIF_RX_RING_SIZE)
> +		if (count + XEN_NETIF_NR_SLOTS_MIN >= XEN_NETIF_RX_RING_SIZE)
>  			break;
>  	}
>  

Are the two changes above really correct? You're having an skb as
input here, and hence you want to use all the frags, and nothing
beyond. Another question is whether the frontend can handle
those, but that aspect isn't affected by the code being modified
here.

> @@ -904,38 +944,56 @@ static void netbk_fatal_tx_err(struct xenvif *vif)
>  
>  static int netbk_count_requests(struct xenvif *vif,
>  				struct xen_netif_tx_request *first,
> +				RING_IDX first_idx,
>  				struct xen_netif_tx_request *txp,
>  				int work_to_do)
>  {
>  	RING_IDX cons = vif->tx.req_cons;
> -	int frags = 0;
> +	int slots = 0;
> +	int drop_err = 0;
>  
>  	if (!(first->flags & XEN_NETTXF_more_data))
>  		return 0;
>  
>  	do {
> -		if (frags >= work_to_do) {
> -			netdev_err(vif->dev, "Need more frags\n");
> +		if (slots >= work_to_do) {
> +			netdev_err(vif->dev, "Need more slots\n");
>  			netbk_fatal_tx_err(vif);
>  			return -ENODATA;
>  		}
>  
> -		if (unlikely(frags >= MAX_SKB_FRAGS)) {
> -			netdev_err(vif->dev, "Too many frags\n");
> +		/* Xen network protocol had implicit dependency on
> +		 * MAX_SKB_FRAGS. XEN_NETIF_NR_SLOTS_MIN is set to the
> +		 * historical MAX_SKB_FRAGS value 18 to honor the same
> +		 * behavior as before. Any packet using more than 18
> +		 * slots but less than max_skb_slots slots is dropped
> +		 */
> +		if (!drop_err && slots >= XEN_NETIF_NR_SLOTS_MIN) {
> +			if (net_ratelimit())
> +				netdev_dbg(vif->dev, "Too many slots\n");
> +			drop_err = -E2BIG;
> +		}
> +
> +		/* This guest is really using too many slots and
> +		 * considered malicious.
> +		 */
> +		if (unlikely(slots >= max_skb_slots)) {
> +			netdev_err(vif->dev,
> +				   "Malicious frontend using too many slots\n");

Wouldn't you better swap this and the previous if?

>  			netbk_fatal_tx_err(vif);
>  			return -E2BIG;
>  		}
>  
> -		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + frags),
> +		memcpy(txp, RING_GET_REQUEST(&vif->tx, cons + slots),
>  		       sizeof(*txp));
>  		if (txp->size > first->size) {
> -			netdev_err(vif->dev, "Frag is bigger than frame.\n");
> +			netdev_err(vif->dev, "Packet is bigger than frame.\n");

I don't think "packet" is the right term here.

Jan

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html