diff mbox

net: fec: fix potential use after free

Message ID 1387479182.19078.369.camel@edumazet-glaptop2.roam.corp.google.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Dec. 19, 2013, 6:53 p.m. UTC 
From: Eric Dumazet <edumazet@google.com>

skb_tx_timestamp(skb) should be called _before_ TX completion
has a chance to trigger, otherwise it is too late and we access
freed memory.

Signed-off-by: Eric Dumazet <edumazet@google.com>
Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
Cc: Frank Li <Frank.Li@freescale.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/net/ethernet/freescale/fec_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)



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

Comments

Richard Cochran Dec. 19, 2013, 7:32 p.m. UTC | #1 
On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> skb_tx_timestamp(skb) should be called _before_ TX completion
> has a chance to trigger, otherwise it is too late and we access
> freed memory.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")

So the hw_lock spin lock was protecting against this issue?

Nice catch. How ever did you find this?

Acked-by: Richard Cochran <richardcochran@gmail.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Frank Li Dec. 19, 2013, 7:55 p.m. UTC | #2 
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Thursday, December 19, 2013 1:33 PM
> To: Eric Dumazet
> Cc: David Miller; netdev; Li Frank-B20596
> Subject: Re: [PATCH] net: fec: fix potential use after free
> 
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> > skb_tx_timestamp(skb) should be called _before_ TX completion has a
> > chance to trigger, otherwise it is too late and we access freed
> > memory.
> >
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix
> > dead lock")
> 
> So the hw_lock spin lock was protecting against this issue?
> 
> Nice catch. How ever did you find this?
> 
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> 

Acked-by: Frank Li <Frank.Li@freescale.com>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 19, 2013, 8:03 p.m. UTC | #3 
On Thu, 2013-12-19 at 20:32 +0100, Richard Cochran wrote:
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > skb_tx_timestamp(skb) should be called _before_ TX completion
> > has a chance to trigger, otherwise it is too late and we access
> > freed memory.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
> 
> So the hw_lock spin lock was protecting against this issue?
> 
> Nice catch. How ever did you find this?

While looking at another driver, founding the same issue.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhi Li Dec. 19, 2013, 8:05 p.m. UTC | #4 
On Thu, Dec 19, 2013 at 1:32 PM, Richard Cochran
<richardcochran@gmail.com> wrote:
> On Thu, Dec 19, 2013 at 10:53:02AM -0800, Eric Dumazet wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> skb_tx_timestamp(skb) should be called _before_ TX completion
>> has a chance to trigger, otherwise it is too late and we access
>> freed memory.
>>
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")
>
> So the hw_lock spin lock was protecting against this issue?
>
> Nice catch. How ever did you find this?

I think It is not related with hw_lock.  After trigger transfer,
there are possibility that tx completed immediately and irq handle
will free skb.

>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 19, 2013, 8:05 p.m. UTC | #5 
On Thu, 2013-12-19 at 12:03 -0800, Eric Dumazet wrote:

> While looking at another driver, founding the same issue.

( drivers/net/ethernet/arc/emac_main.c )



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Dec. 19, 2013, 8:18 p.m. UTC | #6 
On Thu, 2013-12-19 at 14:05 -0600, Frank Li wrote:

> I think It is not related with hw_lock.  After trigger transfer,
> there are possibility that tx completed immediately and irq handle
> will free skb.

Well, before the de5fb0a05348 commit, tx completion and start_xmit()
could not run in //

So hw_lock was effectively preventing the issue.



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Dec. 19, 2013, 11:31 p.m. UTC | #7 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 19 Dec 2013 10:53:02 -0800

> From: Eric Dumazet <edumazet@google.com>
> 
> skb_tx_timestamp(skb) should be called _before_ TX completion
> has a chance to trigger, otherwise it is too late and we access
> freed memory.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: de5fb0a05348 ("net: fec: put tx to napi poll function to fix dead lock")

Applied and queued up for -stable, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index e7c8b749c5a5..50bb71c663e2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -428,6 +428,8 @@  fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* If this was the last BD in the ring, start at the beginning again. */
 	bdp = fec_enet_get_nextdesc(bdp, fep);
 
+	skb_tx_timestamp(skb);
+
 	fep->cur_tx = bdp;
 
 	if (fep->cur_tx == fep->dirty_tx)
@@ -436,8 +438,6 @@  fec_enet_start_xmit(struct sk_buff *skb, struct net_device *ndev)
 	/* Trigger transmission start */
 	writel(0, fep->hwp + FEC_X_DES_ACTIVE);
 
-	skb_tx_timestamp(skb);
-
 	return NETDEV_TX_OK;
 }