| Message ID | 20240909175821.2047-1-kxwang23@m.fudan.edu.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: seeq: Fix use after free vulnerability in ether3 Driver Due to Race Condition | expand |
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
On 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 ...
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); }
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(+)