drivers/net/ethernet/seeq/ether3.c | 1 + 1 file changed, 1 insertion(+)
In the ether3_probe function, a timer is initialized with a callback
function ether3_ledoff, bound to &prev(dev)->timer. Once the timer is
started, there is a risk of a race condition if the module or device
is removed, triggering the ether3_remove function to perform cleanup.
The sequence of operations that may lead to a UAF bug is as follows:
CPU0 CPU1
| ether3_ledoff
ether3_remove |
free_netdev(dev); |
put_devic |
kfree(dev); |
| ether3_outw(priv(dev)->regs.config2 |= CFG2_CTRLO, REG_CONFIG2);
| // use dev
Fix it by ensuring that the timer is canceled before proceeding with
the cleanup in ether3_remove.
Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
---
drivers/net/ethernet/seeq/ether3.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/seeq/ether3.c b/drivers/net/ethernet/seeq/ether3.c
index c672f92d65e9..f9d27c9d6808 100644
--- a/drivers/net/ethernet/seeq/ether3.c
+++ b/drivers/net/ethernet/seeq/ether3.c
@@ -850,6 +850,7 @@ static void ether3_remove(struct expansion_card *ec)
ecard_set_drvdata(ec, NULL);
unregister_netdev(dev);
+ del_timer_sync(&priv(dev)->timer);
free_netdev(dev);
ecard_release_resources(ec);
}
--
2.39.1.windows.1On Tue, Sep 10, 2024 at 01:58:21AM +0800, Kaixin Wang wrote: > In the ether3_probe function, a timer is initialized with a callback > function ether3_ledoff, bound to &prev(dev)->timer. Once the timer is > started, there is a risk of a race condition if the module or device > is removed, triggering the ether3_remove function to perform cleanup. > The sequence of operations that may lead to a UAF bug is as follows: > > CPU0 CPU1 > > | ether3_ledoff > ether3_remove | > free_netdev(dev); | > put_devic | > kfree(dev); | > | ether3_outw(priv(dev)->regs.config2 |= CFG2_CTRLO, REG_CONFIG2); > | // use dev > > Fix it by ensuring that the timer is canceled before proceeding with > the cleanup in ether3_remove. > > Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn> This seems like it is a bug fix to me. If so, it should have a Fixes tag (immediately above your Signed-off-by line, no blank line in between). And be targeted at net. Subject: [PATCH net] ... Unless the bug only exists in net-next. Subject: [PATCH net-next] ... Link: https://docs.kernel.org/process/maintainer-netdev.html ...
On 9/9/24 19:58, Kaixin Wang wrote: > In the ether3_probe function, a timer is initialized with a callback > function ether3_ledoff, bound to &prev(dev)->timer. Once the timer is > started, there is a risk of a race condition if the module or device > is removed, triggering the ether3_remove function to perform cleanup. > The sequence of operations that may lead to a UAF bug is as follows: > > CPU0 CPU1 > > | ether3_ledoff > ether3_remove | > free_netdev(dev); | > put_devic | > kfree(dev); | > | ether3_outw(priv(dev)->regs.config2 |= CFG2_CTRLO, REG_CONFIG2); > | // use dev > > Fix it by ensuring that the timer is canceled before proceeding with > the cleanup in ether3_remove. this code change indeed prevents UAF bug but as is, the CFG2_CTRLO flag of REG_CONFIG2 will be left in state "ON" it would be better to first turn the LED off unconditionally > > Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn> > --- > drivers/net/ethernet/seeq/ether3.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/ethernet/seeq/ether3.c b/drivers/net/ethernet/seeq/ether3.c > index c672f92d65e9..f9d27c9d6808 100644 > --- a/drivers/net/ethernet/seeq/ether3.c > +++ b/drivers/net/ethernet/seeq/ether3.c > @@ -850,6 +850,7 @@ static void ether3_remove(struct expansion_card *ec) > ecard_set_drvdata(ec, NULL); > > unregister_netdev(dev); > + del_timer_sync(&priv(dev)->timer); > free_netdev(dev); > ecard_release_resources(ec); > }
At 2024-09-11 17:29:44, "Przemek Kitszel" <przemyslaw.kitszel@intel.com> wrote: >On 9/9/24 19:58, Kaixin Wang wrote: >> In the ether3_probe function, a timer is initialized with a callback >> function ether3_ledoff, bound to &prev(dev)->timer. Once the timer is >> started, there is a risk of a race condition if the module or device >> is removed, triggering the ether3_remove function to perform cleanup. >> The sequence of operations that may lead to a UAF bug is as follows: >> >> CPU0 CPU1 >> >> | ether3_ledoff >> ether3_remove | >> free_netdev(dev); | >> put_devic | >> kfree(dev); | >> | ether3_outw(priv(dev)->regs.config2 |= CFG2_CTRLO, REG_CONFIG2); >> | // use dev >> >> Fix it by ensuring that the timer is canceled before proceeding with >> the cleanup in ether3_remove. > >this code change indeed prevents UAF bug >but as is, the CFG2_CTRLO flag of REG_CONFIG2 will be left in state "ON" > >it would be better to first turn the LED off unconditionally > I will fix it in the next version of patch. >> >> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn> >> --- >> drivers/net/ethernet/seeq/ether3.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/net/ethernet/seeq/ether3.c b/drivers/net/ethernet/seeq/ether3.c >> index c672f92d65e9..f9d27c9d6808 100644 >> --- a/drivers/net/ethernet/seeq/ether3.c >> +++ b/drivers/net/ethernet/seeq/ether3.c >> @@ -850,6 +850,7 @@ static void ether3_remove(struct expansion_card *ec) >> ecard_set_drvdata(ec, NULL); >> >> unregister_netdev(dev); >> + del_timer_sync(&priv(dev)->timer); >> free_netdev(dev); >> ecard_release_resources(ec); >> } > > Thanks for the review! Best regards, Kaixin Wang
© 2016 - 2024 Red Hat, Inc.