| Message ID | 20221203092941.10880-1-yuehaibing@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 5a5a3e564de6a8db987410c5c2f4748d50ea82b8 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] ravb: Fix potential use-after-free in ravb_rx_gbeth() | expand |
Hi YueHaibing, Thanks for the feedback. > Subject: [PATCH net] ravb: Fix potential use-after-free in > ravb_rx_gbeth() > > The skb is delivered to napi_gro_receive() which may free it, after > calling this, dereferencing skb may trigger use-after-free. Can you please reconfirm the changes you have done is actually fixing any issue? If yes, please provide the details. Current code, napi_gro_receive(&priv->napi[q], priv->rx_1st_skb); - stats->rx_bytes += priv->rx_1st_skb->len; + stats->rx_bytes += pkt_len; Note: I haven't tested your patch yet to see it cause any regression. Cheers, Biju > > Fixes: 1c59eb678cbd ("ravb: Fillup ravb_rx_gbeth() stub") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > b/drivers/net/ethernet/renesas/ravb_main.c > index 6bc923326268..33f723a9f471 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -841,7 +841,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, > int *quota, int q) > napi_gro_receive(&priv->napi[q], > priv->rx_1st_skb); > stats->rx_packets++; > - stats->rx_bytes += priv->rx_1st_skb->len; > + stats->rx_bytes += pkt_len; > break; > } > } > -- > 2.34.1
Hi Biju, On Sat, Dec 3, 2022 at 11:29 AM Biju Das <biju.das.jz@bp.renesas.com> wrote: > > Subject: [PATCH net] ravb: Fix potential use-after-free in > > ravb_rx_gbeth() > > > > The skb is delivered to napi_gro_receive() which may free it, after > > calling this, dereferencing skb may trigger use-after-free. > > Can you please reconfirm the changes you have done is actually fixing any issue? > If yes, please provide the details. > > Current code, > > napi_gro_receive(&priv->napi[q], priv->rx_1st_skb); IIUIC, after this, priv->rx_1st_skb may have been freed... > > - stats->rx_bytes += priv->rx_1st_skb->len; ... so accessing priv->rx_1st_skb->len here may be a UAF. > + stats->rx_bytes += pkt_len; So this change looks correct to me, as pkt_len was stored to priv->rx_1st_skb->len using skb_put() before. Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Note: I haven't tested your patch yet to see it cause any regression. > > Cheers, > Biju > > > > > Fixes: 1c59eb678cbd ("ravb: Fillup ravb_rx_gbeth() stub") > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > --- > > drivers/net/ethernet/renesas/ravb_main.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > b/drivers/net/ethernet/renesas/ravb_main.c > > index 6bc923326268..33f723a9f471 100644 > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > @@ -841,7 +841,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, > > int *quota, int q) > > napi_gro_receive(&priv->napi[q], > > priv->rx_1st_skb); > > stats->rx_packets++; > > - stats->rx_bytes += priv->rx_1st_skb->len; > > + stats->rx_bytes += pkt_len; > > break; > > } > > } Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > Subject: Re: [PATCH net] ravb: Fix potential use-after-free in > ravb_rx_gbeth() > > Hi Biju, > > On Sat, Dec 3, 2022 at 11:29 AM Biju Das <biju.das.jz@bp.renesas.com> > wrote: > > > Subject: [PATCH net] ravb: Fix potential use-after-free in > > > ravb_rx_gbeth() > > > > > > The skb is delivered to napi_gro_receive() which may free it, after > > > calling this, dereferencing skb may trigger use-after-free. > > > > Can you please reconfirm the changes you have done is actually fixing > any issue? > > If yes, please provide the details. > > > > Current code, > > > > napi_gro_receive(&priv->napi[q], priv->rx_1st_skb); > > IIUIC, after this, priv->rx_1st_skb may have been freed... > > > > > - stats->rx_bytes += priv->rx_1st_skb->len; > > ... so accessing priv->rx_1st_skb->len here may be a UAF. > > > + stats->rx_bytes += pkt_len; > > So this change looks correct to me, as pkt_len was stored to > priv->rx_1st_skb->len using skb_put() before. Thanks for detailed explanation. It makes sense now. Cheers, Biju > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > > > > Note: I haven't tested your patch yet to see it cause any regression. > > > > Cheers, > > Biju > > > > > > > > Fixes: 1c59eb678cbd ("ravb: Fillup ravb_rx_gbeth() stub") > > > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > > > --- > > > drivers/net/ethernet/renesas/ravb_main.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c > > > b/drivers/net/ethernet/renesas/ravb_main.c > > > index 6bc923326268..33f723a9f471 100644 > > > --- a/drivers/net/ethernet/renesas/ravb_main.c > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c > > > @@ -841,7 +841,7 @@ static bool ravb_rx_gbeth(struct net_device > > > *ndev, int *quota, int q) > > > napi_gro_receive(&priv->napi[q], > > > priv->rx_1st_skb); > > > stats->rx_packets++; > > > - stats->rx_bytes += priv->rx_1st_skb- > >len; > > > + stats->rx_bytes += pkt_len; > > > break; > > > } > > > } > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux- > m68k.org > > In personal conversations with technical people, I call myself a hacker. > But when I'm talking to journalists I just say "programmer" or something > like that. > -- Linus Torvalds
Hello: This patch was applied to netdev/net.git (master) by Paolo Abeni <pabeni@redhat.com>: On Sat, 3 Dec 2022 17:29:41 +0800 you wrote: > The skb is delivered to napi_gro_receive() which may free it, after calling this, > dereferencing skb may trigger use-after-free. > > Fixes: 1c59eb678cbd ("ravb: Fillup ravb_rx_gbeth() stub") > Signed-off-by: YueHaibing <yuehaibing@huawei.com> > --- > drivers/net/ethernet/renesas/ravb_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Here is the summary with links: - [net] ravb: Fix potential use-after-free in ravb_rx_gbeth() https://git.kernel.org/netdev/net/c/5a5a3e564de6 You are awesome, thank you!
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 6bc923326268..33f723a9f471 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -841,7 +841,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q) napi_gro_receive(&priv->napi[q], priv->rx_1st_skb); stats->rx_packets++; - stats->rx_bytes += priv->rx_1st_skb->len; + stats->rx_bytes += pkt_len; break; } }
The skb is delivered to napi_gro_receive() which may free it, after calling this, dereferencing skb may trigger use-after-free. Fixes: 1c59eb678cbd ("ravb: Fillup ravb_rx_gbeth() stub") Signed-off-by: YueHaibing <yuehaibing@huawei.com> --- drivers/net/ethernet/renesas/ravb_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)