[PATCH 1/3] NET: pch, fix use after free

[PATCH 1/3] NET: pch, fix use after free

[Jiri Slaby]

Stanse found that pch_gbe_xmit_frame uses skb after it is freed. Fix
that.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Masayuki Ohtake <masa-korg@dsn.okisemi.com>
---
 drivers/net/pch_gbe/pch_gbe_main.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/net/pch_gbe/pch_gbe_main.c b/drivers/net/pch_gbe/pch_gbe_main.c
index 53c56cf..e44644f 100644
--- a/drivers/net/pch_gbe/pch_gbe_main.c
+++ b/drivers/net/pch_gbe/pch_gbe_main.c
@@ -1847,9 +1847,9 @@ static int pch_gbe_xmit_frame(struct sk_buff *skb, struct net_device *netdev)
 	unsigned long flags;
 
 	if (unlikely(skb->len > (adapter->hw.mac.max_frame_size - 4))) {
-		dev_kfree_skb_any(skb);
 		pr_err("Transfer length Error: skb len: %d > max: %d\n",
 		       skb->len, adapter->hw.mac.max_frame_size);
+		dev_kfree_skb_any(skb);
 		adapter->stats.tx_length_errors++;
 		return NETDEV_TX_OK;
 	}
-- 
1.7.3.1

[PATCH 2/3] ATM: iphase, remove sleep-inside-atomic

[Jiri Slaby]

Stanse found that ia_init_one locks a spinlock and inside of that it
calls ia_start which calls:
* request_irq
* tx_init which does kmalloc(GFP_KERNEL)

Both of them can thus sleep and result in a deadlock. I don't see a
reason to have a per-device spinlock there which is used only there
and inited right before the lock location. So remove it completely.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Chas Williams <chas@cmf.nrl.navy.mil>
---
 drivers/atm/iphase.c |    6 ------
 drivers/atm/iphase.h |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c
