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/[email protected]/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4
Link: https://lore.kernel.org/all/[email protected]/
CC: Julia Lawall <[email protected]>
CC: Nicolas Palix <[email protected]>
CC: Matthias Brugger <[email protected]>
CC: AngeloGioacchino Del Regno <[email protected]>
CC: Ingo Molnar <[email protected]>
CC: Peter Zijlstra <[email protected]>
CC: Juri Lelli <[email protected]>
CC: Vincent Guittot <[email protected]>
CC: Dietmar Eggemann <[email protected]>
CC: Steven Rostedt <[email protected]>
CC: Ben Segall <[email protected]>
CC: Mel Gorman <[email protected]>
CC: Daniel Bristot de Oliveira <[email protected]>
CC: Valentin Schneider <[email protected]>
Signed-off-by: Schspa Shi <[email protected]>
---
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
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/[email protected]/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4
> Link: https://lore.kernel.org/all/[email protected]/
>
> CC: Julia Lawall <[email protected]>
> CC: Nicolas Palix <[email protected]>
> CC: Matthias Brugger <[email protected]>
> CC: AngeloGioacchino Del Regno <[email protected]>
> CC: Ingo Molnar <[email protected]>
> CC: Peter Zijlstra <[email protected]>
> CC: Juri Lelli <[email protected]>
> CC: Vincent Guittot <[email protected]>
> CC: Dietmar Eggemann <[email protected]>
> CC: Steven Rostedt <[email protected]>
> CC: Ben Segall <[email protected]>
> CC: Mel Gorman <[email protected]>
> CC: Daniel Bristot de Oliveira <[email protected]>
> CC: Valentin Schneider <[email protected]>
>
> Signed-off-by: Schspa Shi <[email protected]>
> ---
> 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
>
On Mon, 27 Feb 2023 12:20:28 +0100
Peter Zijlstra <[email protected]> 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
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.
On Mon, 27 Feb 2023 16:43:59 +0100
Peter Zijlstra <[email protected]> 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
Peter Zijlstra <[email protected]> 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/[email protected]/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4
>> Link: https://lore.kernel.org/all/[email protected]/
>>
>> CC: Julia Lawall <[email protected]>
>> CC: Nicolas Palix <[email protected]>
>> CC: Matthias Brugger <[email protected]>
>> CC: AngeloGioacchino Del Regno <[email protected]>
>> CC: Ingo Molnar <[email protected]>
>> CC: Peter Zijlstra <[email protected]>
>> CC: Juri Lelli <[email protected]>
>> CC: Vincent Guittot <[email protected]>
>> CC: Dietmar Eggemann <[email protected]>
>> CC: Steven Rostedt <[email protected]>
>> CC: Ben Segall <[email protected]>
>> CC: Mel Gorman <[email protected]>
>> CC: Daniel Bristot de Oliveira <[email protected]>
>> CC: Valentin Schneider <[email protected]>
>>
>> Signed-off-by: Schspa Shi <[email protected]>
>> ---
>> 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
Steven Rostedt <[email protected]> writes:
> On Mon, 27 Feb 2023 16:43:59 +0100
> Peter Zijlstra <[email protected]> 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