[PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free

[PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free

[Bogdan Purcareata]

Once a Tx frame descriptor is enqueued, an interrupt might be triggered
to process the Tx confirmation and free the skb, hitting a memory use
after free when updating the tx_bytes statistic based on skb->len.

Use the frame descriptor length instead.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index b9a0a31..0f3e497 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -616,7 +616,7 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev)
 		free_tx_fd(priv, &fd, NULL);
 	} else {
 		percpu_stats->tx_packets++;
-		percpu_stats->tx_bytes += skb->len;
+		percpu_stats->tx_bytes += dpaa2_fd_get_len(&fd);
 	}
 
 	return NETDEV_TX_OK;
-- 
2.7.4

[PATCH 2/2] staging: fsl-dpaa2/eth: Error report format fixes

[Bogdan Purcareata]

Fix mishaps in error format strings.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
index 0f3e497..26017fe 100644
--- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
+++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
@@ -656,7 +656,7 @@ static void dpaa2_eth_tx_conf(struct dpaa2_eth_priv *priv,
 		has_fas_errors = (fd_errors & DPAA2_FD_CTRL_FAERR) &&
 				 !!(dpaa2_fd_get_frc(fd) & DPAA2_FD_FRC_FASV);
 		if (net_ratelimit())
-			netdev_dbg(priv->net_dev, "TX frame FD error: %x08\n",
+			netdev_dbg(priv->net_dev, "TX frame FD error: 0x%08x\n",
 				   fd_errors);
 	}
 
@@ -670,7 +670,7 @@ static void dpaa2_eth_tx_conf(struct dpaa2_eth_priv *priv,
 	percpu_stats->tx_errors++;
 
 	if (has_fas_errors && net_ratelimit())
-		netdev_dbg(priv->net_dev, "TX frame FAS error: %x08\n",
+		netdev_dbg(priv->net_dev, "TX frame FAS error: 0x%08x\n",
 			   status & DPAA2_FAS_TX_ERR_MASK);
 }
 
-- 
2.7.4

Re: [PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free

[Dan Carpenter]

On Thu, Jul 20, 2017 at 10:58:37AM +0000, Bogdan Purcareata wrote:
> Once a Tx frame descriptor is enqueued, an interrupt might be triggered
> to process the Tx confirmation and free the skb, hitting a memory use
> after free when updating the tx_bytes statistic based on skb->len.
> 
> Use the frame descriptor length instead.
> 
> Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> ---
>  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> index b9a0a31..0f3e497 100644
> --- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> +++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> @@ -616,7 +616,7 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb, struct net_device *net_dev)
>  		free_tx_fd(priv, &fd, NULL);
>  	} else {
>  		percpu_stats->tx_packets++;
> -		percpu_stats->tx_bytes += skb->len;
> +		percpu_stats->tx_bytes += dpaa2_fd_get_len(&fd);

This feels like the wrong thing.  Can't we just save skb->len earlier
in the function and use it here?  This is the common case right?  So
we'd be saving slightly wrong information for almost every packet.

regards,
dan carpenter

RE: [PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free

[Bogdan Purcareata]

> -----Original Message-----
> From: Dan Carpenter [mailto:dan.carpenter@oracle.com]
> Sent: Thursday, July 20, 2017 2:18 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: Ruxandra Ioana Radulescu <ruxandra.radulescu@nxp.com>;
> gregkh@linuxfoundation.org; linux-kernel@vger.kernel.org;
> devel@driverdev.osuosl.org
> Subject: Re: [PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free
> 
> On Thu, Jul 20, 2017 at 10:58:37AM +0000, Bogdan Purcareata wrote:
> > Once a Tx frame descriptor is enqueued, an interrupt might be triggered
> > to process the Tx confirmation and free the skb, hitting a memory use
> > after free when updating the tx_bytes statistic based on skb->len.
> >
> > Use the frame descriptor length instead.
> >
> > Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > ---
> >  drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> > index b9a0a31..0f3e497 100644
> > --- a/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> > +++ b/drivers/staging/fsl-dpaa2/ethernet/dpaa2-eth.c
> > @@ -616,7 +616,7 @@ static netdev_tx_t dpaa2_eth_tx(struct sk_buff *skb,
> struct net_device *net_dev)
> >  		free_tx_fd(priv, &fd, NULL);
> >  	} else {
> >  		percpu_stats->tx_packets++;
> > -		percpu_stats->tx_bytes += skb->len;
> > +		percpu_stats->tx_bytes += dpaa2_fd_get_len(&fd);
> 
> This feels like the wrong thing.  Can't we just save skb->len earlier
> in the function and use it here?  This is the common case right?  So
> we'd be saving slightly wrong information for almost every packet.

The "len" field in the frame descriptor means the length of the actual data, like the "len" field in the skb. It's set to skb->len both for linear (build_single_fd) and fragmented (build_sg_fd) skbs. I thought it would be more straightforward to use it rather than define an additional local variable solely for this purpose.

Thank you!
Bogdan P.

Re: [PATCH 1/2] staging: fsl-dpaa2/eth: Fix skb use after free

[Dan Carpenter]

Ah.  Good.  My mistake.

regards,
dan carpenter