Re: [RFC PATCH] cocci: cpi: add complete api check script

[Schspa Shi]

Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxx/T/#mf6a41a7009bb47af1b15adf2b7b355e495f609c4
>> Link: https://lore.kernel.org/all/7d1021f1-c88e-5a03-3b92-087f9be37491@xxxxxxxxxxxxxxxxxxx/
>> 
>> CC: Julia Lawall <Julia.Lawall@xxxxxxxx>
>> CC: Nicolas Palix <nicolas.palix@xxxxxxx>
>> CC: Matthias Brugger <matthias.bgg@xxxxxxxxx>
>> CC: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
>> CC: Ingo Molnar <mingo@xxxxxxxxxx>
>> CC: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>> CC: Juri Lelli <juri.lelli@xxxxxxxxxx>
>> CC: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>> CC: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
>> CC: Steven Rostedt <rostedt@xxxxxxxxxxx>
>> CC: Ben Segall <bsegall@xxxxxxxxxx>
>> CC: Mel Gorman <mgorman@xxxxxxx>
>> CC: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
>> CC: Valentin Schneider <vschneid@xxxxxxxxxx>
>> 
>> Signed-off-by: Schspa Shi <schspa@xxxxxxxxx>
>> ---
>>  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