netdev - Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.

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: <200708140203.30711.jesper.juhl@gmail.com>
Date:	Tue, 14 Aug 2007 02:03:30 +0200
From:	Jesper Juhl <jesper.juhl@...il.com>
To:	Jeremy Fitzhardinge <jeremy@...p.org>
Cc:	Jeff Garzik <jeff@...zik.org>, NetDev <netdev@...r.kernel.org>,
	Keir Fraser <keir@...source.com>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Jesper Juhl <jesper.juhl@...il.com>
Subject: Re: [PATCH] xen-netfront: Avoid deref'ing skbafter it is potentially freed.

On Monday 13 August 2007 21:54:37 Jeremy Fitzhardinge wrote:
> xennet_tx_bug_gc can free the skb before we use it, so make sure we don't.
> 
> Jeff, this is -rc material.
> 
> Signed-off-by: Keir Fraser <keir@...source.com>
> Signed-off-by: Jeremy Fitzhardinge <jeremy@...source.com>
> Cc: Jeff Garzik <jeff@...zik.org>
> 
> diff -r 8bfc43f6d1b0 drivers/net/xen-netfront.c
> --- a/drivers/net/xen-netfront.c	Tue Aug 07 14:26:30 2007 -0700
> +++ b/drivers/net/xen-netfront.c	Mon Aug 13 09:39:15 2007 -0700
> @@ -566,15 +566,16 @@ static int xennet_start_xmit(struct sk_b
>  	if (notify)
>  		notify_remote_via_irq(np->netdev->irq);
>  
> +	np->stats.tx_bytes += skb->len;
> +	np->stats.tx_packets++;
> +
> +	/* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
>  	xennet_tx_buf_gc(dev);
>  
>  	if (!netfront_tx_slot_available(np))
>  		netif_stop_queue(dev);
>  
>  	spin_unlock_irq(&np->tx_lock);
> -
> -	np->stats.tx_bytes += skb->len;
> -	np->stats.tx_packets++;
>  
>  	return 0;
>  
This moves the updating of both tx_bytes and tx_packets inside the 
spinlock, but as far as I can see we only _really_ need to move the 
tx_bytes update. Considering that we generally want to do as little 
work as possible while holding a lock, wouldn't the following be 
slightly better?


Signed-off-by: Keir Fraser <keir@...source.com>
Signed-off-by: Jeremy Fitzhardinge <jeremy@...source.com>
Cc: Jeff Garzik <jeff@...zik.org>
Signed-off-by: Jesper Juhl <jesper.juhl@...il.com>
---

 drivers/net/xen-netfront.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/net/xen-netfront.c b/drivers/net/xen-netfront.c
index 489f69c..640e270 100644
--- a/drivers/net/xen-netfront.c
+++ b/drivers/net/xen-netfront.c
@@ -566,6 +566,9 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (notify)
 		notify_remote_via_irq(np->netdev->irq);
 
+	np->stats.tx_bytes += skb->len;
+
+	/* Note: It is not safe to access skb after xennet_tx_buf_gc()! */
 	xennet_tx_buf_gc(dev);
 
 	if (!netfront_tx_slot_available(np))
@@ -573,7 +576,6 @@ static int xennet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_unlock_irq(&np->tx_lock);
 
-	np->stats.tx_bytes += skb->len;
 	np->stats.tx_packets++;
 
 	return 0;
-
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