drivers/hsi/clients/ssi_protocol.c | 1 + 1 file changed, 1 insertion(+)
In the ssi_protocol_probe function, &ssi->work is bound with
ssip_xmit_work, In ssip_pn_setup, the ssip_pn_xmit function
within the ssip_pn_ops structure is capable of starting the
work.
If we remove the module which will call ssi_protocol_remove
to make a cleanup, it will free ssi through kfree(ssi),
while the work mentioned above will be used. The sequence
of operations that may lead to a UAF bug is as follows:
CPU0 CPU1
| ssip_xmit_work
ssi_protocol_remove |
kfree(ssi); |
| struct hsi_client *cl = ssi->cl;
| // use ssi
Fix it by ensuring that the work is canceled before proceeding
with the cleanup in ssi_protocol_remove
Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn>
---
drivers/hsi/clients/ssi_protocol.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hsi/clients/ssi_protocol.c b/drivers/hsi/clients/ssi_protocol.c
index afe470f3661c..3506c70e3505 100644
--- a/drivers/hsi/clients/ssi_protocol.c
+++ b/drivers/hsi/clients/ssi_protocol.c
@@ -1155,6 +1155,7 @@ static int ssi_protocol_remove(struct device *dev)
unregister_netdev(ssi->netdev);
ssip_free_cmds(ssi);
hsi_client_set_drvdata(cl, NULL);
+ cancel_work_sync(&ssi->work)
kfree(ssi);
return 0;
--
2.25.1Hi, On Wed, Sep 11, 2024 at 11:19:15PM GMT, Kaixin Wang wrote: > In the ssi_protocol_probe function, &ssi->work is bound with > ssip_xmit_work, In ssip_pn_setup, the ssip_pn_xmit function > within the ssip_pn_ops structure is capable of starting the > work. > > If we remove the module which will call ssi_protocol_remove > to make a cleanup, it will free ssi through kfree(ssi), > while the work mentioned above will be used. The sequence > of operations that may lead to a UAF bug is as follows: > > CPU0 CPU1 > > | ssip_xmit_work > ssi_protocol_remove | > kfree(ssi); | > | struct hsi_client *cl = ssi->cl; > | // use ssi > > Fix it by ensuring that the work is canceled before proceeding > with the cleanup in ssi_protocol_remove > > Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn> > --- This does not even compile :( During module removal the network device is unregistered (and thus stopped), which calls ssip_reset(), which should stop any activity regarding traffic exchange. That's the right place to cancel the work. Greetings, -- Sebastian > drivers/hsi/clients/ssi_protocol.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/hsi/clients/ssi_protocol.c b/drivers/hsi/clients/ssi_protocol.c > index afe470f3661c..3506c70e3505 100644 > --- a/drivers/hsi/clients/ssi_protocol.c > +++ b/drivers/hsi/clients/ssi_protocol.c > @@ -1155,6 +1155,7 @@ static int ssi_protocol_remove(struct device *dev) > unregister_netdev(ssi->netdev); > ssip_free_cmds(ssi); > hsi_client_set_drvdata(cl, NULL); > + cancel_work_sync(&ssi->work) > kfree(ssi); > > return 0; > -- > 2.25.1 > >
At 2024-09-14 15:20:07, "Sebastian Reichel" <sre@kernel.org> wrote: >Hi, > >On Wed, Sep 11, 2024 at 11:19:15PM GMT, Kaixin Wang wrote: >> In the ssi_protocol_probe function, &ssi->work is bound with >> ssip_xmit_work, In ssip_pn_setup, the ssip_pn_xmit function >> within the ssip_pn_ops structure is capable of starting the >> work. >> >> If we remove the module which will call ssi_protocol_remove >> to make a cleanup, it will free ssi through kfree(ssi), >> while the work mentioned above will be used. The sequence >> of operations that may lead to a UAF bug is as follows: >> >> CPU0 CPU1 >> >> | ssip_xmit_work >> ssi_protocol_remove | >> kfree(ssi); | >> | struct hsi_client *cl = ssi->cl; >> | // use ssi >> >> Fix it by ensuring that the work is canceled before proceeding >> with the cleanup in ssi_protocol_remove >> >> Signed-off-by: Kaixin Wang <kxwang23@m.fudan.edu.cn> >> --- > >This does not even compile :( > Sorry for my mistake. >During module removal the network device is unregistered (and thus >stopped), which calls ssip_reset(), which should stop any activity >regarding traffic exchange. That's the right place to cancel the >work. > >Greetings, > >-- Sebastian > Thanks. I will fix it in the next version. Best regards, Kaixin Wang >> drivers/hsi/clients/ssi_protocol.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/hsi/clients/ssi_protocol.c b/drivers/hsi/clients/ssi_protocol.c >> index afe470f3661c..3506c70e3505 100644 >> --- a/drivers/hsi/clients/ssi_protocol.c >> +++ b/drivers/hsi/clients/ssi_protocol.c >> @@ -1155,6 +1155,7 @@ static int ssi_protocol_remove(struct device *dev) >> unregister_netdev(ssi->netdev); >> ssip_free_cmds(ssi); >> hsi_client_set_drvdata(cl, NULL); >> + cancel_work_sync(&ssi->work) >> kfree(ssi); >> >> return 0; >> -- >> 2.25.1 >> >>
Hi Kaixin,
kernel test robot noticed the following build errors:
[auto build test ERROR on sre-hsi/for-next]
[also build test ERROR on linus/master v6.11-rc7 next-20240912]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Kaixin-Wang/HSI-ssi_protocol-Fix-use-after-free-vulnerability-in-ssi_protocol-Driver-Due-to-Race-Condition/20240911-232206
base: https://git.kernel.org/pub/scm/linux/kernel/git/sre/linux-hsi.git for-next
patch link: https://lore.kernel.org/r/20240911151915.844957-1-kxwang23%40m.fudan.edu.cn
patch subject: [PATCH] HSI: ssi_protocol: Fix use after free vulnerability in ssi_protocol Driver Due to Race Condition
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20240913/202409131457.oUtHawfD-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240913/202409131457.oUtHawfD-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409131457.oUtHawfD-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/hsi/clients/ssi_protocol.c: In function 'ssi_protocol_remove':
>> drivers/hsi/clients/ssi_protocol.c:1158:37: error: expected ';' before 'kfree'
1158 | cancel_work_sync(&ssi->work)
| ^
| ;
1159 | kfree(ssi);
| ~~~~~
vim +1158 drivers/hsi/clients/ssi_protocol.c
1148
1149 static int ssi_protocol_remove(struct device *dev)
1150 {
1151 struct hsi_client *cl = to_hsi_client(dev);
1152 struct ssi_protocol *ssi = hsi_client_drvdata(cl);
1153
1154 list_del(&ssi->link);
1155 unregister_netdev(ssi->netdev);
1156 ssip_free_cmds(ssi);
1157 hsi_client_set_drvdata(cl, NULL);
> 1158 cancel_work_sync(&ssi->work)
1159 kfree(ssi);
1160
1161 return 0;
1162 }
1163
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Wed, Sep 11, 2024 at 11:19:15PM +0800, Kaixin Wang wrote: > In the ssi_protocol_probe function, &ssi->work is bound with > ssip_xmit_work, In ssip_pn_setup, the ssip_pn_xmit function > within the ssip_pn_ops structure is capable of starting the > work. > > If we remove the module which will call ssi_protocol_remove > to make a cleanup, it will free ssi through kfree(ssi), > while the work mentioned above will be used. The sequence > of operations that may lead to a UAF bug is as follows: > > CPU0 CPU1 > > | ssip_xmit_work > ssi_protocol_remove | > kfree(ssi); | > | struct hsi_client *cl = ssi->cl; > | // use ssi > > Fix it by ensuring that the work is canceled before proceeding > with the cleanup in ssi_protocol_remove Sounds legit to me. But I have no time to review, FWIW, Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> I believe Sebastian will conduct a proper review before applying. -- With Best Regards, Andy Shevchenko
© 2016 - 2024 Red Hat, Inc.