| Message ID | 20240702210838.2703228-1-rrendec@redhat.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 9a0c28efeec6383ef22e97437616b920e7320b67 |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | net: rswitch: Avoid use-after-free in rswitch_poll() | expand |
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
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 >
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.
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!
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; }
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(-)