index 8b358d7..9309d47 100644
--- a/drivers/atm/iphase.c
+++ b/drivers/atm/iphase.c
@@ -3156,7 +3156,6 @@ static int __devinit ia_init_one(struct pci_dev *pdev,
 {  
 	struct atm_dev *dev;  
 	IADEV *iadev;  
-        unsigned long flags;
 	int ret;
 
 	iadev = kzalloc(sizeof(*iadev), GFP_KERNEL);
@@ -3188,19 +3187,14 @@ static int __devinit ia_init_one(struct pci_dev *pdev,
 	ia_dev[iadev_count] = iadev;
 	_ia_dev[iadev_count] = dev;
 	iadev_count++;
-	spin_lock_init(&iadev->misc_lock);
-	/* First fixes first. I don't want to think about this now. */
-	spin_lock_irqsave(&iadev->misc_lock, flags); 
 	if (ia_init(dev) || ia_start(dev)) {  
 		IF_INIT(printk("IA register failed!\n");)
 		iadev_count--;
 		ia_dev[iadev_count] = NULL;
 		_ia_dev[iadev_count] = NULL;
-		spin_unlock_irqrestore(&iadev->misc_lock, flags); 
 		ret = -EINVAL;
 		goto err_out_deregister_dev;
 	}
-	spin_unlock_irqrestore(&iadev->misc_lock, flags); 
 	IF_EVENT(printk("iadev_count = %d\n", iadev_count);)
 
 	iadev->next_board = ia_boards;  
diff --git a/drivers/atm/iphase.h b/drivers/atm/iphase.h
index b2cd20f..077735e 100644
--- a/drivers/atm/iphase.h
+++ b/drivers/atm/iphase.h
@@ -1022,7 +1022,7 @@ typedef struct iadev_t {
 	struct dle_q rx_dle_q;  
 	struct free_desc_q *rx_free_desc_qhead;  
 	struct sk_buff_head rx_dma_q;  
-        spinlock_t rx_lock, misc_lock;
+	spinlock_t rx_lock;
 	struct atm_vcc **rx_open;	/* list of all open VCs */  
         u16 num_rx_desc, rx_buf_sz, rxing;
         u32 rx_pkt_ram, rx_tmp_cnt;
-- 
1.7.3.1

[PATCH 3/3] NET: wimax, fix use after free

[Jiri Slaby]

Stanse found that i2400m_rx frees skb, but still uses skb->len even
though it has skb_len defined. So use skb_len properly in the code.

And also define it unsinged int rather than size_t to solve
compilation warnings.

Signed-off-by: Jiri Slaby <jslaby@suse.cz>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Cc: linux-wimax@intel.com
---
 drivers/net/wimax/i2400m/rx.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c
index c4876d0..844133b 100644
--- a/drivers/net/wimax/i2400m/rx.c
+++ b/drivers/net/wimax/i2400m/rx.c
@@ -1244,16 +1244,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
 	int i, result;
 	struct device *dev = i2400m_dev(i2400m);
 	const struct i2400m_msg_hdr *msg_hdr;
-	size_t pl_itr, pl_size, skb_len;
+	size_t pl_itr, pl_size;
 	unsigned long flags;
-	unsigned num_pls, single_last;
+	unsigned num_pls, single_last, skb_len;
 
 	skb_len = skb->len;
-	d_fnstart(4, dev, "(i2400m %p skb %p [size %zu])\n",
+	d_fnstart(4, dev, "(i2400m %p skb %p [size %u])\n",
 		  i2400m, skb, skb_len);
 	result = -EIO;
 	msg_hdr = (void *) skb->data;
-	result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb->len);
+	result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb_len);
 	if (result < 0)
 		goto error_msg_hdr_check;
 	result = -EIO;
@@ -1261,10 +1261,10 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
 	pl_itr = sizeof(*msg_hdr) +	/* Check payload descriptor(s) */
 		num_pls * sizeof(msg_hdr->pld[0]);
 	pl_itr = ALIGN(pl_itr, I2400M_PL_ALIGN);
-	if (pl_itr > skb->len) {	/* got all the payload descriptors? */
+	if (pl_itr > skb_len) {	/* got all the payload descriptors? */
 		dev_err(dev, "RX: HW BUG? message too short (%u bytes) for "
 			"%u payload descriptors (%zu each, total %zu)\n",
-			skb->len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
+			skb_len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
 		goto error_pl_descr_short;
 	}
 	/* Walk each payload payload--check we really got it */
@@ -1272,7 +1272,7 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
 		/* work around old gcc warnings */
 		pl_size = i2400m_pld_size(&msg_hdr->pld[i]);
 		result = i2400m_rx_pl_descr_check(i2400m, &msg_hdr->pld[i],
-						  pl_itr, skb->len);
+						  pl_itr, skb_len);
 		if (result < 0)
 			goto error_pl_descr_check;
 		single_last = num_pls == 1 || i == num_pls - 1;
@@ -1290,16 +1290,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
 	if (i < i2400m->rx_pl_min)
 		i2400m->rx_pl_min = i;
 	i2400m->rx_num++;
-	i2400m->rx_size_acc += skb->len;
-	if (skb->len < i2400m->rx_size_min)
-		i2400m->rx_size_min = skb->len;
-	if (skb->len > i2400m->rx_size_max)
-		i2400m->rx_size_max = skb->len;
+	i2400m->rx_size_acc += skb_len;
+	if (skb_len < i2400m->rx_size_min)
+		i2400m->rx_size_min = skb_len;
+	if (skb_len > i2400m->rx_size_max)
+		i2400m->rx_size_max = skb_len;
 	spin_unlock_irqrestore(&i2400m->rx_lock, flags);
 error_pl_descr_check:
 error_pl_descr_short:
 error_msg_hdr_check:
-	d_fnend(4, dev, "(i2400m %p skb %p [size %zu]) = %d\n",
+	d_fnend(4, dev, "(i2400m %p skb %p [size %u]) = %d\n",
 		i2400m, skb, skb_len, result);
 	return result;
 }
-- 
1.7.3.1

Re: [PATCH 1/3] NET: pch, fix use after free

[Eric Dumazet]

Le lundi 11 octobre 2010 à 11:26 +0200, Jiri Slaby a écrit :
> Stanse found that pch_gbe_xmit_frame uses skb after it is freed. Fix
> that.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> Cc: Masayuki Ohtake <masa-korg@dsn.okisemi.com>
> ---
>  drivers/net/pch_gbe/pch_gbe_main.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)

Applicable to net-next-2.6 only, this driver is not yet in Linus tree

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH 3/3] NET: wimax, fix use after free

[Inaky Perez-Gonzalez]

On Mon, 2010-10-11 at 02:26 -0700, Jiri Slaby wrote: 
> Stanse found that i2400m_rx frees skb, but still uses skb->len even
> though it has skb_len defined. So use skb_len properly in the code.
> 
> And also define it unsinged int rather than size_t to solve
> compilation warnings.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Ops, fail. Thanks for the catch. I assume you have compile tested it.

Acked-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

> Cc: linux-wimax@intel.com
> ---
>  drivers/net/wimax/i2400m/rx.c |   26 +++++++++++++-------------
>  1 files changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/wimax/i2400m/rx.c b/drivers/net/wimax/i2400m/rx.c
> index c4876d0..844133b 100644
> --- a/drivers/net/wimax/i2400m/rx.c
> +++ b/drivers/net/wimax/i2400m/rx.c
> @@ -1244,16 +1244,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
>  	int i, result;
>  	struct device *dev = i2400m_dev(i2400m);
>  	const struct i2400m_msg_hdr *msg_hdr;
> -	size_t pl_itr, pl_size, skb_len;
> +	size_t pl_itr, pl_size;
>  	unsigned long flags;
> -	unsigned num_pls, single_last;
> +	unsigned num_pls, single_last, skb_len;
>  
>  	skb_len = skb->len;
> -	d_fnstart(4, dev, "(i2400m %p skb %p [size %zu])\n",
> +	d_fnstart(4, dev, "(i2400m %p skb %p [size %u])\n",
>  		  i2400m, skb, skb_len);
>  	result = -EIO;
>  	msg_hdr = (void *) skb->data;
> -	result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb->len);
> +	result = i2400m_rx_msg_hdr_check(i2400m, msg_hdr, skb_len);
>  	if (result < 0)
>  		goto error_msg_hdr_check;
>  	result = -EIO;
> @@ -1261,10 +1261,10 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
>  	pl_itr = sizeof(*msg_hdr) +	/* Check payload descriptor(s) */
>  		num_pls * sizeof(msg_hdr->pld[0]);
>  	pl_itr = ALIGN(pl_itr, I2400M_PL_ALIGN);
> -	if (pl_itr > skb->len) {	/* got all the payload descriptors? */
> +	if (pl_itr > skb_len) {	/* got all the payload descriptors? */
>  		dev_err(dev, "RX: HW BUG? message too short (%u bytes) for "
>  			"%u payload descriptors (%zu each, total %zu)\n",
> -			skb->len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
> +			skb_len, num_pls, sizeof(msg_hdr->pld[0]), pl_itr);
>  		goto error_pl_descr_short;
>  	}
>  	/* Walk each payload payload--check we really got it */
> @@ -1272,7 +1272,7 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
>  		/* work around old gcc warnings */
>  		pl_size = i2400m_pld_size(&msg_hdr->pld[i]);
>  		result = i2400m_rx_pl_descr_check(i2400m, &msg_hdr->pld[i],
> -						  pl_itr, skb->len);
> +						  pl_itr, skb_len);
>  		if (result < 0)
>  			goto error_pl_descr_check;
>  		single_last = num_pls == 1 || i == num_pls - 1;
> @@ -1290,16 +1290,16 @@ int i2400m_rx(struct i2400m *i2400m, struct sk_buff *skb)
>  	if (i < i2400m->rx_pl_min)
>  		i2400m->rx_pl_min = i;
>  	i2400m->rx_num++;
> -	i2400m->rx_size_acc += skb->len;
> -	if (skb->len < i2400m->rx_size_min)
> -		i2400m->rx_size_min = skb->len;
> -	if (skb->len > i2400m->rx_size_max)
> -		i2400m->rx_size_max = skb->len;
> +	i2400m->rx_size_acc += skb_len;
> +	if (skb_len < i2400m->rx_size_min)
> +		i2400m->rx_size_min = skb_len;
> +	if (skb_len > i2400m->rx_size_max)
> +		i2400m->rx_size_max = skb_len;
>  	spin_unlock_irqrestore(&i2400m->rx_lock, flags);
>  error_pl_descr_check:
>  error_pl_descr_short:
>  error_msg_hdr_check:
> -	d_fnend(4, dev, "(i2400m %p skb %p [size %zu]) = %d\n",
> +	d_fnend(4, dev, "(i2400m %p skb %p [size %u]) = %d\n",
>  		i2400m, skb, skb_len, result);
>  	return result;
>  }

Re: [PATCH 3/3] NET: wimax, fix use after free

[David Miller]

From: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Date: Mon, 11 Oct 2010 09:46:46 -0700

> On Mon, 2010-10-11 at 02:26 -0700, Jiri Slaby wrote: 
>> Stanse found that i2400m_rx frees skb, but still uses skb->len even
>> though it has skb_len defined. So use skb_len properly in the code.
>> 
>> And also define it unsinged int rather than size_t to solve
>> compilation warnings.
>> 
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
> 
> Ops, fail. Thanks for the catch. I assume you have compile tested it.
> 
> Acked-by: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>

Applied.

Re: [PATCH 2/3] ATM: iphase, remove sleep-inside-atomic

[David Miller]

From: Jiri Slaby <jslaby@suse.cz>
Date: Mon, 11 Oct 2010 11:26:57 +0200

> Stanse found that ia_init_one locks a spinlock and inside of that it
> calls ia_start which calls:
> * request_irq
> * tx_init which does kmalloc(GFP_KERNEL)
> 
> Both of them can thus sleep and result in a deadlock. I don't see a
> reason to have a per-device spinlock there which is used only there
> and inited right before the lock location. So remove it completely.
> 
> Signed-off-by: Jiri Slaby <jslaby@suse.cz>

Applied.

Re: [PATCH 1/3] NET: pch, fix use after free

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Mon, 11 Oct 2010 11:38:38 +0200

> Le lundi 11 octobre 2010 à 11:26 +0200, Jiri Slaby a écrit :
>> Stanse found that pch_gbe_xmit_frame uses skb after it is freed. Fix
>> that.
>> 
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: Masayuki Ohtake <masa-korg@dsn.okisemi.com>
>> ---
>>  drivers/net/pch_gbe/pch_gbe_main.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> Applicable to net-next-2.6 only, this driver is not yet in Linus tree
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

I'll apply this to net-next-2.6, thanks.