[PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[Radu Rendec]

The use-after-free is actually in rswitch_tx_free(), which is inlined in
rswitch_poll(). Since `skb` and `gq->skbs[gq->dirty]` are in fact the
same pointer, the skb is first freed using dev_kfree_skb_any(), then the
value in skb->len is used to update the interface statistics.

Let's move around the instructions to use skb->len before the skb is
freed.

This bug is trivial to reproduce using KFENCE. It will trigger a splat
every few packets. A simple ARP request or ICMP echo request is enough.

Signed-off-by: Radu Rendec <rrendec@redhat.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index dcab638c57fe8..24c90d8f5a442 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -871,13 +871,13 @@ static void rswitch_tx_free(struct net_device *ndev)
 		dma_rmb();
 		skb = gq->skbs[gq->dirty];
 		if (skb) {
+			rdev->ndev->stats.tx_packets++;
+			rdev->ndev->stats.tx_bytes += skb->len;
 			dma_unmap_single(ndev->dev.parent,
 					 gq->unmap_addrs[gq->dirty],
 					 skb->len, DMA_TO_DEVICE);
 			dev_kfree_skb_any(gq->skbs[gq->dirty]);
 			gq->skbs[gq->dirty] = NULL;
-			rdev->ndev->stats.tx_packets++;
-			rdev->ndev->stats.tx_bytes += skb->len;
 		}
 		desc->desc.die_dt = DT_EEMPTY;
 	}
-- 
2.45.2

RE: [PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[Yoshihiro Shimoda]

Hello Radu,

> From: Radu Rendec, Sent: Wednesday, July 3, 2024 6:09 AM
> 
> The use-after-free is actually in rswitch_tx_free(), which is inlined in
> rswitch_poll(). Since `skb` and `gq->skbs[gq->dirty]` are in fact the
> same pointer, the skb is first freed using dev_kfree_skb_any(), then the
> value in skb->len is used to update the interface statistics.
> 
> Let's move around the instructions to use skb->len before the skb is
> freed.
> 
> This bug is trivial to reproduce using KFENCE. It will trigger a splat
> every few packets. A simple ARP request or ICMP echo request is enough.
> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>

Thank you very much for your patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda

> ---
>  drivers/net/ethernet/renesas/rswitch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index dcab638c57fe8..24c90d8f5a442 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -871,13 +871,13 @@ static void rswitch_tx_free(struct net_device *ndev)
>  		dma_rmb();
>  		skb = gq->skbs[gq->dirty];
>  		if (skb) {
> +			rdev->ndev->stats.tx_packets++;
> +			rdev->ndev->stats.tx_bytes += skb->len;
>  			dma_unmap_single(ndev->dev.parent,
>  					 gq->unmap_addrs[gq->dirty],
>  					 skb->len, DMA_TO_DEVICE);
>  			dev_kfree_skb_any(gq->skbs[gq->dirty]);
>  			gq->skbs[gq->dirty] = NULL;
> -			rdev->ndev->stats.tx_packets++;
> -			rdev->ndev->stats.tx_bytes += skb->len;
>  		}
>  		desc->desc.die_dt = DT_EEMPTY;
>  	}
> --
> 2.45.2

Re: [PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[Niklas Söderlund]

Hi Radu,

On 2024-07-02 17:08:37 -0400, Radu Rendec wrote:
> The use-after-free is actually in rswitch_tx_free(), which is inlined in
> rswitch_poll(). Since `skb` and `gq->skbs[gq->dirty]` are in fact the
> same pointer, the skb is first freed using dev_kfree_skb_any(), then the
> value in skb->len is used to update the interface statistics.
> 
> Let's move around the instructions to use skb->len before the skb is
> freed.
> 
> This bug is trivial to reproduce using KFENCE. It will trigger a splat
> every few packets. A simple ARP request or ICMP echo request is enough.
> 
> Signed-off-by: Radu Rendec <rrendec@redhat.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

> ---
>  drivers/net/ethernet/renesas/rswitch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
> index dcab638c57fe8..24c90d8f5a442 100644
> --- a/drivers/net/ethernet/renesas/rswitch.c
> +++ b/drivers/net/ethernet/renesas/rswitch.c
> @@ -871,13 +871,13 @@ static void rswitch_tx_free(struct net_device *ndev)
>  		dma_rmb();
>  		skb = gq->skbs[gq->dirty];
>  		if (skb) {
> +			rdev->ndev->stats.tx_packets++;
> +			rdev->ndev->stats.tx_bytes += skb->len;
>  			dma_unmap_single(ndev->dev.parent,
>  					 gq->unmap_addrs[gq->dirty],
>  					 skb->len, DMA_TO_DEVICE);
>  			dev_kfree_skb_any(gq->skbs[gq->dirty]);
>  			gq->skbs[gq->dirty] = NULL;
> -			rdev->ndev->stats.tx_packets++;
> -			rdev->ndev->stats.tx_bytes += skb->len;
>  		}
>  		desc->desc.die_dt = DT_EEMPTY;
>  	}
> -- 
> 2.45.2
> 

-- 
Kind Regards,
Niklas Söderlund

Re: [PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[Jakub Kicinski]

On Tue,  2 Jul 2024 17:08:37 -0400 Radu Rendec wrote:
> The use-after-free is actually in rswitch_tx_free(), which is inlined in
> rswitch_poll(). Since `skb` and `gq->skbs[gq->dirty]` are in fact the
> same pointer, the skb is first freed using dev_kfree_skb_any(), then the
> value in skb->len is used to update the interface statistics.
> 
> Let's move around the instructions to use skb->len before the skb is
> freed.
> 
> This bug is trivial to reproduce using KFENCE. It will trigger a splat
> every few packets. A simple ARP request or ICMP echo request is enough.

Please remember to add a Fixes tag in the future. I added one when
applying.

Re: [PATCH] net: rswitch: Avoid use-after-free in rswitch_poll()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue,  2 Jul 2024 17:08:37 -0400 you wrote:
> The use-after-free is actually in rswitch_tx_free(), which is inlined in
> rswitch_poll(). Since `skb` and `gq->skbs[gq->dirty]` are in fact the
> same pointer, the skb is first freed using dev_kfree_skb_any(), then the
> value in skb->len is used to update the interface statistics.
> 
> Let's move around the instructions to use skb->len before the skb is
> freed.
> 
> [...]

Here is the summary with links:
  - net: rswitch: Avoid use-after-free in rswitch_poll()
    https://git.kernel.org/netdev/net/c/9a0c28efeec6

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html