* [cocci] [RFC PATCH] cocci: cpi: add complete api check script @ 2023-02-27 7:53 Schspa Shi 2023-02-27 11:20 ` Peter Zijlstra 2023-02-27 15:54 ` [cocci] [RFC PATCH] coccinelle: Add SmPL script for completion API check Markus Elfring 0 siblings, 2 replies; 8+ messages in thread From: Schspa Shi @ 2023-02-27 7:53 UTC (permalink / raw) To: linux-kernel, cocci, peterz, mcgrof Cc: Schspa Shi, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider When DECLARE_COMPLETION_ONSTACK was used, the user must to ensure the other process won't reference the completion variable on stack. For a killable/interruptiable version, we need extra code(add locks/use xchg) to ensure this. This patch provide a SmPL script to detect bad DECLARE_COMPLETION_ONSTACK(_MAP) API usage, but far from perfect. This is a common problem, and a lot of drivers have simpler problem. The fellowing is a list of problems find by this SmPL patch, due to the complex use of wait_for_complete* API, there will still be some false negatives and false positives. This RFC patch is mainly used to discuss improvement methods. If we introduce the wait_for_complete*_onstack API, it will be easier to modify these problems, and the patch rules of SmPL will be very easy. In the process of trying to write SmPL scripts, I strongly recommend introducing two onstack APIs to complete this operation. file:/Users/schspa/work/src/linux/drivers/infiniband/ulp/srpt/ib_srpt.c::2962 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/misc/tifm_7xx1.c::268 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/firmware/arm_scmi/driver.c::1001 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::595 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::491 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::538 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::645 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::3175 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2360 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2314 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2634 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1804 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1758 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::2034 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/net/wireless/marvell/mwl8k.c::2259 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/net/wireless/mediatek/mt7601u/mcu.c::317 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/net/wireless/ti/wlcore/main.c::6674 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/net/wwan/t7xx/t7xx_state_monitor.c::416 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::647 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::653 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/soc/qcom/rpmh.c::269 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/aic94xx/aic94xx_tmf.c::339 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_ctl.c::242 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1811 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2266 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1603 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2073 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1807 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1328 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/ibmvscsi/ibmvfc.c::2466 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::844 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::2334 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic7xxx_osm.c::2297 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/lpfc/lpfc_nvmet.c::2119 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/ipr.c::5153 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/scsi_error.c::1157 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_main.c::1215 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c::996 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c::867 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/isci/task.c::317 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::1844 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2310 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2086 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2579 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::6752 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::4074 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/thunderbolt/ctl.c::604 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i2c/busses/i2c-hisi.c::206 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/s390/cio/vfio_ccw_drv.c::71 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/slimbus/messaging.c::154 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::894 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::932 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ctrl.c::377 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/usb/core/devio.c::1142 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/usb/core/hcd.c::2229 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/spi/spi-hisi-sfc-v3xx.c::337 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/bluetooth/hci_bcm4377.c::955 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c::336 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c::278 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::360 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::312 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::238 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/ata/libata-core.c::1558 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::285 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::233 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::262 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/lib/kunit/try-catch.c::76 was suspected to return a variable on stack file:/Users/schspa/work/src/linux/sound/aoa/soundbus/i2sbus/pcm.c::264 was suspected to return a variable on stack To fix this, we can add introducing two new API for this. + +void complete_on_stack(struct completion **x) +{ + struct completion *comp = xchg(*x, NULL); + + if (comp) + complete(comp); +} +EXPORT_SYMBOL(complete_on_stack); + +int __sched wait_for_completion_state_on_stack(struct completion **x, + unsigned int state) +{ + struct completion *comp = *x; + int retval; + + retval = wait_for_completion_state(comp, state); + if (retval) { + if (xchg(*x, NULL)) + return retval; + + /* + * complete_on_stack will call complete shortly. + */ + wait_for_completion(comp); + } + + return retval; +} +EXPORT_SYMBOL(wait_for_completion_state_on_stack); Link: https://lore.kernel.org/all/20221115140233.21981-1-schspa@gmail.com/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4 Link: https://lore.kernel.org/all/7d1021f1-c88e-5a03-3b92-087f9be37491@I-love.SAKURA.ne.jp/ CC: Julia Lawall <Julia.Lawall@inria.fr> CC: Nicolas Palix <nicolas.palix@imag.fr> CC: Matthias Brugger <matthias.bgg@gmail.com> CC: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> CC: Ingo Molnar <mingo@redhat.com> CC: Peter Zijlstra <peterz@infradead.org> CC: Juri Lelli <juri.lelli@redhat.com> CC: Vincent Guittot <vincent.guittot@linaro.org> CC: Dietmar Eggemann <dietmar.eggemann@arm.com> CC: Steven Rostedt <rostedt@goodmis.org> CC: Ben Segall <bsegall@google.com> CC: Mel Gorman <mgorman@suse.de> CC: Daniel Bristot de Oliveira <bristot@redhat.com> CC: Valentin Schneider <vschneid@redhat.com> Signed-off-by: Schspa Shi <schspa@gmail.com> --- scripts/coccinelle/api/complete.cocci | 160 ++++++++++++++++++++++++++ 1 file changed, 160 insertions(+) create mode 100644 scripts/coccinelle/api/complete.cocci diff --git a/scripts/coccinelle/api/complete.cocci b/scripts/coccinelle/api/complete.cocci new file mode 100644 index 000000000000..d4cf32187180 --- /dev/null +++ b/scripts/coccinelle/api/complete.cocci @@ -0,0 +1,160 @@ +// SPDX-License-Identifier: GPL-2.0 +/// +// Copyright: (C) 2023 Schspa Shi. +// Confidence: High +virtual report + +@r1 exists@ +declarer name DECLARE_COMPLETION_ONSTACK; +declarer name DECLARE_COMPLETION_ONSTACK_MAP; +position p; +identifier done; +identifier func; +@@ + +func(...) { +... +( +DECLARE_COMPLETION_ONSTACK(done@p); +| +DECLARE_COMPLETION_ONSTACK_MAP(done@p, ...); +) +... +} + +@locked exists@ +identifier func=r1.func; +identifier done=r1.done; +position p1,p; +@@ + +func(...) { +... +( +mutex_lock@p1 +| +mutex_trylock@p1 +) + (...) +... when != mutex_unlock(...) +done@p +... +} + + +@elocked exists@ +identifier func=r1.func; +identifier done=r1.done; +position p1,p; +expression e; +@@ + +func(...) { +... +e = &done; +... +( +mutex_lock@p1 +| +mutex_trylock@p1 +) + (...) +... when != mutex_unlock(...) +e@p +... +} + + +@has_wait_for_completion exists@ +position p; +identifier done; +identifier func=r1.func; +identifier fb = { wait_for_completion, wait_for_completion_io}; +expression e; +@@ + +func(...) { +... +( +... +fb(&done@p); +... +| +e = &done; +... +fb(e@p); +) +... +} + +@has_while_wait exists@ +position p; +identifier done, ret; +identifier func=r1.func; +identifier fb =~ "wait_for_completion.*"; +expression e; +@@ + +func(...) { +... +while (...) { + ... + ret = fb(&done@p, e); + ... +} +... +} + +@has_while_wait2 exists@ +position p; +identifier done; +identifier func=r1.func; +expression fb =~ "wait_for_completion.*"; +@@ + +func(...) { +... +while (fb(&done@p, ...) == 0) { + ... +} +... +} + + +@r2 depends on (!has_wait_for_completion && !has_while_wait && !has_while_wait2) exists@ +declarer name DECLARE_COMPLETION_ONSTACK; +position p!={locked.p, elocked.p}; +identifier done=r1.done; +identifier func=r1.func; +expression e; +@@ + +func(...) { +... +( +wait_for_completion_interruptible(&done@p) +| +wait_for_completion_killable(&done@p) +| +wait_for_completion_timeout(&done@p, ...) +| +wait_for_completion_io_timeout(&done@p, ...) +| +wait_for_completion_interruptible_timeout(&done@p, ...) +| +wait_for_completion_killable_timeout(&done@p, ...) +| +try_wait_for_completion(&done@p) +| +wait_for_completion_timeout(e@p, ...) +) +... +} + + +@script:python depends on report@ +fp << r2.p; +@@ + +print('file:{:s}::{:s} was suspected to return a variable on stack'.format(fp[0].file, fp[0].line)) + -- 2.37.3 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 7:53 [cocci] [RFC PATCH] cocci: cpi: add complete api check script Schspa Shi @ 2023-02-27 11:20 ` Peter Zijlstra 2023-02-27 15:28 ` Steven Rostedt 2023-02-27 16:10 ` Schspa Shi 2023-02-27 15:54 ` [cocci] [RFC PATCH] coccinelle: Add SmPL script for completion API check Markus Elfring 1 sibling, 2 replies; 8+ messages in thread From: Peter Zijlstra @ 2023-02-27 11:20 UTC (permalink / raw) To: Schspa Shi Cc: linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel On Mon, Feb 27, 2023 at 03:53:47PM +0800, Schspa Shi wrote: > When DECLARE_COMPLETION_ONSTACK was used, the user must to ensure the other > process won't reference the completion variable on stack. For a > killable/interruptiable version, we need extra code(add locks/use xchg) to > ensure this. > > This patch provide a SmPL script to detect bad > DECLARE_COMPLETION_ONSTACK(_MAP) API usage, but far from perfect. Documentation/process/submitting-patches.rst:instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy But also, wth is SmPL, the actual thing included is a coccinelle script. > This is a common problem, and a lot of drivers have simpler problem. The > fellowing is a list of problems find by this SmPL patch, due to the complex > use of wait_for_complete* API, there will still be some false negatives and > false positives. This RFC patch is mainly used to discuss improvement > methods. If we introduce the wait_for_complete*_onstack API, it will be > easier to modify these problems, and the patch rules of SmPL will be very > easy. In the process of trying to write SmPL scripts, I strongly recommend > introducing two onstack APIs to complete this operation. > > file:/Users/schspa/work/src/linux/drivers/infiniband/ulp/srpt/ib_srpt.c::2962 was suspected to return a variable on stack What's with this retarded file path? Are you running on Windows or something daft like that? Please, make them relative to srctree. > file:/Users/schspa/work/src/linux/drivers/misc/tifm_7xx1.c::268 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/firmware/arm_scmi/driver.c::1001 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::595 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::491 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::538 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::645 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::3175 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2360 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2314 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2634 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1804 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1758 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::2034 was suspected to return a variable on stack These don't seem buggy, they take the whole DSI out -- which lives on stack too. > file:/Users/schspa/work/src/linux/drivers/net/wireless/marvell/mwl8k.c::2259 was suspected to return a variable on stack Heh, they seem to have the right idea but a buggy implementation. > file:/Users/schspa/work/src/linux/drivers/net/wireless/mediatek/mt7601u/mcu.c::317 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/net/wireless/ti/wlcore/main.c::6674 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/net/wwan/t7xx/t7xx_state_monitor.c::416 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::647 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::653 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/soc/qcom/rpmh.c::269 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/aic94xx/aic94xx_tmf.c::339 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_ctl.c::242 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1811 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2266 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1603 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2073 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1807 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1328 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/ibmvscsi/ibmvfc.c::2466 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::844 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::2334 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic7xxx_osm.c::2297 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/lpfc/lpfc_nvmet.c::2119 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/ipr.c::5153 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/scsi_error.c::1157 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_main.c::1215 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c::996 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c::867 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/isci/task.c::317 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::1844 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2310 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2086 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2579 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::6752 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::4074 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/thunderbolt/ctl.c::604 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i2c/busses/i2c-hisi.c::206 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/s390/cio/vfio_ccw_drv.c::71 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/slimbus/messaging.c::154 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::894 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::932 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ctrl.c::377 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/usb/core/devio.c::1142 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/usb/core/hcd.c::2229 was suspected to return a variable on stack These do usb_kill_urb() in the fail case. IIUC this avoids the UaF problem this script is trying to finger, no? > file:/Users/schspa/work/src/linux/drivers/spi/spi-hisi-sfc-v3xx.c::337 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/bluetooth/hci_bcm4377.c::955 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c::336 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c::278 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::360 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::312 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::238 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/ata/libata-core.c::1558 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::285 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::233 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::262 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/lib/kunit/try-catch.c::76 was suspected to return a variable on stack > file:/Users/schspa/work/src/linux/sound/aoa/soundbus/i2sbus/pcm.c::264 was suspected to return a variable on stack > > To fix this, we can add introducing two new API for this. > > + > +void complete_on_stack(struct completion **x) > +{ > + struct completion *comp = xchg(*x, NULL); > + > + if (comp) > + complete(comp); > +} > +EXPORT_SYMBOL(complete_on_stack); > + > +int __sched wait_for_completion_state_on_stack(struct completion **x, > + unsigned int state) > +{ > + struct completion *comp = *x; > + int retval; > + > + retval = wait_for_completion_state(comp, state); > + if (retval) { > + if (xchg(*x, NULL)) > + return retval; > + > + /* > + * complete_on_stack will call complete shortly. > + */ > + wait_for_completion(comp); > + } > + > + return retval; > +} > +EXPORT_SYMBOL(wait_for_completion_state_on_stack); So going by the 3 random samples above, only 1 would use this pattern. Does that mean you 'forgot' to audit all these results before proposing a fix? What does that mean for this script? > Link: https://lore.kernel.org/all/20221115140233.21981-1-schspa@gmail.com/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4 > Link: https://lore.kernel.org/all/7d1021f1-c88e-5a03-3b92-087f9be37491@I-love.SAKURA.ne.jp/ > > CC: Julia Lawall <Julia.Lawall@inria.fr> > CC: Nicolas Palix <nicolas.palix@imag.fr> > CC: Matthias Brugger <matthias.bgg@gmail.com> > CC: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > CC: Ingo Molnar <mingo@redhat.com> > CC: Peter Zijlstra <peterz@infradead.org> > CC: Juri Lelli <juri.lelli@redhat.com> > CC: Vincent Guittot <vincent.guittot@linaro.org> > CC: Dietmar Eggemann <dietmar.eggemann@arm.com> > CC: Steven Rostedt <rostedt@goodmis.org> > CC: Ben Segall <bsegall@google.com> > CC: Mel Gorman <mgorman@suse.de> > CC: Daniel Bristot de Oliveira <bristot@redhat.com> > CC: Valentin Schneider <vschneid@redhat.com> > > Signed-off-by: Schspa Shi <schspa@gmail.com> > --- > scripts/coccinelle/api/complete.cocci | 160 ++++++++++++++++++++++++++ > 1 file changed, 160 insertions(+) > create mode 100644 scripts/coccinelle/api/complete.cocci > > diff --git a/scripts/coccinelle/api/complete.cocci b/scripts/coccinelle/api/complete.cocci > new file mode 100644 > index 000000000000..d4cf32187180 > --- /dev/null > +++ b/scripts/coccinelle/api/complete.cocci > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/// > +// Copyright: (C) 2023 Schspa Shi. > +// Confidence: High I'm thinking that 'high' is somewhat premature, 2 out of 3 false positive rate does not inspire confidence. > +virtual report > + > +@r1 exists@ > +declarer name DECLARE_COMPLETION_ONSTACK; > +declarer name DECLARE_COMPLETION_ONSTACK_MAP; > +position p; > +identifier done; > +identifier func; > +@@ > + > +func(...) { > +... > +( > +DECLARE_COMPLETION_ONSTACK(done@p); > +| > +DECLARE_COMPLETION_ONSTACK_MAP(done@p, ...); > +) > +... > +} > + > +@locked exists@ > +identifier func=r1.func; > +identifier done=r1.done; > +position p1,p; > +@@ > + > +func(...) { > +... > +( > +mutex_lock@p1 > +| > +mutex_trylock@p1 > +) > + (...) > +... when != mutex_unlock(...) > +done@p > +... > +} > + > + > +@elocked exists@ > +identifier func=r1.func; > +identifier done=r1.done; > +position p1,p; > +expression e; > +@@ > + > +func(...) { > +... > +e = &done; > +... > +( > +mutex_lock@p1 > +| > +mutex_trylock@p1 > +) > + (...) > +... when != mutex_unlock(...) > +e@p > +... > +} > + > + > +@has_wait_for_completion exists@ > +position p; > +identifier done; > +identifier func=r1.func; > +identifier fb = { wait_for_completion, wait_for_completion_io}; > +expression e; > +@@ > + > +func(...) { > +... > +( > +... > +fb(&done@p); > +... > +| > +e = &done; > +... > +fb(e@p); > +) > +... > +} > + > +@has_while_wait exists@ > +position p; > +identifier done, ret; > +identifier func=r1.func; > +identifier fb =~ "wait_for_completion.*"; > +expression e; > +@@ > + > +func(...) { > +... > +while (...) { > + ... > + ret = fb(&done@p, e); > + ... > +} > +... > +} > + > +@has_while_wait2 exists@ > +position p; > +identifier done; > +identifier func=r1.func; > +expression fb =~ "wait_for_completion.*"; > +@@ > + > +func(...) { > +... > +while (fb(&done@p, ...) == 0) { > + ... > +} > +... > +} > + > + > +@r2 depends on (!has_wait_for_completion && !has_while_wait && !has_while_wait2) exists@ > +declarer name DECLARE_COMPLETION_ONSTACK; > +position p!={locked.p, elocked.p}; > +identifier done=r1.done; > +identifier func=r1.func; > +expression e; > +@@ > + > +func(...) { > +... > +( > +wait_for_completion_interruptible(&done@p) > +| > +wait_for_completion_killable(&done@p) > +| > +wait_for_completion_timeout(&done@p, ...) > +| > +wait_for_completion_io_timeout(&done@p, ...) > +| > +wait_for_completion_interruptible_timeout(&done@p, ...) > +| > +wait_for_completion_killable_timeout(&done@p, ...) > +| > +try_wait_for_completion(&done@p) > +| > +wait_for_completion_timeout(e@p, ...) > +) > +... > +} > + > + > +@script:python depends on report@ > +fp << r2.p; > +@@ > + > +print('file:{:s}::{:s} was suspected to return a variable on stack'.format(fp[0].file, fp[0].line)) > + > -- > 2.37.3 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 11:20 ` Peter Zijlstra @ 2023-02-27 15:28 ` Steven Rostedt 2023-02-27 15:43 ` Peter Zijlstra 2023-02-27 16:10 ` Schspa Shi 1 sibling, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2023-02-27 15:28 UTC (permalink / raw) To: Peter Zijlstra Cc: Schspa Shi, linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel On Mon, 27 Feb 2023 12:20:28 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Feb 27, 2023 at 03:53:47PM +0800, Schspa Shi wrote: > > When DECLARE_COMPLETION_ONSTACK was used, the user must to ensure the other > > process won't reference the completion variable on stack. For a > > killable/interruptiable version, we need extra code(add locks/use xchg) to > > ensure this. > > > > This patch provide a SmPL script to detect bad > > DECLARE_COMPLETION_ONSTACK(_MAP) API usage, but far from perfect. > > Documentation/process/submitting-patches.rst:instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > > But also, wth is SmPL, the actual thing included is a coccinelle script. > > > This is a common problem, and a lot of drivers have simpler problem. The > > fellowing is a list of problems find by this SmPL patch, due to the complex > > use of wait_for_complete* API, there will still be some false negatives and > > false positives. This RFC patch is mainly used to discuss improvement > > methods. If we introduce the wait_for_complete*_onstack API, it will be > > easier to modify these problems, and the patch rules of SmPL will be very > > easy. In the process of trying to write SmPL scripts, I strongly recommend > > introducing two onstack APIs to complete this operation. > > > > file:/Users/schspa/work/src/linux/drivers/infiniband/ulp/srpt/ib_srpt.c::2962 was suspected to return a variable on stack > > What's with this retarded file path? Are you running on Windows or > something daft like that? > > Please, make them relative to srctree. Also, you want to state what sha1 or tag you ran this on. The one file I looked at didn't match line numbers. I looked at: drivers/ufs/core/ufshcd.c Which has (what I'm assuming is the issue that was detected?) /* wait until the task management command is completed */ err = wait_for_completion_io_timeout(&wait, msecs_to_jiffies(TM_CMD_TIMEOUT)); if (!err) { ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_ERR); dev_err(hba->dev, "%s: task management cmd 0x%.2x timed-out\n", __func__, tm_function); if (ufshcd_clear_tm_cmd(hba, task_tag)) dev_WARN(hba->dev, "%s: unable to clear tm cmd (slot %d) after timeout\n", __func__, task_tag); err = -ETIMEDOUT; } else { err = 0; memcpy(treq, hba->utmrdl_base_addr + task_tag, sizeof(*treq)); ufshcd_add_tm_upiu_trace(hba, task_tag, UFS_TM_COMP); } spin_lock_irqsave(hba->host->host_lock, flags); hba->tmf_rqs[req->tag] = NULL; __clear_bit(task_tag, &hba->outstanding_tasks); spin_unlock_irqrestore(hba->host->host_lock, flags); IIUC, the above spin lock protection will prevent the use after free of the completion. As the completion still exists between the timedout wait, and the start of the spinlock. And the spinlock will keep the complete from being visible if that were to run after the spinlock. So what exact race are you trying to catch here? -- Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 15:28 ` Steven Rostedt @ 2023-02-27 15:43 ` Peter Zijlstra 2023-02-27 15:53 ` Steven Rostedt 0 siblings, 1 reply; 8+ messages in thread From: Peter Zijlstra @ 2023-02-27 15:43 UTC (permalink / raw) To: Steven Rostedt Cc: Schspa Shi, linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel On Mon, Feb 27, 2023 at 10:28:08AM -0500, Steven Rostedt wrote: > So what exact race are you trying to catch here? on-stack copmletion with a wait_for_completion that can return early (eg. killable, interruptible, or timeout) can go out of scope (eg, free the completion) with the other side calling complete() on some possibly re-used piece of stack. IOW, Use-after-Free. Care must be taken to ensure the other side (whatever does complete()) is either terminated or otherwise stopped from calling complete() on an out-of-scope variable. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 15:43 ` Peter Zijlstra @ 2023-02-27 15:53 ` Steven Rostedt 2023-02-27 16:36 ` Schspa Shi 0 siblings, 1 reply; 8+ messages in thread From: Steven Rostedt @ 2023-02-27 15:53 UTC (permalink / raw) To: Peter Zijlstra Cc: Schspa Shi, linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel On Mon, 27 Feb 2023 16:43:59 +0100 Peter Zijlstra <peterz@infradead.org> wrote: > On Mon, Feb 27, 2023 at 10:28:08AM -0500, Steven Rostedt wrote: > > > So what exact race are you trying to catch here? > > on-stack copmletion with a wait_for_completion that can return early > (eg. killable, interruptible, or timeout) can go out of scope (eg, free > the completion) with the other side calling complete() on some possibly > re-used piece of stack. > > IOW, Use-after-Free. > > Care must be taken to ensure the other side (whatever does complete()) > is either terminated or otherwise stopped from calling complete() on an > out-of-scope variable. I got that. But as you were stating as well, when care is taken, the script appears to still report it. The example I gave has: req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0); [..] req->end_io_data = &wait; [..] hba->tmf_rqs[req->tag] = req; [..] err = wait_for_completion_io_timeout(&wait, [..] spin_lock_irqsave(hba->host->host_lock, flags); hba->tmf_rqs[req->tag] = NULL; __clear_bit(task_tag, &hba->outstanding_tasks); spin_unlock_irqrestore(hba->host->host_lock, flags); And where the complete is: spin_lock_irqsave(hba->host->host_lock, flags); pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL); issued = hba->outstanding_tasks & ~pending; for_each_set_bit(tag, &issued, hba->nutmrs) { struct request *req = hba->tmf_rqs[tag]; struct completion *c = req->end_io_data; complete(c); ret = IRQ_HANDLED; } spin_unlock_irqrestore(hba->host->host_lock, flags); So the spinlock is making sure that the complete() only works on a completion if it is still there. I guess I should have asked, how is this script differentiating between where there's a problem and where there isn't. If you remove the spinlocks, then there would most definitely be a race, and I'm not even sure if the supplied patch would improve this much. -- Steve ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 15:53 ` Steven Rostedt @ 2023-02-27 16:36 ` Schspa Shi 0 siblings, 0 replies; 8+ messages in thread From: Schspa Shi @ 2023-02-27 16:36 UTC (permalink / raw) To: Steven Rostedt Cc: Peter Zijlstra, linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel Steven Rostedt <rostedt@goodmis.org> writes: > On Mon, 27 Feb 2023 16:43:59 +0100 > Peter Zijlstra <peterz@infradead.org> wrote: > >> On Mon, Feb 27, 2023 at 10:28:08AM -0500, Steven Rostedt wrote: >> >> > So what exact race are you trying to catch here? >> >> on-stack copmletion with a wait_for_completion that can return early >> (eg. killable, interruptible, or timeout) can go out of scope (eg, free >> the completion) with the other side calling complete() on some possibly >> re-used piece of stack. >> >> IOW, Use-after-Free. >> >> Care must be taken to ensure the other side (whatever does complete()) >> is either terminated or otherwise stopped from calling complete() on an >> out-of-scope variable. > > I got that. But as you were stating as well, when care is taken, the script > appears to still report it. The example I gave has: > > req = blk_mq_alloc_request(q, REQ_OP_DRV_OUT, 0); > [..] > req->end_io_data = &wait; > [..] > hba->tmf_rqs[req->tag] = req; > [..] > err = wait_for_completion_io_timeout(&wait, > [..] > spin_lock_irqsave(hba->host->host_lock, flags); > hba->tmf_rqs[req->tag] = NULL; > __clear_bit(task_tag, &hba->outstanding_tasks); > spin_unlock_irqrestore(hba->host->host_lock, flags); > > > And where the complete is: > > spin_lock_irqsave(hba->host->host_lock, flags); > pending = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL); > issued = hba->outstanding_tasks & ~pending; > for_each_set_bit(tag, &issued, hba->nutmrs) { > struct request *req = hba->tmf_rqs[tag]; > struct completion *c = req->end_io_data; > > complete(c); > ret = IRQ_HANDLED; > } > spin_unlock_irqrestore(hba->host->host_lock, flags); > > So the spinlock is making sure that the complete() only works on a > completion if it is still there. > There is nothing wrong with your code. This script will not check the hba->host->host_lock lock, and there is another hba->outstanding_tasks bit mask to ensure that there is no UAF here. But this script doesn't have a way to get these implicit conditions. > I guess I should have asked, how is this script differentiating between > where there's a problem and where there isn't. > > If you remove the spinlocks, then there would most definitely be a race, > and I'm not even sure if the supplied patch would improve this much. > > -- Steve -- BRs Schspa Shi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] cocci: cpi: add complete api check script 2023-02-27 11:20 ` Peter Zijlstra 2023-02-27 15:28 ` Steven Rostedt @ 2023-02-27 16:10 ` Schspa Shi 1 sibling, 0 replies; 8+ messages in thread From: Schspa Shi @ 2023-02-27 16:10 UTC (permalink / raw) To: Peter Zijlstra Cc: linux-kernel, cocci, mcgrof, Julia Lawall, Nicolas Palix, Matthias Brugger, AngeloGioacchino Del Regno, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider, buytenh, johannes.berg, gregkh, tomba, airlied, daniel Peter Zijlstra <peterz@infradead.org> writes: > On Mon, Feb 27, 2023 at 03:53:47PM +0800, Schspa Shi wrote: >> When DECLARE_COMPLETION_ONSTACK was used, the user must to ensure the other >> process won't reference the completion variable on stack. For a >> killable/interruptiable version, we need extra code(add locks/use xchg) to >> ensure this. >> >> This patch provide a SmPL script to detect bad >> DECLARE_COMPLETION_ONSTACK(_MAP) API usage, but far from perfect. > > Documentation/process/submitting-patches.rst:instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy > > But also, wth is SmPL, the actual thing included is a coccinelle script. > Thanks for reminding. >> This is a common problem, and a lot of drivers have simpler problem. The >> fellowing is a list of problems find by this SmPL patch, due to the complex >> use of wait_for_complete* API, there will still be some false negatives and >> false positives. This RFC patch is mainly used to discuss improvement There are still many defects in this script that are commented here. >> methods. If we introduce the wait_for_complete*_onstack API, it will be >> easier to modify these problems, and the patch rules of SmPL will be very >> easy. In the process of trying to write SmPL scripts, I strongly recommend >> introducing two onstack APIs to complete this operation. >> >> file:/Users/schspa/work/src/linux/drivers/infiniband/ulp/srpt/ib_srpt.c::2962 was suspected to return a variable on stack > > What's with this retarded file path? Are you running on Windows or > something daft like that? > > Please, make them relative to srctree. It's the raw output from this coccinelle script running from macOS. So, I did not remove the prefix from the file path. > >> file:/Users/schspa/work/src/linux/drivers/misc/tifm_7xx1.c::268 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/firmware/arm_scmi/driver.c::1001 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::595 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::491 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::538 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dispc-compat.c::645 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::3175 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2360 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2314 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/video/fbdev/omap2/omapfb/dss/dsi.c::2634 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1804 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::1758 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/gpu/drm/omapdrm/dss/dsi.c::2034 was suspected to return a variable on stack > > These don't seem buggy, they take the whole DSI out -- which lives on > stack too. > This RFC patch is only used to illustrate the complexity of this detection. As rewritten in the comments, this coccinelle script is already somewhat complicated, but it still cannot avoid false positives and false negatives. For code writing like dsi, I think it is difficult to cover it with coccinelle script. So I want to introduce the onstack version of the API, so that the checking work will be much easier. It is also much easier to read the source code. When you see such APIs when reviewing the code, you can easily know that there are detailed exception mirroring considerations here, instead of carefully reviewing whether there are some implicit conditions to ensure that there are no problems. >> file:/Users/schspa/work/src/linux/drivers/net/wireless/marvell/mwl8k.c::2259 was suspected to return a variable on stack > > Heh, they seem to have the right idea but a buggy implementation. > >> file:/Users/schspa/work/src/linux/drivers/net/wireless/mediatek/mt7601u/mcu.c::317 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/net/wireless/ti/wlcore/main.c::6674 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/net/wwan/t7xx/t7xx_state_monitor.c::416 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::647 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/soc/apple/rtkit.c::653 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/soc/qcom/rpmh.c::269 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/aic94xx/aic94xx_tmf.c::339 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_ctl.c::242 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1811 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2266 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::1603 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/snic/snic_scsi.c::2073 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1807 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/qla2xxx/qla_os.c::1328 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/ibmvscsi/ibmvfc.c::2466 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::844 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic79xx_osm.c::2334 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/aic7xxx/aic7xxx_osm.c::2297 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/lpfc/lpfc_nvmet.c::2119 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/ipr.c::5153 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/scsi_error.c::1157 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_main.c::1215 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c::996 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c::867 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/isci/task.c::317 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::1844 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2310 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2086 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/scsi/fnic/fnic_scsi.c::2579 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::6752 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/ufs/core/ufshcd.c::4074 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/thunderbolt/ctl.c::604 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i2c/busses/i2c-hisi.c::206 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/s390/cio/vfio_ccw_drv.c::71 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/slimbus/messaging.c::154 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::894 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ngd-ctrl.c::932 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/slimbus/qcom-ctrl.c::377 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/usb/core/devio.c::1142 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/usb/core/hcd.c::2229 was suspected to return a variable on stack > > These do usb_kill_urb() in the fail case. IIUC this avoids the UaF > problem this script is trying to finger, no? > This is a similar usage to DSI, with some implied conditions >> file:/Users/schspa/work/src/linux/drivers/spi/spi-hisi-sfc-v3xx.c::337 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/bluetooth/hci_bcm4377.c::955 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v1.c::336 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/cmd_v2.c::278 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::360 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::312 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/i3c/master/mipi-i3c-hci/core.c::238 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/ata/libata-core.c::1558 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::285 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::233 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/drivers/w1/masters/ds1wm.c::262 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/lib/kunit/try-catch.c::76 was suspected to return a variable on stack >> file:/Users/schspa/work/src/linux/sound/aoa/soundbus/i2sbus/pcm.c::264 was suspected to return a variable on stack >> >> To fix this, we can add introducing two new API for this. >> >> + >> +void complete_on_stack(struct completion **x) >> +{ >> + struct completion *comp = xchg(*x, NULL); >> + >> + if (comp) >> + complete(comp); >> +} >> +EXPORT_SYMBOL(complete_on_stack); >> + >> +int __sched wait_for_completion_state_on_stack(struct completion **x, >> + unsigned int state) >> +{ >> + struct completion *comp = *x; >> + int retval; >> + >> + retval = wait_for_completion_state(comp, state); >> + if (retval) { >> + if (xchg(*x, NULL)) >> + return retval; >> + >> + /* >> + * complete_on_stack will call complete shortly. >> + */ >> + wait_for_completion(comp); >> + } >> + >> + return retval; >> +} >> +EXPORT_SYMBOL(wait_for_completion_state_on_stack); > > So going by the 3 random samples above, only 1 would use this pattern. > > Does that mean you 'forgot' to audit all these results before proposing > a fix? > I checked the output here, and there are instances of incorrect error alerts in this output already pointed out in the previous comments. > What does that mean for this script? > This RFC PATCH is intended to be used to discuss whether to add a new API to simplify this detection and repair work. >> Link: https://lore.kernel.org/all/20221115140233.21981-1-schspa@gmail.com/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4 >> Link: https://lore.kernel.org/all/7d1021f1-c88e-5a03-3b92-087f9be37491@I-love.SAKURA.ne.jp/ >> >> CC: Julia Lawall <Julia.Lawall@inria.fr> >> CC: Nicolas Palix <nicolas.palix@imag.fr> >> CC: Matthias Brugger <matthias.bgg@gmail.com> >> CC: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> CC: Ingo Molnar <mingo@redhat.com> >> CC: Peter Zijlstra <peterz@infradead.org> >> CC: Juri Lelli <juri.lelli@redhat.com> >> CC: Vincent Guittot <vincent.guittot@linaro.org> >> CC: Dietmar Eggemann <dietmar.eggemann@arm.com> >> CC: Steven Rostedt <rostedt@goodmis.org> >> CC: Ben Segall <bsegall@google.com> >> CC: Mel Gorman <mgorman@suse.de> >> CC: Daniel Bristot de Oliveira <bristot@redhat.com> >> CC: Valentin Schneider <vschneid@redhat.com> >> >> Signed-off-by: Schspa Shi <schspa@gmail.com> >> --- >> scripts/coccinelle/api/complete.cocci | 160 ++++++++++++++++++++++++++ >> 1 file changed, 160 insertions(+) >> create mode 100644 scripts/coccinelle/api/complete.cocci >> >> diff --git a/scripts/coccinelle/api/complete.cocci b/scripts/coccinelle/api/complete.cocci >> new file mode 100644 >> index 000000000000..d4cf32187180 >> --- /dev/null >> +++ b/scripts/coccinelle/api/complete.cocci >> @@ -0,0 +1,160 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/// >> +// Copyright: (C) 2023 Schspa Shi. >> +// Confidence: High > > I'm thinking that 'high' is somewhat premature, 2 out of 3 false > positive rate does not inspire confidence. > OK, this definition is not appropriate. >> +virtual report >> + >> +@r1 exists@ >> +declarer name DECLARE_COMPLETION_ONSTACK; >> +declarer name DECLARE_COMPLETION_ONSTACK_MAP; >> +position p; >> +identifier done; >> +identifier func; >> +@@ >> + >> +func(...) { >> +... >> +( >> +DECLARE_COMPLETION_ONSTACK(done@p); >> +| >> +DECLARE_COMPLETION_ONSTACK_MAP(done@p, ...); >> +) >> +... >> +} >> + >> +@locked exists@ >> +identifier func=r1.func; >> +identifier done=r1.done; >> +position p1,p; >> +@@ >> + >> +func(...) { >> +... >> +( >> +mutex_lock@p1 >> +| >> +mutex_trylock@p1 >> +) >> + (...) >> +... when != mutex_unlock(...) >> +done@p >> +... >> +} >> + >> + >> +@elocked exists@ >> +identifier func=r1.func; >> +identifier done=r1.done; >> +position p1,p; >> +expression e; >> +@@ >> + >> +func(...) { >> +... >> +e = &done; >> +... >> +( >> +mutex_lock@p1 >> +| >> +mutex_trylock@p1 >> +) >> + (...) >> +... when != mutex_unlock(...) >> +e@p >> +... >> +} >> + >> + >> +@has_wait_for_completion exists@ >> +position p; >> +identifier done; >> +identifier func=r1.func; >> +identifier fb = { wait_for_completion, wait_for_completion_io}; >> +expression e; >> +@@ >> + >> +func(...) { >> +... >> +( >> +... >> +fb(&done@p); >> +... >> +| >> +e = &done; >> +... >> +fb(e@p); >> +) >> +... >> +} >> + >> +@has_while_wait exists@ >> +position p; >> +identifier done, ret; >> +identifier func=r1.func; >> +identifier fb =~ "wait_for_completion.*"; >> +expression e; >> +@@ >> + >> +func(...) { >> +... >> +while (...) { >> + ... >> + ret = fb(&done@p, e); >> + ... >> +} >> +... >> +} >> + >> +@has_while_wait2 exists@ >> +position p; >> +identifier done; >> +identifier func=r1.func; >> +expression fb =~ "wait_for_completion.*"; >> +@@ >> + >> +func(...) { >> +... >> +while (fb(&done@p, ...) == 0) { >> + ... >> +} >> +... >> +} >> + >> + >> +@r2 depends on (!has_wait_for_completion && !has_while_wait && !has_while_wait2) exists@ >> +declarer name DECLARE_COMPLETION_ONSTACK; >> +position p!={locked.p, elocked.p}; >> +identifier done=r1.done; >> +identifier func=r1.func; >> +expression e; >> +@@ >> + >> +func(...) { >> +... >> +( >> +wait_for_completion_interruptible(&done@p) >> +| >> +wait_for_completion_killable(&done@p) >> +| >> +wait_for_completion_timeout(&done@p, ...) >> +| >> +wait_for_completion_io_timeout(&done@p, ...) >> +| >> +wait_for_completion_interruptible_timeout(&done@p, ...) >> +| >> +wait_for_completion_killable_timeout(&done@p, ...) >> +| >> +try_wait_for_completion(&done@p) >> +| >> +wait_for_completion_timeout(e@p, ...) >> +) >> +... >> +} >> + >> + >> +@script:python depends on report@ >> +fp << r2.p; >> +@@ >> + >> +print('file:{:s}::{:s} was suspected to return a variable on stack'.format(fp[0].file, fp[0].line)) >> + >> -- >> 2.37.3 >> -- BRs Schspa Shi ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [cocci] [RFC PATCH] coccinelle: Add SmPL script for completion API check 2023-02-27 7:53 [cocci] [RFC PATCH] cocci: cpi: add complete api check script Schspa Shi 2023-02-27 11:20 ` Peter Zijlstra @ 2023-02-27 15:54 ` Markus Elfring 1 sibling, 0 replies; 8+ messages in thread From: Markus Elfring @ 2023-02-27 15:54 UTC (permalink / raw) To: Schspa Shi, Julia Lawall, Peter Zijlstra, Luis Chamberlain, cocci Cc: LKML, kernel-janitors, Angelo Gioacchino Del Regno, Ben Segall, Daniel Bristot de Oliveira, Dietmar Eggemann, Ingo Molnar, Juri Lelli, Matthias Brugger, Mel Gorman, Nicolas Palix, Steven Rostedt, Valentin Schneider … > This patch provide a SmPL script to detect bad > DECLARE_COMPLETION_ONSTACK(_MAP) API usage, but far from perfect. … > +++ b/scripts/coccinelle/api/complete.cocci > @@ -0,0 +1,160 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/// > +// Copyright: (C) 2023 Schspa Shi. > +// Confidence: High Would it be helpful to add the field “Keywords”? > +@r1 exists@ > +declarer name DECLARE_COMPLETION_ONSTACK; > +declarer name DECLARE_COMPLETION_ONSTACK_MAP; > +position p; > +identifier done; > +identifier func; > +@@ > + > +func(...) { > +... > +( > +DECLARE_COMPLETION_ONSTACK(done@p); > +| > +DECLARE_COMPLETION_ONSTACK_MAP(done@p, ...); > +) > +... > +} > + > +@locked exists@ > +identifier func=r1.func; > +identifier done=r1.done; > +position p1,p; > +@@ > + > +func(...) { > +... > +( > +mutex_lock@p1 > +| > +mutex_trylock@p1 > +) > + (...) > +... when != mutex_unlock(...) > +done@p > +... > +} > + > + > +@elocked exists@ > +identifier func=r1.func; > +identifier done=r1.done; > +position p1,p; > +expression e; > +@@ > + > +func(...) { > +... > +e = &done; > +... > +( > +mutex_lock@p1 > +| > +mutex_trylock@p1 > +) > + (...) > +... when != mutex_unlock(...) > +e@p > +... > +} > + > + > +@has_wait_for_completion exists@ > +position p; > +identifier done; > +identifier func=r1.func; > +identifier fb = { wait_for_completion, wait_for_completion_io}; > +expression e; > +@@ > + > +func(...) { > +... > +( > +... > +fb(&done@p); > +... > +| > +e = &done; > +... > +fb(e@p); > +) > +... > +} I would prefer the following SmPL code variant. @r1 exists@ declarer name DECLARE_COMPLETION_ONSTACK, DECLARE_COMPLETION_ONSTACK_MAP; identifier done, func; position p; @@ func(...) { ... (DECLARE_COMPLETION_ONSTACK(done@p) |DECLARE_COMPLETION_ONSTACK_MAP(done@p, ...) ); ... } @locked exists@ identifier r1.func, r1.done; position p1, p; @@ func(...) { ... (mutex_lock@p1 |mutex_trylock@p1 )(...) ... when != mutex_unlock(...) done@p ... } @elocked exists@ expression e; identifier r1.func, r1.done; position p1, p; @@ func(...) { ... e = &done; ... (mutex_lock@p1 |mutex_trylock@p1 )(...) ... when != mutex_unlock(...) e@p ... } @has_wait_for_completion exists@ expression e; identifier done, r1.func, fb = {wait_for_completion, wait_for_completion_io}; position p; @@ func(...) { ... ( fb(&done@p); ... | e = &done; ... fb(e@p); ) ... } > +@has_while_wait exists@ > +position p; > +identifier done, ret; > +identifier func=r1.func; > +identifier fb =~ "wait_for_completion.*"; * The metavariables can be grouped once more. * I suggest to restrict the regular expression a bit more by using an anchor. identifier done, ret, r1.func, fb =~ "^wait_for_completion.*"; > +expression e; > +@@ > + > +func(...) { > +... > +while (...) { > + ... > + ret = fb(&done@p, e); > + ... > +} > +... > +} > + > +@has_while_wait2 exists@ > +position p; > +identifier done; > +identifier func=r1.func; > +expression fb =~ "wait_for_completion.*"; > +@@ > + > +func(...) { > +... > +while (fb(&done@p, ...) == 0) { > + ... > +} > +... > +} > + > + > +@r2 depends on (!has_wait_for_completion && !has_while_wait && !has_while_wait2) exists@ Would the dependency specification “!(has_wait_for_completion || has_while_wait || has_while_wait2)” be nicer? > +func(...) { > +... > +( > +wait_for_completion_interruptible(&done@p) > +| > +wait_for_completion_killable(&done@p) > +| > +wait_for_completion_timeout(&done@p, ...) > +| > +wait_for_completion_io_timeout(&done@p, ...) > +| > +wait_for_completion_interruptible_timeout(&done@p, ...) > +| > +wait_for_completion_killable_timeout(&done@p, ...) > +| > +try_wait_for_completion(&done@p) > +| > +wait_for_completion_timeout(e@p, ...) > +) > +... > +} Were the function names (in this SmPL disjunction) ordered according to their call probability? > +@script:python depends on report@ > +fp << r2.p; > +@@ > + > +print('file:{:s}::{:s} was suspected to return a variable on stack'.format(fp[0].file, fp[0].line)) Would the following statement variant be more appropriate? coccilib.report.print_report(fp[0], "WARNING: It seems that a reference was returned to a variable which was stored on a stack.") Regards, Markus ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-03-04 16:40 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-02-27 7:53 [cocci] [RFC PATCH] cocci: cpi: add complete api check script Schspa Shi 2023-02-27 11:20 ` Peter Zijlstra 2023-02-27 15:28 ` Steven Rostedt 2023-02-27 15:43 ` Peter Zijlstra 2023-02-27 15:53 ` Steven Rostedt 2023-02-27 16:36 ` Schspa Shi 2023-02-27 16:10 ` Schspa Shi 2023-02-27 15:54 ` [cocci] [RFC PATCH] coccinelle: Add SmPL script for completion API check Markus Elfring
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).