[PATCH] net: fec: fix potential use after free

[PATCH] net: fec: fix potential use after free

[Eric Dumazet]

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(-)

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;
 }
 

Re: [PATCH] net: fec: fix potential use after free

[Richard Cochran]

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>

RE: [PATCH] net: fec: fix potential use after free

[Frank.Li]

> -----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>

Re: [PATCH] net: fec: fix potential use after free

[Eric Dumazet]

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.

Re: [PATCH] net: fec: fix potential use after free

[Frank Li]

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

Re: [PATCH] net: fec: fix potential use after free

[Eric Dumazet]

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 )

Re: [PATCH] net: fec: fix potential use after free

[Eric Dumazet]

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.

RE: [PATCH] net: fec: fix potential use after free

[Frank.Li]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Thursday, December 19, 2013 2:19 PM
> To: Frank Li
> Cc: Richard Cochran; David Miller; netdev; Li Frank-B20596
> Subject: Re: [PATCH] net: fec: fix potential use after free
> 
> 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.
> 	

Yes, IRQ handle will wait for spin_lock. 

Re: [PATCH] net: fec: fix potential use after free

[David Miller]

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.