* [PATCH] coccicheck: add a test for repeat copy_from_user @ 2016-04-26 22:24 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-04-26 22:24 UTC (permalink / raw) To: Julia Lawall Cc: linux-kernel, Gilles Muller, Nicolas Palix, Michal Marek, Pengfei Wang, cocci This is usually a sign of a resized request. This adds a check for potential races or confusions. The check isn't 100% accurate, so it needs some manual review. Signed-off-by: Kees Cook <keescook@chromium.org> --- scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 scripts/coccinelle/tests/reusercopy.cocci diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci new file mode 100644 index 000000000000..53645de8ae95 --- /dev/null +++ b/scripts/coccinelle/tests/reusercopy.cocci @@ -0,0 +1,36 @@ +/// Recopying from the same user buffer frequently indicates a pattern of +/// Reading a size header, allocating, and then re-reading an entire +/// structure. If the structure's size is not re-validated, this can lead +/// to structure or data size confusions. +/// +// Confidence: Moderate +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Comments: +// Options: -no_includes -include_headers + +virtual report +virtual org + +@cfu_twice@ +position p; +identifier src; +expression dest1, dest2, size1, size2, offset; +@@ + +*copy_from_user(dest1, src, size1) + ... when != src = offset + when != src += offset +*copy_from_user@p(dest2, src, size2) + +@script:python depends on org@ +p << cfu_twice.p; +@@ + +cocci.print_main("potentially dangerous second copy_from_user()",p) + +@script:python depends on report@ +p << cfu_twice.p; +@@ + +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") -- 2.6.3 -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply related [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2016-04-26 22:24 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-04-26 22:24 UTC (permalink / raw) To: cocci This is usually a sign of a resized request. This adds a check for potential races or confusions. The check isn't 100% accurate, so it needs some manual review. Signed-off-by: Kees Cook <keescook@chromium.org> --- scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 scripts/coccinelle/tests/reusercopy.cocci diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci new file mode 100644 index 000000000000..53645de8ae95 --- /dev/null +++ b/scripts/coccinelle/tests/reusercopy.cocci @@ -0,0 +1,36 @@ +/// Recopying from the same user buffer frequently indicates a pattern of +/// Reading a size header, allocating, and then re-reading an entire +/// structure. If the structure's size is not re-validated, this can lead +/// to structure or data size confusions. +/// +// Confidence: Moderate +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Comments: +// Options: -no_includes -include_headers + +virtual report +virtual org + + at cfu_twice@ +position p; +identifier src; +expression dest1, dest2, size1, size2, offset; +@@ + +*copy_from_user(dest1, src, size1) + ... when != src = offset + when != src += offset +*copy_from_user at p(dest2, src, size2) + + at script:python depends on org@ +p << cfu_twice.p; +@@ + +cocci.print_main("potentially dangerous second copy_from_user()",p) + +@script:python depends on report@ +p << cfu_twice.p; +@@ + +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") -- 2.6.3 -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH] coccicheck: add a test for repeat copy_from_user 2016-04-26 22:24 ` [Cocci] " Kees Cook @ 2016-04-26 22:30 ` Kees Cook -1 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-04-26 22:30 UTC (permalink / raw) To: Julia Lawall Cc: LKML, Gilles Muller, Nicolas Palix, Michal Marek, Pengfei Wang, cocci On Tue, Apr 26, 2016 at 3:24 PM, Kees Cook <keescook@chromium.org> wrote: > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers > + > +virtual report > +virtual org > + > +@cfu_twice@ > +position p; > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset > +*copy_from_user@p(dest2, src, size2) > + > +@script:python depends on org@ > +p << cfu_twice.p; > +@@ > + > +cocci.print_main("potentially dangerous second copy_from_user()",p) > + > +@script:python depends on report@ > +p << cfu_twice.p; > +@@ > + > +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") > -- > 2.6.3 > > > -- > Kees Cook > Chrome OS & Brillo Security And here's the current (noisy) output: ./arch/powerpc/platforms/powernv/opal-prd.c:248:6-20: potentially dangerous second copy_from_user() ./sound/isa/sb/sb16_csp.c:391:7-21: potentially dangerous second copy_from_user() ./drivers/gpu/drm/tegra/drm.c:361:6-20: potentially dangerous second copy_from_user() ./fs/exec.c:537:7-21: potentially dangerous second copy_from_user() ./drivers/char/lp.c:387:7-21: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2149:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2247:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2292:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2332:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2355:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2396:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2429:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2481:6-20: potentially dangerous second copy_from_user() ./arch/ia64/kernel/perfmon.c:4833:11-25: potentially dangerous second copy_from_user() ./drivers/staging/i4l/icn/icn.c:1048:7-21: potentially dangerous second copy_from_user() ./drivers/misc/mic/vop/vop_vringh.c:775:9-23: potentially dangerous second copy_from_user() ./drivers/misc/mic/vop/vop_vringh.c:944:6-20: potentially dangerous second copy_from_user() ./fs/coda/psdev.c:128:6-20: potentially dangerous second copy_from_user() ./fs/coda/psdev.c:174:12-26: potentially dangerous second copy_from_user() ./drivers/hid/hid-picolcd_debugfs.c:283:6-20: potentially dangerous second copy_from_user() ./fs/aio.c:1610:15-29: potentially dangerous second copy_from_user() ./fs/splice.c:1459:6-20: potentially dangerous second copy_from_user() ./kernel/kexec_core.c:815:12-26: potentially dangerous second copy_from_user() ./kernel/kexec_core.c:752:12-26: potentially dangerous second copy_from_user() ./drivers/scsi/3w-9xxx.c:691:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/3w-xxxx.c:921:5-19: potentially dangerous second copy_from_user() ./drivers/platform/chrome/cros_ec_dev.c:145:5-19: potentially dangerous second copy_from_user() ./sound/drivers/opl3/opl3_synth.c:204:6-20: potentially dangerous second copy_from_user() ./drivers/scsi/megaraid.c:3465:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/aacraid/commctrl.c:116:5-19: potentially dangerous second copy_from_user() ./drivers/staging/lustre/lustre/obdclass/linux/linux-module.c:116:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/dpt_i2o.c:1847:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:1947:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2078:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2094:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2137:6-20: potentially dangerous second copy_from_user() ./drivers/acpi/custom_method.c:54:5-19: potentially dangerous second copy_from_user() ./drivers/hwtracing/stm/core.c:533:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/3w-sas.c:764:5-19: potentially dangerous second copy_from_user() ./drivers/isdn/hysdn/hysdn_procconf.c:133:6-20: potentially dangerous second copy_from_user() ./drivers/isdn/hysdn/hysdn_procconf.c:160:7-21: potentially dangerous second copy_from_user() ./drivers/isdn/i4l/isdn_ppp.c:875:7-21: potentially dangerous second copy_from_user() ./drivers/isdn/isdnloop/isdnloop.c:980:7-21: potentially dangerous second copy_from_user() ./drivers/md/md.c:6965:6-20: potentially dangerous second copy_from_user() ./net/ipv4/tcp.c:2267:6-20: potentially dangerous second copy_from_user() ./drivers/md/dm-ioctl.c:1738:5-19: potentially dangerous second copy_from_user() I think the check logic could be improved (e.g. doesn't notice "++"), but I haven't had time to dig in much further... -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2016-04-26 22:30 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2016-04-26 22:30 UTC (permalink / raw) To: cocci On Tue, Apr 26, 2016 at 3:24 PM, Kees Cook <keescook@chromium.org> wrote: > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers > + > +virtual report > +virtual org > + > + at cfu_twice@ > +position p; > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset > +*copy_from_user at p(dest2, src, size2) > + > + at script:python depends on org@ > +p << cfu_twice.p; > +@@ > + > +cocci.print_main("potentially dangerous second copy_from_user()",p) > + > + at script:python depends on report@ > +p << cfu_twice.p; > +@@ > + > +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") > -- > 2.6.3 > > > -- > Kees Cook > Chrome OS & Brillo Security And here's the current (noisy) output: ./arch/powerpc/platforms/powernv/opal-prd.c:248:6-20: potentially dangerous second copy_from_user() ./sound/isa/sb/sb16_csp.c:391:7-21: potentially dangerous second copy_from_user() ./drivers/gpu/drm/tegra/drm.c:361:6-20: potentially dangerous second copy_from_user() ./fs/exec.c:537:7-21: potentially dangerous second copy_from_user() ./drivers/char/lp.c:387:7-21: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2149:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2247:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2292:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2332:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2355:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2396:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2429:6-20: potentially dangerous second copy_from_user() ./drivers/net/ethernet/chelsio/cxgb3/cxgb3_main.c:2481:6-20: potentially dangerous second copy_from_user() ./arch/ia64/kernel/perfmon.c:4833:11-25: potentially dangerous second copy_from_user() ./drivers/staging/i4l/icn/icn.c:1048:7-21: potentially dangerous second copy_from_user() ./drivers/misc/mic/vop/vop_vringh.c:775:9-23: potentially dangerous second copy_from_user() ./drivers/misc/mic/vop/vop_vringh.c:944:6-20: potentially dangerous second copy_from_user() ./fs/coda/psdev.c:128:6-20: potentially dangerous second copy_from_user() ./fs/coda/psdev.c:174:12-26: potentially dangerous second copy_from_user() ./drivers/hid/hid-picolcd_debugfs.c:283:6-20: potentially dangerous second copy_from_user() ./fs/aio.c:1610:15-29: potentially dangerous second copy_from_user() ./fs/splice.c:1459:6-20: potentially dangerous second copy_from_user() ./kernel/kexec_core.c:815:12-26: potentially dangerous second copy_from_user() ./kernel/kexec_core.c:752:12-26: potentially dangerous second copy_from_user() ./drivers/scsi/3w-9xxx.c:691:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/3w-xxxx.c:921:5-19: potentially dangerous second copy_from_user() ./drivers/platform/chrome/cros_ec_dev.c:145:5-19: potentially dangerous second copy_from_user() ./sound/drivers/opl3/opl3_synth.c:204:6-20: potentially dangerous second copy_from_user() ./drivers/scsi/megaraid.c:3465:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/aacraid/commctrl.c:116:5-19: potentially dangerous second copy_from_user() ./drivers/staging/lustre/lustre/obdclass/linux/linux-module.c:116:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/dpt_i2o.c:1847:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:1947:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2078:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2094:6-20: potentially dangerous second copy_from_user() ./drivers/net/tun.c:2137:6-20: potentially dangerous second copy_from_user() ./drivers/acpi/custom_method.c:54:5-19: potentially dangerous second copy_from_user() ./drivers/hwtracing/stm/core.c:533:5-19: potentially dangerous second copy_from_user() ./drivers/scsi/3w-sas.c:764:5-19: potentially dangerous second copy_from_user() ./drivers/isdn/hysdn/hysdn_procconf.c:133:6-20: potentially dangerous second copy_from_user() ./drivers/isdn/hysdn/hysdn_procconf.c:160:7-21: potentially dangerous second copy_from_user() ./drivers/isdn/i4l/isdn_ppp.c:875:7-21: potentially dangerous second copy_from_user() ./drivers/isdn/isdnloop/isdnloop.c:980:7-21: potentially dangerous second copy_from_user() ./drivers/md/md.c:6965:6-20: potentially dangerous second copy_from_user() ./net/ipv4/tcp.c:2267:6-20: potentially dangerous second copy_from_user() ./drivers/md/dm-ioctl.c:1738:5-19: potentially dangerous second copy_from_user() I think the check logic could be improved (e.g. doesn't notice "++"), but I haven't had time to dig in much further... -Kees -- Kees Cook Chrome OS & Brillo Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH] coccicheck: add a test for repeat copy_from_user 2016-04-26 22:24 ` [Cocci] " Kees Cook @ 2016-12-27 18:21 ` Julia Lawall -1 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2016-12-27 18:21 UTC (permalink / raw) To: Kees Cook Cc: linux-kernel, Gilles Muller, Nicolas Palix, Michal Marek, Pengfei Wang, cocci, Vaishali Thakkar I totally dropped the ball on this. Many thanks to Vaishali for resurrecting it. Some changes are suggested below. On Tue, 26 Apr 2016, Kees Cook wrote: > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers The options could be: --no-include --include-headers Actually, Coccinelle supports both, but it only officially supports the -- versions. > + > +virtual report > +virtual org Add, the following for the *s: virtual context Then add the following rule: @ok@ position p; expression src,dest; @@ copy_from_user@p(&dest, src, sizeof(dest)) > + > +@cfu_twice@ > +position p; Change this to: position p != ok.p; > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset Add the following lines: when != if (size2 > e1 || ...) { ... return ...; } when != if (size2 > e1 || ...) { ... size2 = e2 ... } These changes drop cases where the last argument to copy_from_usr is the size of the first argument, which seems safe enough, and where there is a test on the size value that can either update it or abort the function. These changes only eliminate false positives, as far as I could tell. If it would be more convenient, I could just send the complete revised patch, or whatever seems convenient. thanks, julia > +*copy_from_user@p(dest2, src, size2) > + > +@script:python depends on org@ > +p << cfu_twice.p; > +@@ > + > +cocci.print_main("potentially dangerous second copy_from_user()",p) > + > +@script:python depends on report@ > +p << cfu_twice.p; > +@@ > + > +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") > -- > 2.6.3 > > > -- > Kees Cook > Chrome OS & Brillo Security > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2016-12-27 18:21 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2016-12-27 18:21 UTC (permalink / raw) To: cocci I totally dropped the ball on this. Many thanks to Vaishali for resurrecting it. Some changes are suggested below. On Tue, 26 Apr 2016, Kees Cook wrote: > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers The options could be: --no-include --include-headers Actually, Coccinelle supports both, but it only officially supports the -- versions. > + > +virtual report > +virtual org Add, the following for the *s: virtual context Then add the following rule: @ok@ position p; expression src,dest; @@ copy_from_user at p(&dest, src, sizeof(dest)) > + > + at cfu_twice@ > +position p; Change this to: position p != ok.p; > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset Add the following lines: when != if (size2 > e1 || ...) { ... return ...; } when != if (size2 > e1 || ...) { ... size2 = e2 ... } These changes drop cases where the last argument to copy_from_usr is the size of the first argument, which seems safe enough, and where there is a test on the size value that can either update it or abort the function. These changes only eliminate false positives, as far as I could tell. If it would be more convenient, I could just send the complete revised patch, or whatever seems convenient. thanks, julia > +*copy_from_user at p(dest2, src, size2) > + > + at script:python depends on org@ > +p << cfu_twice.p; > +@@ > + > +cocci.print_main("potentially dangerous second copy_from_user()",p) > + > + at script:python depends on report@ > +p << cfu_twice.p; > +@@ > + > +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") > -- > 2.6.3 > > > -- > Kees Cook > Chrome OS & Brillo Security > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2016-12-27 18:21 ` [Cocci] " Julia Lawall (?) @ 2017-01-02 15:45 ` Pengfei Wang -1 siblings, 0 replies; 31+ messages in thread From: Pengfei Wang @ 2017-01-02 15:45 UTC (permalink / raw) To: cocci Hi, Thank you for paying attention to this problem as I reported earlier as a double-fetch bug. My research topic is the detection of the double-fetch bugs, and I once conducted my work on the basis of the Coccinelle engine. We totally found 6 previously unknown double-fetch bugs by our approach, five from Linux kernel-4.5 and one from Android kernel-6.0.1. We made the Coccinelle script of our approach publicly available ( https://github.com/wpengfei/double_fetch_cocci.git), hoping it can be useful for the kernel security check. We took into consideration of many double-fetch cases according to the investigation of the kernel source code. Even though it is not 100% accurate yet, it can narrow down the scope from around 40 thousand kernel source files to only 53 candidate files, which include five true cases for kernel-4.5. We hope our work could provide some practical help on avoiding the double-fetch bugs. Please feel free to contact me if you?re interested or would like to give us any feedback. Thank you! Kind regards Pengfei Wang ? 2016?12?28????2:21?Julia Lawall <julia.lawall@lip6.fr> ??? I totally dropped the ball on this. Many thanks to Vaishali for resurrecting it. Some changes are suggested below. On Tue, 26 Apr 2016, Kees Cook wrote: This is usually a sign of a resized request. This adds a check for potential races or confusions. The check isn't 100% accurate, so it needs some manual review. Signed-off-by: Kees Cook <keescook@chromium.org> --- scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 scripts/coccinelle/tests/reusercopy.cocci diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci new file mode 100644 index 000000000000..53645de8ae95 --- /dev/null +++ b/scripts/coccinelle/tests/reusercopy.cocci @@ -0,0 +1,36 @@ +/// Recopying from the same user buffer frequently indicates a pattern of +/// Reading a size header, allocating, and then re-reading an entire +/// structure. If the structure's size is not re-validated, this can lead +/// to structure or data size confusions. +/// +// Confidence: Moderate +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Comments: +// Options: -no_includes -include_headers The options could be: --no-include --include-headers Actually, Coccinelle supports both, but it only officially supports the -- versions. + +virtual report +virtual org Add, the following for the *s: virtual context Then add the following rule: @ok@ position p; expression src,dest; @@ copy_from_user at p(&dest, src, sizeof(dest)) + + at cfu_twice@ +position p; Change this to: position p != ok.p; +identifier src; +expression dest1, dest2, size1, size2, offset; +@@ + +*copy_from_user(dest1, src, size1) + ... when != src = offset + when != src += offset Add the following lines: when != if (size2 > e1 || ...) { ... return ...; } when != if (size2 > e1 || ...) { ... size2 = e2 ... } These changes drop cases where the last argument to copy_from_usr is the size of the first argument, which seems safe enough, and where there is a test on the size value that can either update it or abort the function. These changes only eliminate false positives, as far as I could tell. If it would be more convenient, I could just send the complete revised patch, or whatever seems convenient. thanks, julia +*copy_from_user at p(dest2, src, size2) + + at script:python depends on org@ +p << cfu_twice.p; +@@ + +cocci.print_main("potentially dangerous second copy_from_user()",p) + +@script:python depends on report@ +p << cfu_twice.p; +@@ + +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") -- 2.6.3 -- Kees Cook Chrome OS & Brillo Security -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170102/7bf49a2b/attachment-0001.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: cocci.zip Type: application/zip Size: 86659 bytes Desc: not available URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170102/7bf49a2b/attachment-0001.zip> ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2016-12-27 18:21 ` [Cocci] " Julia Lawall @ 2017-01-09 17:05 ` Vaishali Thakkar -1 siblings, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-09 17:05 UTC (permalink / raw) To: Julia Lawall, Kees Cook Cc: Pengfei Wang, Vaishali Thakkar, linux-kernel, Michal Marek, cocci On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > >> This is usually a sign of a resized request. This adds a check for >> potential races or confusions. The check isn't 100% accurate, so it >> needs some manual review. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> >> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >> new file mode 100644 >> index 000000000000..53645de8ae95 >> --- /dev/null >> +++ b/scripts/coccinelle/tests/reusercopy.cocci >> @@ -0,0 +1,36 @@ >> +/// Recopying from the same user buffer frequently indicates a pattern of >> +/// Reading a size header, allocating, and then re-reading an entire >> +/// structure. If the structure's size is not re-validated, this can lead >> +/// to structure or data size confusions. >> +/// >> +// Confidence: Moderate >> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> +// URL: http://coccinelle.lip6.fr/ >> +// Comments: >> +// Options: -no_includes -include_headers > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > >> + >> +virtual report >> +virtual org > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user@p(&dest, src, sizeof(dest)) > >> + >> +@cfu_twice@ >> +position p; > > Change this to: > > position p != ok.p; > >> +identifier src; >> +expression dest1, dest2, size1, size2, offset; >> +@@ >> + >> +*copy_from_user(dest1, src, size1) >> + ... when != src = offset >> + when != src += offset Here, may be we should add few more lines from Pengfei's script to avoid th potential FPs. > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > thanks, > julia > >> +*copy_from_user@p(dest2, src, size2) >> + >> +@script:python depends on org@ >> +p << cfu_twice.p; >> +@@ >> + >> +cocci.print_main("potentially dangerous second copy_from_user()",p) >> + >> +@script:python depends on report@ >> +p << cfu_twice.p; >> +@@ >> + >> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >> -- >> 2.6.3 >> >> >> -- >> Kees Cook >> Chrome OS & Brillo Security >> > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-09 17:05 ` Vaishali Thakkar 0 siblings, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-09 17:05 UTC (permalink / raw) To: cocci On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > >> This is usually a sign of a resized request. This adds a check for >> potential races or confusions. The check isn't 100% accurate, so it >> needs some manual review. >> >> Signed-off-by: Kees Cook <keescook@chromium.org> >> --- >> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> >> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >> new file mode 100644 >> index 000000000000..53645de8ae95 >> --- /dev/null >> +++ b/scripts/coccinelle/tests/reusercopy.cocci >> @@ -0,0 +1,36 @@ >> +/// Recopying from the same user buffer frequently indicates a pattern of >> +/// Reading a size header, allocating, and then re-reading an entire >> +/// structure. If the structure's size is not re-validated, this can lead >> +/// to structure or data size confusions. >> +/// >> +// Confidence: Moderate >> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> +// URL: http://coccinelle.lip6.fr/ >> +// Comments: >> +// Options: -no_includes -include_headers > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > >> + >> +virtual report >> +virtual org > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user at p(&dest, src, sizeof(dest)) > >> + >> + at cfu_twice@ >> +position p; > > Change this to: > > position p != ok.p; > >> +identifier src; >> +expression dest1, dest2, size1, size2, offset; >> +@@ >> + >> +*copy_from_user(dest1, src, size1) >> + ... when != src = offset >> + when != src += offset Here, may be we should add few more lines from Pengfei's script to avoid th potential FPs. > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > thanks, > julia > >> +*copy_from_user at p(dest2, src, size2) >> + >> + at script:python depends on org@ >> +p << cfu_twice.p; >> +@@ >> + >> +cocci.print_main("potentially dangerous second copy_from_user()",p) >> + >> + at script:python depends on report@ >> +p << cfu_twice.p; >> +@@ >> + >> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >> -- >> 2.6.3 >> >> >> -- >> Kees Cook >> Chrome OS & Brillo Security >> > _______________________________________________ > Cocci mailing list > Cocci at systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-09 17:05 ` Vaishali Thakkar @ 2017-01-09 19:08 ` Julia Lawall -1 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2017-01-09 19:08 UTC (permalink / raw) To: Vaishali Thakkar Cc: Julia Lawall, Kees Cook, Pengfei Wang, Vaishali Thakkar, linux-kernel, Michal Marek, cocci On Mon, 9 Jan 2017, Vaishali Thakkar wrote: > On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > > I totally dropped the ball on this. Many thanks to Vaishali for > > resurrecting it. > > > > Some changes are suggested below. > > > > On Tue, 26 Apr 2016, Kees Cook wrote: > > > > > This is usually a sign of a resized request. This adds a check for > > > potential races or confusions. The check isn't 100% accurate, so it > > > needs some manual review. > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > scripts/coccinelle/tests/reusercopy.cocci | 36 > > > +++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > > > > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci > > > b/scripts/coccinelle/tests/reusercopy.cocci > > > new file mode 100644 > > > index 000000000000..53645de8ae95 > > > --- /dev/null > > > +++ b/scripts/coccinelle/tests/reusercopy.cocci > > > @@ -0,0 +1,36 @@ > > > +/// Recopying from the same user buffer frequently indicates a pattern of > > > +/// Reading a size header, allocating, and then re-reading an entire > > > +/// structure. If the structure's size is not re-validated, this can lead > > > +/// to structure or data size confusions. > > > +/// > > > +// Confidence: Moderate > > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > > > +// URL: http://coccinelle.lip6.fr/ > > > +// Comments: > > > +// Options: -no_includes -include_headers > > > > The options could be: --no-include --include-headers > > > > Actually, Coccinelle supports both, but it only officially supports the > > -- versions. > > > > > + > > > +virtual report > > > +virtual org > > > > Add, the following for the *s: > > > > virtual context > > > > Then add the following rule: > > > > @ok@ > > position p; > > expression src,dest; > > @@ > > > > copy_from_user@p(&dest, src, sizeof(dest)) > > > > > + > > > +@cfu_twice@ > > > +position p; > > > > Change this to: > > > > position p != ok.p; > > > > > +identifier src; > > > +expression dest1, dest2, size1, size2, offset; > > > +@@ > > > + > > > +*copy_from_user(dest1, src, size1) > > > + ... when != src = offset > > > + when != src += offset > > Here, may be we should add few more lines from Pengfei's > script to avoid th potential FPs. Which lines (I don't have it handy)? julia > > > Add the following lines: > > > > when != if (size2 > e1 || ...) { ... return ...; } > > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > > > These changes drop cases where the last argument to copy_from_usr is the > > size of the first argument, which seems safe enough, and where there is a > > test on the size value that can either update it or abort the function. > > These changes only eliminate false positives, as far as I could tell. > > > > If it would be more convenient, I could just send the complete revised > > patch, or whatever seems convenient. > > I was also thinking that probably we should also add other user space memory > API functions. May be get_user and strncpy_from_user. Although I'm not sure > how common it is to find such patterns for both of these functions. > > > thanks, > > julia > > > > > +*copy_from_user@p(dest2, src, size2) > > > + > > > +@script:python depends on org@ > > > +p << cfu_twice.p; > > > +@@ > > > + > > > +cocci.print_main("potentially dangerous second copy_from_user()",p) > > > + > > > +@script:python depends on report@ > > > +p << cfu_twice.p; > > > +@@ > > > + > > > +coccilib.report.print_report(p[0],"potentially dangerous second > > > copy_from_user()") > > > -- > > > 2.6.3 > > > > > > > > > -- > > > Kees Cook > > > Chrome OS & Brillo Security > > > > > _______________________________________________ > > Cocci mailing list > > Cocci@systeme.lip6.fr > > https://systeme.lip6.fr/mailman/listinfo/cocci > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-09 19:08 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2017-01-09 19:08 UTC (permalink / raw) To: cocci On Mon, 9 Jan 2017, Vaishali Thakkar wrote: > On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > > I totally dropped the ball on this. Many thanks to Vaishali for > > resurrecting it. > > > > Some changes are suggested below. > > > > On Tue, 26 Apr 2016, Kees Cook wrote: > > > > > This is usually a sign of a resized request. This adds a check for > > > potential races or confusions. The check isn't 100% accurate, so it > > > needs some manual review. > > > > > > Signed-off-by: Kees Cook <keescook@chromium.org> > > > --- > > > scripts/coccinelle/tests/reusercopy.cocci | 36 > > > +++++++++++++++++++++++++++++++ > > > 1 file changed, 36 insertions(+) > > > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > > > > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci > > > b/scripts/coccinelle/tests/reusercopy.cocci > > > new file mode 100644 > > > index 000000000000..53645de8ae95 > > > --- /dev/null > > > +++ b/scripts/coccinelle/tests/reusercopy.cocci > > > @@ -0,0 +1,36 @@ > > > +/// Recopying from the same user buffer frequently indicates a pattern of > > > +/// Reading a size header, allocating, and then re-reading an entire > > > +/// structure. If the structure's size is not re-validated, this can lead > > > +/// to structure or data size confusions. > > > +/// > > > +// Confidence: Moderate > > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > > > +// URL: http://coccinelle.lip6.fr/ > > > +// Comments: > > > +// Options: -no_includes -include_headers > > > > The options could be: --no-include --include-headers > > > > Actually, Coccinelle supports both, but it only officially supports the > > -- versions. > > > > > + > > > +virtual report > > > +virtual org > > > > Add, the following for the *s: > > > > virtual context > > > > Then add the following rule: > > > > @ok@ > > position p; > > expression src,dest; > > @@ > > > > copy_from_user at p(&dest, src, sizeof(dest)) > > > > > + > > > + at cfu_twice@ > > > +position p; > > > > Change this to: > > > > position p != ok.p; > > > > > +identifier src; > > > +expression dest1, dest2, size1, size2, offset; > > > +@@ > > > + > > > +*copy_from_user(dest1, src, size1) > > > + ... when != src = offset > > > + when != src += offset > > Here, may be we should add few more lines from Pengfei's > script to avoid th potential FPs. Which lines (I don't have it handy)? julia > > > Add the following lines: > > > > when != if (size2 > e1 || ...) { ... return ...; } > > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > > > These changes drop cases where the last argument to copy_from_usr is the > > size of the first argument, which seems safe enough, and where there is a > > test on the size value that can either update it or abort the function. > > These changes only eliminate false positives, as far as I could tell. > > > > If it would be more convenient, I could just send the complete revised > > patch, or whatever seems convenient. > > I was also thinking that probably we should also add other user space memory > API functions. May be get_user and strncpy_from_user. Although I'm not sure > how common it is to find such patterns for both of these functions. > > > thanks, > > julia > > > > > +*copy_from_user at p(dest2, src, size2) > > > + > > > + at script:python depends on org@ > > > +p << cfu_twice.p; > > > +@@ > > > + > > > +cocci.print_main("potentially dangerous second copy_from_user()",p) > > > + > > > + at script:python depends on report@ > > > +p << cfu_twice.p; > > > +@@ > > > + > > > +coccilib.report.print_report(p[0],"potentially dangerous second > > > copy_from_user()") > > > -- > > > 2.6.3 > > > > > > > > > -- > > > Kees Cook > > > Chrome OS & Brillo Security > > > > > _______________________________________________ > > Cocci mailing list > > Cocci at systeme.lip6.fr > > https://systeme.lip6.fr/mailman/listinfo/cocci > > > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-09 19:08 ` Julia Lawall @ 2017-01-09 20:56 ` Kees Cook -1 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-09 20:56 UTC (permalink / raw) To: Julia Lawall Cc: Vaishali Thakkar, Pengfei Wang, Vaishali Thakkar, LKML, Michal Marek, cocci On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Mon, 9 Jan 2017, Vaishali Thakkar wrote: > >> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >> > I totally dropped the ball on this. Many thanks to Vaishali for >> > resurrecting it. >> > >> > Some changes are suggested below. >> > >> > On Tue, 26 Apr 2016, Kees Cook wrote: >> > >> > > This is usually a sign of a resized request. This adds a check for >> > > potential races or confusions. The check isn't 100% accurate, so it >> > > needs some manual review. >> > > >> > > Signed-off-by: Kees Cook <keescook@chromium.org> >> > > --- >> > > scripts/coccinelle/tests/reusercopy.cocci | 36 >> > > +++++++++++++++++++++++++++++++ >> > > 1 file changed, 36 insertions(+) >> > > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> > > >> > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci >> > > b/scripts/coccinelle/tests/reusercopy.cocci >> > > new file mode 100644 >> > > index 000000000000..53645de8ae95 >> > > --- /dev/null >> > > +++ b/scripts/coccinelle/tests/reusercopy.cocci >> > > @@ -0,0 +1,36 @@ >> > > +/// Recopying from the same user buffer frequently indicates a pattern of >> > > +/// Reading a size header, allocating, and then re-reading an entire >> > > +/// structure. If the structure's size is not re-validated, this can lead >> > > +/// to structure or data size confusions. >> > > +/// >> > > +// Confidence: Moderate >> > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> > > +// URL: http://coccinelle.lip6.fr/ >> > > +// Comments: >> > > +// Options: -no_includes -include_headers >> > >> > The options could be: --no-include --include-headers >> > >> > Actually, Coccinelle supports both, but it only officially supports the >> > -- versions. >> > >> > > + >> > > +virtual report >> > > +virtual org >> > >> > Add, the following for the *s: >> > >> > virtual context >> > >> > Then add the following rule: >> > >> > @ok@ >> > position p; >> > expression src,dest; >> > @@ >> > >> > copy_from_user@p(&dest, src, sizeof(dest)) >> > >> > > + >> > > +@cfu_twice@ >> > > +position p; >> > >> > Change this to: >> > >> > position p != ok.p; >> > >> > > +identifier src; >> > > +expression dest1, dest2, size1, size2, offset; >> > > +@@ >> > > + >> > > +*copy_from_user(dest1, src, size1) >> > > + ... when != src = offset >> > > + when != src += offset >> >> Here, may be we should add few more lines from Pengfei's >> script to avoid th potential FPs. > > Which lines (I don't have it handy)? I'm going to compare https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci to my original one, add your improvements and see what I get... -Kees > > julia > >> >> > Add the following lines: >> > >> > when != if (size2 > e1 || ...) { ... return ...; } >> > when != if (size2 > e1 || ...) { ... size2 = e2 ... } >> > >> > These changes drop cases where the last argument to copy_from_usr is the >> > size of the first argument, which seems safe enough, and where there is a >> > test on the size value that can either update it or abort the function. >> > These changes only eliminate false positives, as far as I could tell. >> > >> > If it would be more convenient, I could just send the complete revised >> > patch, or whatever seems convenient. >> >> I was also thinking that probably we should also add other user space memory >> API functions. May be get_user and strncpy_from_user. Although I'm not sure >> how common it is to find such patterns for both of these functions. >> >> > thanks, >> > julia >> > >> > > +*copy_from_user@p(dest2, src, size2) >> > > + >> > > +@script:python depends on org@ >> > > +p << cfu_twice.p; >> > > +@@ >> > > + >> > > +cocci.print_main("potentially dangerous second copy_from_user()",p) >> > > + >> > > +@script:python depends on report@ >> > > +p << cfu_twice.p; >> > > +@@ >> > > + >> > > +coccilib.report.print_report(p[0],"potentially dangerous second >> > > copy_from_user()") >> > > -- >> > > 2.6.3 >> > > >> > > >> > > -- >> > > Kees Cook >> > > Chrome OS & Brillo Security >> > > >> > _______________________________________________ >> > Cocci mailing list >> > Cocci@systeme.lip6.fr >> > https://systeme.lip6.fr/mailman/listinfo/cocci >> > >> >> -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-09 20:56 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-09 20:56 UTC (permalink / raw) To: cocci On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: > > > On Mon, 9 Jan 2017, Vaishali Thakkar wrote: > >> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >> > I totally dropped the ball on this. Many thanks to Vaishali for >> > resurrecting it. >> > >> > Some changes are suggested below. >> > >> > On Tue, 26 Apr 2016, Kees Cook wrote: >> > >> > > This is usually a sign of a resized request. This adds a check for >> > > potential races or confusions. The check isn't 100% accurate, so it >> > > needs some manual review. >> > > >> > > Signed-off-by: Kees Cook <keescook@chromium.org> >> > > --- >> > > scripts/coccinelle/tests/reusercopy.cocci | 36 >> > > +++++++++++++++++++++++++++++++ >> > > 1 file changed, 36 insertions(+) >> > > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> > > >> > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci >> > > b/scripts/coccinelle/tests/reusercopy.cocci >> > > new file mode 100644 >> > > index 000000000000..53645de8ae95 >> > > --- /dev/null >> > > +++ b/scripts/coccinelle/tests/reusercopy.cocci >> > > @@ -0,0 +1,36 @@ >> > > +/// Recopying from the same user buffer frequently indicates a pattern of >> > > +/// Reading a size header, allocating, and then re-reading an entire >> > > +/// structure. If the structure's size is not re-validated, this can lead >> > > +/// to structure or data size confusions. >> > > +/// >> > > +// Confidence: Moderate >> > > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> > > +// URL: http://coccinelle.lip6.fr/ >> > > +// Comments: >> > > +// Options: -no_includes -include_headers >> > >> > The options could be: --no-include --include-headers >> > >> > Actually, Coccinelle supports both, but it only officially supports the >> > -- versions. >> > >> > > + >> > > +virtual report >> > > +virtual org >> > >> > Add, the following for the *s: >> > >> > virtual context >> > >> > Then add the following rule: >> > >> > @ok@ >> > position p; >> > expression src,dest; >> > @@ >> > >> > copy_from_user at p(&dest, src, sizeof(dest)) >> > >> > > + >> > > + at cfu_twice@ >> > > +position p; >> > >> > Change this to: >> > >> > position p != ok.p; >> > >> > > +identifier src; >> > > +expression dest1, dest2, size1, size2, offset; >> > > +@@ >> > > + >> > > +*copy_from_user(dest1, src, size1) >> > > + ... when != src = offset >> > > + when != src += offset >> >> Here, may be we should add few more lines from Pengfei's >> script to avoid th potential FPs. > > Which lines (I don't have it handy)? I'm going to compare https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci to my original one, add your improvements and see what I get... -Kees > > julia > >> >> > Add the following lines: >> > >> > when != if (size2 > e1 || ...) { ... return ...; } >> > when != if (size2 > e1 || ...) { ... size2 = e2 ... } >> > >> > These changes drop cases where the last argument to copy_from_usr is the >> > size of the first argument, which seems safe enough, and where there is a >> > test on the size value that can either update it or abort the function. >> > These changes only eliminate false positives, as far as I could tell. >> > >> > If it would be more convenient, I could just send the complete revised >> > patch, or whatever seems convenient. >> >> I was also thinking that probably we should also add other user space memory >> API functions. May be get_user and strncpy_from_user. Although I'm not sure >> how common it is to find such patterns for both of these functions. >> >> > thanks, >> > julia >> > >> > > +*copy_from_user at p(dest2, src, size2) >> > > + >> > > + at script:python depends on org@ >> > > +p << cfu_twice.p; >> > > +@@ >> > > + >> > > +cocci.print_main("potentially dangerous second copy_from_user()",p) >> > > + >> > > + at script:python depends on report@ >> > > +p << cfu_twice.p; >> > > +@@ >> > > + >> > > +coccilib.report.print_report(p[0],"potentially dangerous second >> > > copy_from_user()") >> > > -- >> > > 2.6.3 >> > > >> > > >> > > -- >> > > Kees Cook >> > > Chrome OS & Brillo Security >> > > >> > _______________________________________________ >> > Cocci mailing list >> > Cocci at systeme.lip6.fr >> > https://systeme.lip6.fr/mailman/listinfo/cocci >> > >> >> -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-09 20:56 ` Kees Cook @ 2017-01-09 22:02 ` Kees Cook -1 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-09 22:02 UTC (permalink / raw) To: Julia Lawall Cc: Vaishali Thakkar, Pengfei Wang, Vaishali Thakkar, LKML, Michal Marek, cocci On Mon, Jan 9, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> >> On Mon, 9 Jan 2017, Vaishali Thakkar wrote: >> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >> >> Which lines (I don't have it handy)? > > I'm going to compare > https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci > to my original one, add your improvements and see what I get... Okay, I finally had time to look at this. Pengfei added two other logical cases that should be checked for, IIUC: 1) destination alias checking (with assignment either before or after the first copy_from_user): struct thing object; struct thing *pointer = &object; copy_from_user(..., &object); ... copy_from_user(..., pointer); 2) field writes (via . or ->, instead of short writes): struct thing object; copy_from_user(..., &object.field); ... copy_from_user(..., &object); It'd probably better to convert Pengfei's into being able to run under the coccicheck target. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-09 22:02 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-09 22:02 UTC (permalink / raw) To: cocci On Mon, Jan 9, 2017 at 12:56 PM, Kees Cook <keescook@chromium.org> wrote: > On Mon, Jan 9, 2017 at 11:08 AM, Julia Lawall <julia.lawall@lip6.fr> wrote: >> >> On Mon, 9 Jan 2017, Vaishali Thakkar wrote: >> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >> >> Which lines (I don't have it handy)? > > I'm going to compare > https://github.com/wpengfei/double_fetch_cocci/blob/master/pattern_match_linux.cocci > to my original one, add your improvements and see what I get... Okay, I finally had time to look at this. Pengfei added two other logical cases that should be checked for, IIUC: 1) destination alias checking (with assignment either before or after the first copy_from_user): struct thing object; struct thing *pointer = &object; copy_from_user(..., &object); ... copy_from_user(..., pointer); 2) field writes (via . or ->, instead of short writes): struct thing object; copy_from_user(..., &object.field); ... copy_from_user(..., &object); It'd probably better to convert Pengfei's into being able to run under the coccicheck target. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-09 17:05 ` Vaishali Thakkar (?) (?) @ 2017-01-10 8:21 ` Pengfei Wang 2017-01-10 8:40 ` Vaishali Thakkar 2017-01-10 19:15 ` Kees Cook -1 siblings, 2 replies; 31+ messages in thread From: Pengfei Wang @ 2017-01-10 8:21 UTC (permalink / raw) To: cocci > ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? > > On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >> I totally dropped the ball on this. Many thanks to Vaishali for >> resurrecting it. >> >> Some changes are suggested below. >> >> On Tue, 26 Apr 2016, Kees Cook wrote: >> >>> This is usually a sign of a resized request. This adds a check for >>> potential races or confusions. The check isn't 100% accurate, so it >>> needs some manual review. >>> >>> Signed-off-by: Kees Cook <keescook@chromium.org> >>> --- >>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>> 1 file changed, 36 insertions(+) >>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>> >>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>> new file mode 100644 >>> index 000000000000..53645de8ae95 >>> --- /dev/null >>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>> @@ -0,0 +1,36 @@ >>> +/// Recopying from the same user buffer frequently indicates a pattern of >>> +/// Reading a size header, allocating, and then re-reading an entire >>> +/// structure. If the structure's size is not re-validated, this can lead >>> +/// to structure or data size confusions. >>> +/// >>> +// Confidence: Moderate >>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>> +// URL: http://coccinelle.lip6.fr/ >>> +// Comments: >>> +// Options: -no_includes -include_headers >> >> The options could be: --no-include --include-headers >> >> Actually, Coccinelle supports both, but it only officially supports the >> -- versions. >> >>> + >>> +virtual report >>> +virtual org >> >> Add, the following for the *s: >> >> virtual context >> >> Then add the following rule: >> >> @ok@ >> position p; >> expression src,dest; >> @@ >> >> copy_from_user at p(&dest, src, sizeof(dest)) >> >>> + >>> + at cfu_twice@ >>> +position p; >> >> Change this to: >> >> position p != ok.p; >> >>> +identifier src; >>> +expression dest1, dest2, size1, size2, offset; >>> +@@ >>> + >>> +*copy_from_user(dest1, src, size1) >>> + ... when != src = offset >>> + when != src += offset > > Here, may be we should add few more lines from Pengfei's > script to avoid th potential FPs. > >> Add the following lines: >> >> when != if (size2 > e1 || ...) { ... return ...; } >> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >> >> These changes drop cases where the last argument to copy_from_usr is the >> size of the first argument, which seems safe enough, and where there is a >> test on the size value that can either update it or abort the function. >> These changes only eliminate false positives, as far as I could tell. >> >> If it would be more convenient, I could just send the complete revised >> patch, or whatever seems convenient. > > I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. I strongly recommend you adding get_user() API , which is used pervasively within the kernel just like copy_from user(). In many situations, there is a combination use, get_user() copies first then followed by a copy_from_user() copy. According to our investigation, this typical situation works by get_user() firstly copying a field of a specific struct to check, then copy_from_user() copies in the whole struct to use. Of course, the struct field is fetch twice. Regards Pengfei > >> thanks, >> julia >> >>> +*copy_from_user at p(dest2, src, size2) >>> + >>> + at script:python depends on org@ >>> +p << cfu_twice.p; >>> +@@ >>> + >>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>> + >>> + at script:python depends on report@ >>> +p << cfu_twice.p; >>> +@@ >>> + >>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>> -- >>> 2.6.3 >>> >>> >>> -- >>> Kees Cook >>> Chrome OS & Brillo Security >>> >> _______________________________________________ >> Cocci mailing list >> Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> >> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170110/8110b1b3/attachment-0001.html> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 8:21 ` Pengfei Wang @ 2017-01-10 8:40 ` Vaishali Thakkar 2017-01-10 19:15 ` Kees Cook 1 sibling, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-10 8:40 UTC (permalink / raw) To: Pengfei Wang Cc: Julia Lawall, Kees Cook, Vaishali Thakkar, linux-kernel, Michal Marek, cocci On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: > >> 在 2017年1月10日,上午1:05,Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道: >> >> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>> I totally dropped the ball on this. Many thanks to Vaishali for >>> resurrecting it. >>> >>> Some changes are suggested below. >>> >>> On Tue, 26 Apr 2016, Kees Cook wrote: >>> >>>> This is usually a sign of a resized request. This adds a check for >>>> potential races or confusions. The check isn't 100% accurate, so it >>>> needs some manual review. >>>> >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>> --- >>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 36 insertions(+) >>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>> >>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>> new file mode 100644 >>>> index 000000000000..53645de8ae95 >>>> --- /dev/null >>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>> @@ -0,0 +1,36 @@ >>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>> +/// Reading a size header, allocating, and then re-reading an entire >>>> +/// structure. If the structure's size is not re-validated, this can lead >>>> +/// to structure or data size confusions. >>>> +/// >>>> +// Confidence: Moderate >>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>> +// URL: http://coccinelle.lip6.fr/ >>>> +// Comments: >>>> +// Options: -no_includes -include_headers >>> >>> The options could be: --no-include --include-headers >>> >>> Actually, Coccinelle supports both, but it only officially supports the >>> -- versions. >>> >>>> + >>>> +virtual report >>>> +virtual org >>> >>> Add, the following for the *s: >>> >>> virtual context >>> >>> Then add the following rule: >>> >>> @ok@ >>> position p; >>> expression src,dest; >>> @@ >>> >>> copy_from_user@p(&dest, src, sizeof(dest)) >>> >>>> + >>>> +@cfu_twice@ >>>> +position p; >>> >>> Change this to: >>> >>> position p != ok.p; >>> >>>> +identifier src; >>>> +expression dest1, dest2, size1, size2, offset; >>>> +@@ >>>> + >>>> +*copy_from_user(dest1, src, size1) >>>> + ... when != src = offset >>>> + when != src += offset >> >> Here, may be we should add few more lines from Pengfei's >> script to avoid th potential FPs. >> >>> Add the following lines: >>> >>> when != if (size2 > e1 || ...) { ... return ...; } >>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>> >>> These changes drop cases where the last argument to copy_from_usr is the >>> size of the first argument, which seems safe enough, and where there is a >>> test on the size value that can either update it or abort the function. >>> These changes only eliminate false positives, as far as I could tell. >>> >>> If it would be more convenient, I could just send the complete revised >>> patch, or whatever seems convenient. >> >> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > > I strongly recommend you adding get_user() API , which is used pervasively > within the kernel just like copy_from user(). Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to include everything in the pattern matching rule. I'll send that as well. > In many situations, there is a combination use, get_user() copies first then > followed by a copy_from_user() copy. According to our investigation, this typical > situation works by get_user() firstly copying a field of a specific struct to check, > then copy_from_user() copies in the whole struct to use. Of course, the struct > field is fetch twice. Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like this: get_user(..., src.arg) //where src.arg = field of a structure ... copy_from_user(..., src, ...) //where src is a whole structure If that is the case then we would need to have one more new script or rule for such kind of combinational patterns. Disjunction can probably give FPs. Thanks! > Regards > Pengfei >> >>> thanks, >>> julia >>> >>>> +*copy_from_user@p(dest2, src, size2) >>>> + >>>> +@script:python depends on org@ >>>> +p << cfu_twice.p; >>>> +@@ >>>> + >>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>> + >>>> +@script:python depends on report@ >>>> +p << cfu_twice.p; >>>> +@@ >>>> + >>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>> -- >>>> 2.6.3 >>>> >>>> >>>> -- >>>> Kees Cook >>>> Chrome OS & Brillo Security >>>> >>> _______________________________________________ >>> Cocci mailing list >>> Cocci@systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> >>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-10 8:40 ` Vaishali Thakkar 0 siblings, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-10 8:40 UTC (permalink / raw) To: cocci On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: > >> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >> >> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>> I totally dropped the ball on this. Many thanks to Vaishali for >>> resurrecting it. >>> >>> Some changes are suggested below. >>> >>> On Tue, 26 Apr 2016, Kees Cook wrote: >>> >>>> This is usually a sign of a resized request. This adds a check for >>>> potential races or confusions. The check isn't 100% accurate, so it >>>> needs some manual review. >>>> >>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>> --- >>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 36 insertions(+) >>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>> >>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>> new file mode 100644 >>>> index 000000000000..53645de8ae95 >>>> --- /dev/null >>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>> @@ -0,0 +1,36 @@ >>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>> +/// Reading a size header, allocating, and then re-reading an entire >>>> +/// structure. If the structure's size is not re-validated, this can lead >>>> +/// to structure or data size confusions. >>>> +/// >>>> +// Confidence: Moderate >>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>> +// URL: http://coccinelle.lip6.fr/ >>>> +// Comments: >>>> +// Options: -no_includes -include_headers >>> >>> The options could be: --no-include --include-headers >>> >>> Actually, Coccinelle supports both, but it only officially supports the >>> -- versions. >>> >>>> + >>>> +virtual report >>>> +virtual org >>> >>> Add, the following for the *s: >>> >>> virtual context >>> >>> Then add the following rule: >>> >>> @ok@ >>> position p; >>> expression src,dest; >>> @@ >>> >>> copy_from_user at p(&dest, src, sizeof(dest)) >>> >>>> + >>>> + at cfu_twice@ >>>> +position p; >>> >>> Change this to: >>> >>> position p != ok.p; >>> >>>> +identifier src; >>>> +expression dest1, dest2, size1, size2, offset; >>>> +@@ >>>> + >>>> +*copy_from_user(dest1, src, size1) >>>> + ... when != src = offset >>>> + when != src += offset >> >> Here, may be we should add few more lines from Pengfei's >> script to avoid th potential FPs. >> >>> Add the following lines: >>> >>> when != if (size2 > e1 || ...) { ... return ...; } >>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>> >>> These changes drop cases where the last argument to copy_from_usr is the >>> size of the first argument, which seems safe enough, and where there is a >>> test on the size value that can either update it or abort the function. >>> These changes only eliminate false positives, as far as I could tell. >>> >>> If it would be more convenient, I could just send the complete revised >>> patch, or whatever seems convenient. >> >> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > > I strongly recommend you adding get_user() API , which is used pervasively > within the kernel just like copy_from user(). Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to include everything in the pattern matching rule. I'll send that as well. > In many situations, there is a combination use, get_user() copies first then > followed by a copy_from_user() copy. According to our investigation, this typical > situation works by get_user() firstly copying a field of a specific struct to check, > then copy_from_user() copies in the whole struct to use. Of course, the struct > field is fetch twice. Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like this: get_user(..., src.arg) //where src.arg = field of a structure ... copy_from_user(..., src, ...) //where src is a whole structure If that is the case then we would need to have one more new script or rule for such kind of combinational patterns. Disjunction can probably give FPs. Thanks! > Regards > Pengfei >> >>> thanks, >>> julia >>> >>>> +*copy_from_user at p(dest2, src, size2) >>>> + >>>> + at script:python depends on org@ >>>> +p << cfu_twice.p; >>>> +@@ >>>> + >>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>> + >>>> + at script:python depends on report@ >>>> +p << cfu_twice.p; >>>> +@@ >>>> + >>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>> -- >>>> 2.6.3 >>>> >>>> >>>> -- >>>> Kees Cook >>>> Chrome OS & Brillo Security >>>> >>> _______________________________________________ >>> Cocci mailing list >>> Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> >>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 8:40 ` Vaishali Thakkar (?) @ 2017-01-10 9:02 ` Pengfei Wang 2017-01-10 17:46 ` Vaishali Thakkar -1 siblings, 1 reply; 31+ messages in thread From: Pengfei Wang @ 2017-01-10 9:02 UTC (permalink / raw) To: cocci > ? 2017?1?10????4:40?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? > > On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >> >>> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >>> >>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>> resurrecting it. >>>> >>>> Some changes are suggested below. >>>> >>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>> >>>>> This is usually a sign of a resized request. This adds a check for >>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>> needs some manual review. >>>>> >>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>> --- >>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>> >>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>>> new file mode 100644 >>>>> index 000000000000..53645de8ae95 >>>>> --- /dev/null >>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>> @@ -0,0 +1,36 @@ >>>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>> +/// structure. If the structure's size is not re-validated, this can lead >>>>> +/// to structure or data size confusions. >>>>> +/// >>>>> +// Confidence: Moderate >>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>> +// URL: http://coccinelle.lip6.fr/ >>>>> +// Comments: >>>>> +// Options: -no_includes -include_headers >>>> >>>> The options could be: --no-include --include-headers >>>> >>>> Actually, Coccinelle supports both, but it only officially supports the >>>> -- versions. >>>> >>>>> + >>>>> +virtual report >>>>> +virtual org >>>> >>>> Add, the following for the *s: >>>> >>>> virtual context >>>> >>>> Then add the following rule: >>>> >>>> @ok@ >>>> position p; >>>> expression src,dest; >>>> @@ >>>> >>>> copy_from_user at p(&dest, src, sizeof(dest)) >>>> >>>>> + >>>>> + at cfu_twice@ >>>>> +position p; >>>> >>>> Change this to: >>>> >>>> position p != ok.p; >>>> >>>>> +identifier src; >>>>> +expression dest1, dest2, size1, size2, offset; >>>>> +@@ >>>>> + >>>>> +*copy_from_user(dest1, src, size1) >>>>> + ... when != src = offset >>>>> + when != src += offset >>> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >>> >>>> Add the following lines: >>>> >>>> when != if (size2 > e1 || ...) { ... return ...; } >>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>> >>>> These changes drop cases where the last argument to copy_from_usr is the >>>> size of the first argument, which seems safe enough, and where there is a >>>> test on the size value that can either update it or abort the function. >>>> These changes only eliminate false positives, as far as I could tell. >>>> >>>> If it would be more convenient, I could just send the complete revised >>>> patch, or whatever seems convenient. >>> >>> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. >> >> I strongly recommend you adding get_user() API , which is used pervasively >> within the kernel just like copy_from user(). > > Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to > include everything in the pattern matching rule. I'll send that as well. > >> In many situations, there is a combination use, get_user() copies first then >> followed by a copy_from_user() copy. According to our investigation, this typical >> situation works by get_user() firstly copying a field of a specific struct to check, >> then copy_from_user() copies in the whole struct to use. Of course, the struct >> field is fetch twice. > > Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like > this: > > get_user(..., src.arg) //where src.arg = field of a structure > ... > copy_from_user(..., src, ...) //where src is a whole structure > > If that is the case then we would need to have one more new script > or rule for such kind of combinational patterns. Disjunction can > probably give FPs. Yes, I?ve seen these cases when examining the source code. Actually, copying a field first and then copying the whole struct is very common in the kernel especially the driver. For example, when a struct (or a message as we call it) is variable length, the first copy is used to check its size field, and allocate a kernel buffer based on it, then the second copy is to copy the whole message also based on the size. There are also situations of the variable type messages. The reason that they use get_user() instead of copy_from_user() for the first copy is because get_user() is defined as a macro, which works faster than a function call that copy_from_user() does when copy simple data type such as char and int. Regards Pengfei > Thanks! > >> Regards >> Pengfei >>> >>>> thanks, >>>> julia >>>> >>>>> +*copy_from_user at p(dest2, src, size2) >>>>> + >>>>> + at script:python depends on org@ >>>>> +p << cfu_twice.p; >>>>> +@@ >>>>> + >>>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>>> + >>>>> + at script:python depends on report@ >>>>> +p << cfu_twice.p; >>>>> +@@ >>>>> + >>>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>>> -- >>>>> 2.6.3 >>>>> >>>>> >>>>> -- >>>>> Kees Cook >>>>> Chrome OS & Brillo Security >>>>> >>>> _______________________________________________ >>>> Cocci mailing list >>>> Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> <mailto:Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr>> >>>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170110/eadab6db/attachment-0001.html> ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 9:02 ` Pengfei Wang @ 2017-01-10 17:46 ` Vaishali Thakkar 0 siblings, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-10 17:46 UTC (permalink / raw) To: Pengfei Wang Cc: Julia Lawall, Kees Cook, Vaishali Thakkar, linux-kernel, Michal Marek, cocci On Tuesday 10 January 2017 02:32 PM, Pengfei Wang wrote: > >> 在 2017年1月10日,下午4:40,Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道: >> >> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >>> >>>> 在 2017年1月10日,上午1:05,Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道: >>>> >>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>>> resurrecting it. >>>>> >>>>> Some changes are suggested below. >>>>> >>>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>>> >>>>>> This is usually a sign of a resized request. This adds a check for >>>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>>> needs some manual review. >>>>>> >>>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>>> --- >>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>>> >>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>>>> new file mode 100644 >>>>>> index 000000000000..53645de8ae95 >>>>>> --- /dev/null >>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>>> @@ -0,0 +1,36 @@ >>>>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>>> +/// structure. If the structure's size is not re-validated, this can lead >>>>>> +/// to structure or data size confusions. >>>>>> +/// >>>>>> +// Confidence: Moderate >>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>>> +// URL: http://coccinelle.lip6.fr/ >>>>>> +// Comments: >>>>>> +// Options: -no_includes -include_headers >>>>> >>>>> The options could be: --no-include --include-headers >>>>> >>>>> Actually, Coccinelle supports both, but it only officially supports the >>>>> -- versions. >>>>> >>>>>> + >>>>>> +virtual report >>>>>> +virtual org >>>>> >>>>> Add, the following for the *s: >>>>> >>>>> virtual context >>>>> >>>>> Then add the following rule: >>>>> >>>>> @ok@ >>>>> position p; >>>>> expression src,dest; >>>>> @@ >>>>> >>>>> copy_from_user@p(&dest, src, sizeof(dest)) >>>>> >>>>>> + >>>>>> +@cfu_twice@ >>>>>> +position p; >>>>> >>>>> Change this to: >>>>> >>>>> position p != ok.p; >>>>> >>>>>> +identifier src; >>>>>> +expression dest1, dest2, size1, size2, offset; >>>>>> +@@ >>>>>> + >>>>>> +*copy_from_user(dest1, src, size1) >>>>>> + ... when != src = offset >>>>>> + when != src += offset >>>> >>>> Here, may be we should add few more lines from Pengfei's >>>> script to avoid th potential FPs. >>>> >>>>> Add the following lines: >>>>> >>>>> when != if (size2 > e1 || ...) { ... return ...; } >>>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>>> >>>>> These changes drop cases where the last argument to copy_from_usr is the >>>>> size of the first argument, which seems safe enough, and where there is a >>>>> test on the size value that can either update it or abort the function. >>>>> These changes only eliminate false positives, as far as I could tell. >>>>> >>>>> If it would be more convenient, I could just send the complete revised >>>>> patch, or whatever seems convenient. >>>> >>>> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. >>> >>> I strongly recommend you adding get_user() API , which is used pervasively >>> within the kernel just like copy_from user(). >> >> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to >> include everything in the pattern matching rule. I'll send that as well. >> >>> In many situations, there is a combination use, get_user() copies first then >>> followed by a copy_from_user() copy. According to our investigation, this typical >>> situation works by get_user() firstly copying a field of a specific struct to check, >>> then copy_from_user() copies in the whole struct to use. Of course, the struct >>> field is fetch twice. >> >> Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like >> this: >> >> get_user(..., src.arg) //where src.arg = field of a structure >> ... >> copy_from_user(..., src, ...) //where src is a whole structure >> >> If that is the case then we would need to have one more new script >> or rule for such kind of combinational patterns. Disjunction can >> probably give FPs. > > Yes, I’ve seen these cases when examining the source code. Actually, copying a field > first and then copying the whole struct is very common in the kernel especially the driver. > For example, when a struct (or a message as we call it) is variable length, the first copy is > used to check its size field, and allocate a kernel buffer based on it, then the second copy is > to copy the whole message also based on the size. There are also situations of the > variable type messages. > > The reason that they use get_user() instead of copy_from_user() for the first copy is because > get_user() is defined as a macro, which works faster than a function call that copy_from_user() does > when copy simple data type such as char and int. I see. If possible, can you point me to a code or actual bug [reported by you or others] which has this kind of pattern particularly? I wrote a separate rule for the kind of pattern you have described but I am not sure if this kind of code is suspicious. Like you said, it is very common to use this pattern in drivers. So may be suspicious one can have a specific pattern for this combinational usage of get_user and copy_from_user. Thanks. > > Regards > Pengfei > > >> Thanks! >> >>> Regards >>> Pengfei >>>> >>>>> thanks, >>>>> julia >>>>> >>>>>> +*copy_from_user@p(dest2, src, size2) >>>>>> + >>>>>> +@script:python depends on org@ >>>>>> +p << cfu_twice.p; >>>>>> +@@ >>>>>> + >>>>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>>>> + >>>>>> +@script:python depends on report@ >>>>>> +p << cfu_twice.p; >>>>>> +@@ >>>>>> + >>>>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>>>> -- >>>>>> 2.6.3 >>>>>> >>>>>> >>>>>> -- >>>>>> Kees Cook >>>>>> Chrome OS & Brillo Security >>>>>> >>>>> _______________________________________________ >>>>> Cocci mailing list >>>>> Cocci@systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> <mailto:Cocci@systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr>> >>>>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>> > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-10 17:46 ` Vaishali Thakkar 0 siblings, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-10 17:46 UTC (permalink / raw) To: cocci On Tuesday 10 January 2017 02:32 PM, Pengfei Wang wrote: > >> ? 2017?1?10????4:40?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >> >> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >>> >>>> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >>>> >>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>>> resurrecting it. >>>>> >>>>> Some changes are suggested below. >>>>> >>>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>>> >>>>>> This is usually a sign of a resized request. This adds a check for >>>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>>> needs some manual review. >>>>>> >>>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>>> --- >>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 36 insertions(+) >>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>>> >>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>>>> new file mode 100644 >>>>>> index 000000000000..53645de8ae95 >>>>>> --- /dev/null >>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>>> @@ -0,0 +1,36 @@ >>>>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>>> +/// structure. If the structure's size is not re-validated, this can lead >>>>>> +/// to structure or data size confusions. >>>>>> +/// >>>>>> +// Confidence: Moderate >>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>>> +// URL: http://coccinelle.lip6.fr/ >>>>>> +// Comments: >>>>>> +// Options: -no_includes -include_headers >>>>> >>>>> The options could be: --no-include --include-headers >>>>> >>>>> Actually, Coccinelle supports both, but it only officially supports the >>>>> -- versions. >>>>> >>>>>> + >>>>>> +virtual report >>>>>> +virtual org >>>>> >>>>> Add, the following for the *s: >>>>> >>>>> virtual context >>>>> >>>>> Then add the following rule: >>>>> >>>>> @ok@ >>>>> position p; >>>>> expression src,dest; >>>>> @@ >>>>> >>>>> copy_from_user at p(&dest, src, sizeof(dest)) >>>>> >>>>>> + >>>>>> + at cfu_twice@ >>>>>> +position p; >>>>> >>>>> Change this to: >>>>> >>>>> position p != ok.p; >>>>> >>>>>> +identifier src; >>>>>> +expression dest1, dest2, size1, size2, offset; >>>>>> +@@ >>>>>> + >>>>>> +*copy_from_user(dest1, src, size1) >>>>>> + ... when != src = offset >>>>>> + when != src += offset >>>> >>>> Here, may be we should add few more lines from Pengfei's >>>> script to avoid th potential FPs. >>>> >>>>> Add the following lines: >>>>> >>>>> when != if (size2 > e1 || ...) { ... return ...; } >>>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>>> >>>>> These changes drop cases where the last argument to copy_from_usr is the >>>>> size of the first argument, which seems safe enough, and where there is a >>>>> test on the size value that can either update it or abort the function. >>>>> These changes only eliminate false positives, as far as I could tell. >>>>> >>>>> If it would be more convenient, I could just send the complete revised >>>>> patch, or whatever seems convenient. >>>> >>>> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. >>> >>> I strongly recommend you adding get_user() API , which is used pervasively >>> within the kernel just like copy_from user(). >> >> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to >> include everything in the pattern matching rule. I'll send that as well. >> >>> In many situations, there is a combination use, get_user() copies first then >>> followed by a copy_from_user() copy. According to our investigation, this typical >>> situation works by get_user() firstly copying a field of a specific struct to check, >>> then copy_from_user() copies in the whole struct to use. Of course, the struct >>> field is fetch twice. >> >> Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like >> this: >> >> get_user(..., src.arg) //where src.arg = field of a structure >> ... >> copy_from_user(..., src, ...) //where src is a whole structure >> >> If that is the case then we would need to have one more new script >> or rule for such kind of combinational patterns. Disjunction can >> probably give FPs. > > Yes, I?ve seen these cases when examining the source code. Actually, copying a field > first and then copying the whole struct is very common in the kernel especially the driver. > For example, when a struct (or a message as we call it) is variable length, the first copy is > used to check its size field, and allocate a kernel buffer based on it, then the second copy is > to copy the whole message also based on the size. There are also situations of the > variable type messages. > > The reason that they use get_user() instead of copy_from_user() for the first copy is because > get_user() is defined as a macro, which works faster than a function call that copy_from_user() does > when copy simple data type such as char and int. I see. If possible, can you point me to a code or actual bug [reported by you or others] which has this kind of pattern particularly? I wrote a separate rule for the kind of pattern you have described but I am not sure if this kind of code is suspicious. Like you said, it is very common to use this pattern in drivers. So may be suspicious one can have a specific pattern for this combinational usage of get_user and copy_from_user. Thanks. > > Regards > Pengfei > > >> Thanks! >> >>> Regards >>> Pengfei >>>> >>>>> thanks, >>>>> julia >>>>> >>>>>> +*copy_from_user at p(dest2, src, size2) >>>>>> + >>>>>> + at script:python depends on org@ >>>>>> +p << cfu_twice.p; >>>>>> +@@ >>>>>> + >>>>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>>>> + >>>>>> + at script:python depends on report@ >>>>>> +p << cfu_twice.p; >>>>>> +@@ >>>>>> + >>>>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>>>> -- >>>>>> 2.6.3 >>>>>> >>>>>> >>>>>> -- >>>>>> Kees Cook >>>>>> Chrome OS & Brillo Security >>>>>> >>>>> _______________________________________________ >>>>> Cocci mailing list >>>>> Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> <mailto:Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr>> >>>>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>> > > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 17:46 ` Vaishali Thakkar (?) @ 2017-01-11 2:10 ` Pengfei Wang 2017-01-11 6:10 ` Vaishali Thakkar 2017-01-11 6:12 ` Julia Lawall -1 siblings, 2 replies; 31+ messages in thread From: Pengfei Wang @ 2017-01-11 2:10 UTC (permalink / raw) To: cocci > ? 2017?1?11????1:46?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? > > On Tuesday 10 January 2017 02:32 PM, Pengfei Wang wrote: >> >>> ? 2017?1?10????4:40?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >>> >>> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >>>> >>>>> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >>>>> >>>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>>>> resurrecting it. >>>>>> >>>>>> Some changes are suggested below. >>>>>> >>>>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>>>> >>>>>>> This is usually a sign of a resized request. This adds a check for >>>>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>>>> needs some manual review. >>>>>>> >>>>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>>>> --- >>>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >>>>>>> 1 file changed, 36 insertions(+) >>>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>>>> >>>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >>>>>>> new file mode 100644 >>>>>>> index 000000000000..53645de8ae95 >>>>>>> --- /dev/null >>>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>>>> @@ -0,0 +1,36 @@ >>>>>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>>>> +/// structure. If the structure's size is not re-validated, this can lead >>>>>>> +/// to structure or data size confusions. >>>>>>> +/// >>>>>>> +// Confidence: Moderate >>>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>>>> +// URL: http://coccinelle.lip6.fr/ >>>>>>> +// Comments: >>>>>>> +// Options: -no_includes -include_headers >>>>>> >>>>>> The options could be: --no-include --include-headers >>>>>> >>>>>> Actually, Coccinelle supports both, but it only officially supports the >>>>>> -- versions. >>>>>> >>>>>>> + >>>>>>> +virtual report >>>>>>> +virtual org >>>>>> >>>>>> Add, the following for the *s: >>>>>> >>>>>> virtual context >>>>>> >>>>>> Then add the following rule: >>>>>> >>>>>> @ok@ >>>>>> position p; >>>>>> expression src,dest; >>>>>> @@ >>>>>> >>>>>> copy_from_user at p(&dest, src, sizeof(dest)) >>>>>> >>>>>>> + >>>>>>> + at cfu_twice@ >>>>>>> +position p; >>>>>> >>>>>> Change this to: >>>>>> >>>>>> position p != ok.p; >>>>>> >>>>>>> +identifier src; >>>>>>> +expression dest1, dest2, size1, size2, offset; >>>>>>> +@@ >>>>>>> + >>>>>>> +*copy_from_user(dest1, src, size1) >>>>>>> + ... when != src = offset >>>>>>> + when != src += offset >>>>> >>>>> Here, may be we should add few more lines from Pengfei's >>>>> script to avoid th potential FPs. >>>>> >>>>>> Add the following lines: >>>>>> >>>>>> when != if (size2 > e1 || ...) { ... return ...; } >>>>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>>>> >>>>>> These changes drop cases where the last argument to copy_from_usr is the >>>>>> size of the first argument, which seems safe enough, and where there is a >>>>>> test on the size value that can either update it or abort the function. >>>>>> These changes only eliminate false positives, as far as I could tell. >>>>>> >>>>>> If it would be more convenient, I could just send the complete revised >>>>>> patch, or whatever seems convenient. >>>>> >>>>> I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. >>>> >>>> I strongly recommend you adding get_user() API , which is used pervasively >>>> within the kernel just like copy_from user(). >>> >>> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to >>> include everything in the pattern matching rule. I'll send that as well. >>> >>>> In many situations, there is a combination use, get_user() copies first then >>>> followed by a copy_from_user() copy. According to our investigation, this typical >>>> situation works by get_user() firstly copying a field of a specific struct to check, >>>> then copy_from_user() copies in the whole struct to use. Of course, the struct >>>> field is fetch twice. >>> >>> Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like >>> this: >>> >>> get_user(..., src.arg) //where src.arg = field of a structure >>> ... >>> copy_from_user(..., src, ...) //where src is a whole structure >>> >>> If that is the case then we would need to have one more new script >>> or rule for such kind of combinational patterns. Disjunction can >>> probably give FPs. >> >> Yes, I?ve seen these cases when examining the source code. Actually, copying a field >> first and then copying the whole struct is very common in the kernel especially the driver. >> For example, when a struct (or a message as we call it) is variable length, the first copy is >> used to check its size field, and allocate a kernel buffer based on it, then the second copy is >> to copy the whole message also based on the size. There are also situations of the >> variable type messages. >> >> The reason that they use get_user() instead of copy_from_user() for the first copy is because >> get_user() is defined as a macro, which works faster than a function call that copy_from_user() does >> when copy simple data type such as char and int. > > I see. If possible, can you point me to a code or actual bug > [reported by you or others] which has this kind of pattern > particularly? > > I wrote a separate rule for the kind of pattern you have > described but I am not sure if this kind of code is suspicious. > Like you said, it is very common to use this pattern in drivers. > So may be suspicious one can have a specific pattern for this > combinational usage of get_user and copy_from_user. > > Thanks. > Hi, I attached 4 cases that prove this combinational situation, and all of them come from Linux kernel-4.5 linux-4.5/drivers/vhost/vhost.c : line746 and 764 linux-4.5/sound/pci/asihpi/hpioctl.c line 130 and 139 linux-4.5/arch/sparc/kernel/signal_32.c line 86 and 94 linux-4.5/drivers/scsi/sg.c line 624 and 664 Even though these are not buggy ones because the double-fetched data is not used again, they still prove that the combinational usage of get_user() and copy_from_user() (or _get_user() and __copy_from_user()) exists. These situations could turn into buggy ones when the code is updated by a different developer who accidentally reuses the double-fetched data. Hope this helps, thanks. Regards Pengfei >> >> Regards >> Pengfei >> >> >>> Thanks! >>> >>>> Regards >>>> Pengfei >>>>> >>>>>> thanks, >>>>>> julia >>>>>> >>>>>>> +*copy_from_user at p(dest2, src, size2) >>>>>>> + >>>>>>> + at script:python depends on org@ >>>>>>> +p << cfu_twice.p; >>>>>>> +@@ >>>>>>> + >>>>>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>>>>> + >>>>>>> + at script:python depends on report@ >>>>>>> +p << cfu_twice.p; >>>>>>> +@@ >>>>>>> + >>>>>>> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >>>>>>> -- >>>>>>> 2.6.3 >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Kees Cook >>>>>>> Chrome OS & Brillo Security >>>>>>> >>>>>> _______________________________________________ >>>>>> Cocci mailing list >>>>>> Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> <mailto:Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr>> <mailto:Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr> <mailto:Cocci at systeme.lip6.fr <mailto:Cocci@systeme.lip6.fr>>> >>>>>> https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>>> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0005.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: sg.c Type: application/octet-stream Size: 74285 bytes Desc: not available URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0004.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0006.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: signal_32.c Type: application/octet-stream Size: 15227 bytes Desc: not available URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0005.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0007.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: hpioctl.c Type: application/octet-stream Size: 15439 bytes Desc: not available URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0006.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0008.html> -------------- next part -------------- A non-text attachment was scrubbed... Name: vhost.c Type: application/octet-stream Size: 43724 bytes Desc: not available URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0007.obj> -------------- next part -------------- An HTML attachment was scrubbed... URL: <https://systeme.lip6.fr/pipermail/cocci/attachments/20170111/e3cc5c06/attachment-0009.html> ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-11 2:10 ` Pengfei Wang @ 2017-01-11 6:10 ` Vaishali Thakkar 2017-01-11 6:12 ` Julia Lawall 1 sibling, 0 replies; 31+ messages in thread From: Vaishali Thakkar @ 2017-01-11 6:10 UTC (permalink / raw) To: cocci On Wednesday 11 January 2017 07:40 AM, Pengfei Wang wrote: > >> ? 2017?1?11????1:46?Vaishali Thakkar <vaishali.thakkar@oracle.com >> <mailto:vaishali.thakkar@oracle.com>> ??? >> >> On Tuesday 10 January 2017 02:32 PM, Pengfei Wang wrote: >>> >>>> ? 2017?1?10????4:40?Vaishali Thakkar <vaishali.thakkar@oracle.com >>>> <mailto:vaishali.thakkar@oracle.com>> ??? >>>> >>>> On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >>>>> >>>>>> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com >>>>>> <mailto:vaishali.thakkar@oracle.com>> ??? >>>>>> >>>>>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>>>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>>>>> resurrecting it. >>>>>>> >>>>>>> Some changes are suggested below. >>>>>>> >>>>>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>>>>> >>>>>>>> This is usually a sign of a resized request. This adds a check for >>>>>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>>>>> needs some manual review. >>>>>>>> >>>>>>>> Signed-off-by: Kees Cook <keescook@chromium.org >>>>>>>> <mailto:keescook@chromium.org>> >>>>>>>> --- >>>>>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 >>>>>>>> +++++++++++++++++++++++++++++++ >>>>>>>> 1 file changed, 36 insertions(+) >>>>>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>>>>> >>>>>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci >>>>>>>> b/scripts/coccinelle/tests/reusercopy.cocci >>>>>>>> new file mode 100644 >>>>>>>> index 000000000000..53645de8ae95 >>>>>>>> --- /dev/null >>>>>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>>>>> @@ -0,0 +1,36 @@ >>>>>>>> +/// Recopying from the same user buffer frequently indicates a pattern of >>>>>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>>>>> +/// structure. If the structure's size is not re-validated, this can lead >>>>>>>> +/// to structure or data size confusions. >>>>>>>> +/// >>>>>>>> +// Confidence: Moderate >>>>>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>>>>> +// URL: http://coccinelle.lip6.fr/ >>>>>>>> +// Comments: >>>>>>>> +// Options: -no_includes -include_headers >>>>>>> >>>>>>> The options could be: --no-include --include-headers >>>>>>> >>>>>>> Actually, Coccinelle supports both, but it only officially supports the >>>>>>> -- versions. >>>>>>> >>>>>>>> + >>>>>>>> +virtual report >>>>>>>> +virtual org >>>>>>> >>>>>>> Add, the following for the *s: >>>>>>> >>>>>>> virtual context >>>>>>> >>>>>>> Then add the following rule: >>>>>>> >>>>>>> @ok@ >>>>>>> position p; >>>>>>> expression src,dest; >>>>>>> @@ >>>>>>> >>>>>>> copy_from_user at p(&dest, src, sizeof(dest)) >>>>>>> >>>>>>>> + >>>>>>>> + at cfu_twice@ >>>>>>>> +position p; >>>>>>> >>>>>>> Change this to: >>>>>>> >>>>>>> position p != ok.p; >>>>>>> >>>>>>>> +identifier src; >>>>>>>> +expression dest1, dest2, size1, size2, offset; >>>>>>>> +@@ >>>>>>>> + >>>>>>>> +*copy_from_user(dest1, src, size1) >>>>>>>> + ... when != src = offset >>>>>>>> + when != src += offset >>>>>> >>>>>> Here, may be we should add few more lines from Pengfei's >>>>>> script to avoid th potential FPs. >>>>>> >>>>>>> Add the following lines: >>>>>>> >>>>>>> when != if (size2 > e1 || ...) { ... return ...; } >>>>>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>>>>> >>>>>>> These changes drop cases where the last argument to copy_from_usr is the >>>>>>> size of the first argument, which seems safe enough, and where there is a >>>>>>> test on the size value that can either update it or abort the function. >>>>>>> These changes only eliminate false positives, as far as I could tell. >>>>>>> >>>>>>> If it would be more convenient, I could just send the complete revised >>>>>>> patch, or whatever seems convenient. >>>>>> >>>>>> I was also thinking that probably we should also add other user space >>>>>> memory API functions. May be get_user and strncpy_from_user. Although I'm >>>>>> not sure how common it is to find such patterns for both of these functions. >>>>> >>>>> I strongly recommend you adding get_user() API , which is used pervasively >>>>> within the kernel just like copy_from user(). >>>> >>>> Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to >>>> include everything in the pattern matching rule. I'll send that as well. >>>> >>>>> In many situations, there is a combination use, get_user() copies first then >>>>> followed by a copy_from_user() copy. According to our investigation, this >>>>> typical >>>>> situation works by get_user() firstly copying a field of a specific struct >>>>> to check, >>>>> then copy_from_user() copies in the whole struct to use. Of course, the struct >>>>> field is fetch twice. >>>> >>>> Do you mean that there is a problem when we have get_user() followed by >>>> copy_from_user()? Basically something like >>>> this: >>>> >>>> get_user(..., src.arg) //where src.arg = field of a structure >>>> ... >>>> copy_from_user(..., src, ...) //where src is a whole structure >>>> >>>> If that is the case then we would need to have one more new script >>>> or rule for such kind of combinational patterns. Disjunction can >>>> probably give FPs. >>> >>> Yes, I?ve seen these cases when examining the source code. Actually, copying >>> a field >>> first and then copying the whole struct is very common in the kernel >>> especially the driver. >>> For example, when a struct (or a message as we call it) is variable length, >>> the first copy is >>> used to check its size field, and allocate a kernel buffer based on it, then >>> the second copy is >>> to copy the whole message also based on the size. There are also situations >>> of the >>> variable type messages. >>> >>> The reason that they use get_user() instead of copy_from_user() for the first >>> copy is because >>> get_user() is defined as a macro, which works faster than a function call >>> that copy_from_user() does >>> when copy simple data type such as char and int. >> >> I see. If possible, can you point me to a code or actual bug >> [reported by you or others] which has this kind of pattern >> particularly? >> >> I wrote a separate rule for the kind of pattern you have >> described but I am not sure if this kind of code is suspicious. >> Like you said, it is very common to use this pattern in drivers. >> So may be suspicious one can have a specific pattern for this >> combinational usage of get_user and copy_from_user. >> >> Thanks. >> > > Hi, > > I attached 4 cases that prove this combinational situation, and all of them come > from Linux kernel-4.5 > > linux-4.5/drivers/vhost/vhost.c : line746 and 764 > linux-4.5/sound/pci/asihpi/hpioctl.c line 130 and 139 > linux-4.5/arch/sparc/kernel/signal_32.c line 86 and 94 > linux-4.5/drivers/scsi/sg.c line 624 and 664 > > Even though these are not buggy ones because the double-fetched data is not used > again, they still prove > that the combinational usage of get_user() and copy_from_user() (or _get_user() > and __copy_from_user()) > exists. These situations could turn into buggy ones when the code is updated by > a different developer who > accidentally reuses the double-fetched data. Ok, thanks. > Hope this helps, thanks. > > Regards > Pengfei > > > > > > > > > > > >>> >>> Regards >>> Pengfei >>> >>> >>>> Thanks! >>>> >>>>> Regards >>>>> Pengfei >>>>>> >>>>>>> thanks, >>>>>>> julia >>>>>>> >>>>>>>> +*copy_from_user at p(dest2, src, size2) >>>>>>>> + >>>>>>>> + at script:python depends on org@ >>>>>>>> +p << cfu_twice.p; >>>>>>>> +@@ >>>>>>>> + >>>>>>>> +cocci.print_main("potentially dangerous second copy_from_user()",p) >>>>>>>> + >>>>>>>> + at script:python depends on report@ >>>>>>>> +p << cfu_twice.p; >>>>>>>> +@@ >>>>>>>> + >>>>>>>> +coccilib.report.print_report(p[0],"potentially dangerous second >>>>>>>> copy_from_user()") >>>>>>>> -- >>>>>>>> 2.6.3 >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Kees Cook >>>>>>>> Chrome OS & Brillo Security >>>>>>>> >>>>>>> _______________________________________________ >>>>>>> Cocci mailing list >>>>>>> Cocci at systeme.lip6.fr >>>>>>> <mailto:Cocci@systeme.lip6.fr><mailto:Cocci@systeme.lip6.fr> >>>>>>> <mailto:Cocci at systeme.lip6.fr<mailto:Cocci@systeme.lip6.fr>> >>>>>>> https://systeme.lip6.fr/mailman/listinfo/cocci<https://systeme.lip6.fr/mailman/listinfo/cocci> >>>>>>> <https://systeme.lip6.fr/mailman/listinfo/cocci<https://systeme.lip6.fr/mailman/listinfo/cocci>> > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-11 2:10 ` Pengfei Wang @ 2017-01-11 6:12 ` Julia Lawall 2017-01-11 6:12 ` Julia Lawall 1 sibling, 0 replies; 31+ messages in thread From: Julia Lawall @ 2017-01-11 6:12 UTC (permalink / raw) To: Pengfei Wang Cc: Vaishali Thakkar, Kees Cook, Vaishali Thakkar, linux-kernel, Michal Marek, cocci I looked at the get_user part of the original script. It looks like most of the complexity is to deal with the possibility of the src location being expressed in two different ways between the two calls. Even if this happens in practice only for get_user, it would seem that it could happen for copy_from_user as well. So I think we could just throw both get_user and copy_from_user into the same rule? I'm also not sure to understand why there are cases for things like get_user(exp1, src->f1) ... get_user(exp2,src) Can this happen? The types seem wrong. Likewise, I see the need to take into account a second argument of src++, but not the need to take into account a second argument of src+4. Either there is src+4 in both calls or the addresses involved are just different. Perhaps I'm missing something, though. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-11 6:12 ` Julia Lawall 0 siblings, 0 replies; 31+ messages in thread From: Julia Lawall @ 2017-01-11 6:12 UTC (permalink / raw) To: cocci I looked at the get_user part of the original script. It looks like most of the complexity is to deal with the possibility of the src location being expressed in two different ways between the two calls. Even if this happens in practice only for get_user, it would seem that it could happen for copy_from_user as well. So I think we could just throw both get_user and copy_from_user into the same rule? I'm also not sure to understand why there are cases for things like get_user(exp1, src->f1) ... get_user(exp2,src) Can this happen? The types seem wrong. Likewise, I see the need to take into account a second argument of src++, but not the need to take into account a second argument of src+4. Either there is src+4 in both calls or the addresses involved are just different. Perhaps I'm missing something, though. julia ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-11 6:12 ` Julia Lawall @ 2017-01-11 13:44 ` Pengfei Wang -1 siblings, 0 replies; 31+ messages in thread From: Pengfei Wang @ 2017-01-11 13:44 UTC (permalink / raw) To: Julia Lawall Cc: Vaishali Thakkar, Kees Cook, Vaishali Thakkar, linux-kernel, Michal Marek, cocci > 在 2017年1月11日,下午2:12,Julia Lawall <julia.lawall@lip6.fr> 写道: > > I looked at the get_user part of the original script. It looks like most > of the complexity is to deal with the possibility of the src location > being expressed in two different ways between the two calls. Yes, in addition to pointer alias and the “field-whole” double-fetch type as we mentioned previously, we also need to take into consideration of the explicit data type conversion of the src pointer, such as: get_user(dst, src) … get_user(dst, (int*)src) Also the embedded computation at the argument position, such as get_user(dst, ++src) , get_user(dst, align(src)), or get_user(dst, src[i]), which could cause false positives. Loops also cause false positives. > Even if this > happens in practice only for get_user, it would seem that it could happen > for copy_from_user as well. So I think we could just throw both get_user > and copy_from_user into the same rule? > Agreed. > I'm also not sure to understand why there are cases for things like > > get_user(exp1, src->f1) > ... > get_user(exp2,src) > > Can this happen? The types seem wrong. I think it is unreasonable. It doesn’t work in practice. It exists in my script because I combined different situations with disjunction but forgot to remove the infeasible ones. Please remove it. A practical one should be: get_user(exp1, src->f1) … copy_from_user(exp2, src ,size) > Likewise, I see the need to take into account a second argument of src++, > but not the need to take into account a second argument of src+4. Either > there is src+4 in both calls or the addresses involved are just different. src++ or src+4 are used when handling long messages byte by byte or word by word by means of a loop. I paid attention to these because they cause false positives as src pointers have changed for the double fetches. I remember both of these two situations when examining the source code but I cannot guarantee the src+4 situation exists as I don’t have an example in hand now. I suggest we focus on the src++ for now. If src+4 cause any false positives, we’ll add it, too. Regards Pengfei > > Perhaps I'm missing something, though. > > julia > ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-11 13:44 ` Pengfei Wang 0 siblings, 0 replies; 31+ messages in thread From: Pengfei Wang @ 2017-01-11 13:44 UTC (permalink / raw) To: cocci > ? 2017?1?11????2:12?Julia Lawall <julia.lawall@lip6.fr> ??? > > I looked at the get_user part of the original script. It looks like most > of the complexity is to deal with the possibility of the src location > being expressed in two different ways between the two calls. Yes, in addition to pointer alias and the ?field-whole? double-fetch type as we mentioned previously, we also need to take into consideration of the explicit data type conversion of the src pointer, such as: get_user(dst, src) ? get_user(dst, (int*)src) Also the embedded computation at the argument position, such as get_user(dst, ++src) , get_user(dst, align(src)), or get_user(dst, src[i]), which could cause false positives. Loops also cause false positives. > Even if this > happens in practice only for get_user, it would seem that it could happen > for copy_from_user as well. So I think we could just throw both get_user > and copy_from_user into the same rule? > Agreed. > I'm also not sure to understand why there are cases for things like > > get_user(exp1, src->f1) > ... > get_user(exp2,src) > > Can this happen? The types seem wrong. I think it is unreasonable. It doesn?t work in practice. It exists in my script because I combined different situations with disjunction but forgot to remove the infeasible ones. Please remove it. A practical one should be: get_user(exp1, src->f1) ? copy_from_user(exp2, src ,size) > Likewise, I see the need to take into account a second argument of src++, > but not the need to take into account a second argument of src+4. Either > there is src+4 in both calls or the addresses involved are just different. src++ or src+4 are used when handling long messages byte by byte or word by word by means of a loop. I paid attention to these because they cause false positives as src pointers have changed for the double fetches. I remember both of these two situations when examining the source code but I cannot guarantee the src+4 situation exists as I don?t have an example in hand now. I suggest we focus on the src++ for now. If src+4 cause any false positives, we?ll add it, too. Regards Pengfei > > Perhaps I'm missing something, though. > > julia > ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 8:40 ` Vaishali Thakkar @ 2017-01-10 19:16 ` Kees Cook -1 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-10 19:16 UTC (permalink / raw) To: Vaishali Thakkar Cc: Pengfei Wang, Julia Lawall, Vaishali Thakkar, LKML, Michal Marek, cocci On Tue, Jan 10, 2017 at 12:40 AM, Vaishali Thakkar <vaishali.thakkar@oracle.com> wrote: > On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >> >> >>> 在 2017年1月10日,上午1:05,Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道: >>> >>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>> >>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>> resurrecting it. >>>> >>>> Some changes are suggested below. >>>> >>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>> >>>>> This is usually a sign of a resized request. This adds a check for >>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>> needs some manual review. >>>>> >>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>> --- >>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 >>>>> +++++++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>> >>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci >>>>> b/scripts/coccinelle/tests/reusercopy.cocci >>>>> new file mode 100644 >>>>> index 000000000000..53645de8ae95 >>>>> --- /dev/null >>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>> @@ -0,0 +1,36 @@ >>>>> +/// Recopying from the same user buffer frequently indicates a pattern >>>>> of >>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>> +/// structure. If the structure's size is not re-validated, this can >>>>> lead >>>>> +/// to structure or data size confusions. >>>>> +/// >>>>> +// Confidence: Moderate >>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>> +// URL: http://coccinelle.lip6.fr/ >>>>> +// Comments: >>>>> +// Options: -no_includes -include_headers >>>> >>>> >>>> The options could be: --no-include --include-headers >>>> >>>> Actually, Coccinelle supports both, but it only officially supports the >>>> -- versions. >>>> >>>>> + >>>>> +virtual report >>>>> +virtual org >>>> >>>> >>>> Add, the following for the *s: >>>> >>>> virtual context >>>> >>>> Then add the following rule: >>>> >>>> @ok@ >>>> position p; >>>> expression src,dest; >>>> @@ >>>> >>>> copy_from_user@p(&dest, src, sizeof(dest)) >>>> >>>>> + >>>>> +@cfu_twice@ >>>>> +position p; >>>> >>>> >>>> Change this to: >>>> >>>> position p != ok.p; >>>> >>>>> +identifier src; >>>>> +expression dest1, dest2, size1, size2, offset; >>>>> +@@ >>>>> + >>>>> +*copy_from_user(dest1, src, size1) >>>>> + ... when != src = offset >>>>> + when != src += offset >>> >>> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >>> >>>> Add the following lines: >>>> >>>> when != if (size2 > e1 || ...) { ... return ...; } >>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>> >>>> These changes drop cases where the last argument to copy_from_usr is the >>>> size of the first argument, which seems safe enough, and where there is >>>> a >>>> test on the size value that can either update it or abort the function. >>>> These changes only eliminate false positives, as far as I could tell. >>>> >>>> If it would be more convenient, I could just send the complete revised >>>> patch, or whatever seems convenient. >>> >>> >>> I was also thinking that probably we should also add other user space >>> memory API functions. May be get_user and strncpy_from_user. Although I'm >>> not sure how common it is to find such patterns for both of these functions. >> >> >> I strongly recommend you adding get_user() API , which is used pervasively >> within the kernel just like copy_from user(). > > > Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to > include everything in the pattern matching rule. I'll send that as well. > >> In many situations, there is a combination use, get_user() copies first >> then >> followed by a copy_from_user() copy. According to our investigation, this >> typical >> situation works by get_user() firstly copying a field of a specific struct >> to check, >> then copy_from_user() copies in the whole struct to use. Of course, the >> struct >> field is fetch twice. > > > Do you mean that there is a problem when we have get_user() followed by > copy_from_user()? Basically something like > this: > > get_user(..., src.arg) //where src.arg = field of a structure > ... > copy_from_user(..., src, ...) //where src is a whole structure > > If that is the case then we would need to have one more new script > or rule for such kind of combinational patterns. Disjunction can > probably give FPs. Yup, we need a single script: I just split them into three for comparisons. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-10 19:16 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-10 19:16 UTC (permalink / raw) To: cocci On Tue, Jan 10, 2017 at 12:40 AM, Vaishali Thakkar <vaishali.thakkar@oracle.com> wrote: > On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote: >> >> >>> ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? >>> >>> On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: >>>> >>>> I totally dropped the ball on this. Many thanks to Vaishali for >>>> resurrecting it. >>>> >>>> Some changes are suggested below. >>>> >>>> On Tue, 26 Apr 2016, Kees Cook wrote: >>>> >>>>> This is usually a sign of a resized request. This adds a check for >>>>> potential races or confusions. The check isn't 100% accurate, so it >>>>> needs some manual review. >>>>> >>>>> Signed-off-by: Kees Cook <keescook@chromium.org> >>>>> --- >>>>> scripts/coccinelle/tests/reusercopy.cocci | 36 >>>>> +++++++++++++++++++++++++++++++ >>>>> 1 file changed, 36 insertions(+) >>>>> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >>>>> >>>>> diff --git a/scripts/coccinelle/tests/reusercopy.cocci >>>>> b/scripts/coccinelle/tests/reusercopy.cocci >>>>> new file mode 100644 >>>>> index 000000000000..53645de8ae95 >>>>> --- /dev/null >>>>> +++ b/scripts/coccinelle/tests/reusercopy.cocci >>>>> @@ -0,0 +1,36 @@ >>>>> +/// Recopying from the same user buffer frequently indicates a pattern >>>>> of >>>>> +/// Reading a size header, allocating, and then re-reading an entire >>>>> +/// structure. If the structure's size is not re-validated, this can >>>>> lead >>>>> +/// to structure or data size confusions. >>>>> +/// >>>>> +// Confidence: Moderate >>>>> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >>>>> +// URL: http://coccinelle.lip6.fr/ >>>>> +// Comments: >>>>> +// Options: -no_includes -include_headers >>>> >>>> >>>> The options could be: --no-include --include-headers >>>> >>>> Actually, Coccinelle supports both, but it only officially supports the >>>> -- versions. >>>> >>>>> + >>>>> +virtual report >>>>> +virtual org >>>> >>>> >>>> Add, the following for the *s: >>>> >>>> virtual context >>>> >>>> Then add the following rule: >>>> >>>> @ok@ >>>> position p; >>>> expression src,dest; >>>> @@ >>>> >>>> copy_from_user at p(&dest, src, sizeof(dest)) >>>> >>>>> + >>>>> + at cfu_twice@ >>>>> +position p; >>>> >>>> >>>> Change this to: >>>> >>>> position p != ok.p; >>>> >>>>> +identifier src; >>>>> +expression dest1, dest2, size1, size2, offset; >>>>> +@@ >>>>> + >>>>> +*copy_from_user(dest1, src, size1) >>>>> + ... when != src = offset >>>>> + when != src += offset >>> >>> >>> Here, may be we should add few more lines from Pengfei's >>> script to avoid th potential FPs. >>> >>>> Add the following lines: >>>> >>>> when != if (size2 > e1 || ...) { ... return ...; } >>>> when != if (size2 > e1 || ...) { ... size2 = e2 ... } >>>> >>>> These changes drop cases where the last argument to copy_from_usr is the >>>> size of the first argument, which seems safe enough, and where there is >>>> a >>>> test on the size value that can either update it or abort the function. >>>> These changes only eliminate false positives, as far as I could tell. >>>> >>>> If it would be more convenient, I could just send the complete revised >>>> patch, or whatever seems convenient. >>> >>> >>> I was also thinking that probably we should also add other user space >>> memory API functions. May be get_user and strncpy_from_user. Although I'm >>> not sure how common it is to find such patterns for both of these functions. >> >> >> I strongly recommend you adding get_user() API , which is used pervasively >> within the kernel just like copy_from user(). > > > Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to > include everything in the pattern matching rule. I'll send that as well. > >> In many situations, there is a combination use, get_user() copies first >> then >> followed by a copy_from_user() copy. According to our investigation, this >> typical >> situation works by get_user() firstly copying a field of a specific struct >> to check, >> then copy_from_user() copies in the whole struct to use. Of course, the >> struct >> field is fetch twice. > > > Do you mean that there is a problem when we have get_user() followed by > copy_from_user()? Basically something like > this: > > get_user(..., src.arg) //where src.arg = field of a structure > ... > copy_from_user(..., src, ...) //where src is a whole structure > > If that is the case then we would need to have one more new script > or rule for such kind of combinational patterns. Disjunction can > probably give FPs. Yup, we need a single script: I just split them into three for comparisons. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user 2017-01-10 8:21 ` Pengfei Wang @ 2017-01-10 19:15 ` Kees Cook 2017-01-10 19:15 ` Kees Cook 1 sibling, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-10 19:15 UTC (permalink / raw) To: Pengfei Wang Cc: Vaishali Thakkar, Julia Lawall, Vaishali Thakkar, LKML, Michal Marek, cocci On Tue, Jan 10, 2017 at 12:21 AM, Pengfei Wang <wpengfeinudt@gmail.com> wrote: > > 在 2017年1月10日,上午1:05,Vaishali Thakkar <vaishali.thakkar@oracle.com> 写道: > > On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 > +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci > b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers > > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > > + > +virtual report > +virtual org > > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user@p(&dest, src, sizeof(dest)) > > + > +@cfu_twice@ > +position p; > > > Change this to: > > position p != ok.p; > > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset > > > Here, may be we should add few more lines from Pengfei's > script to avoid th potential FPs. > > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. > > > I was also thinking that probably we should also add other user space memory > API functions. May be get_user and strncpy_from_user. Although I'm not sure > how common it is to find such patterns for both of these functions. > > > I strongly recommend you adding get_user() API , which is used pervasively > within the kernel just like copy_from user(). > > In many situations, there is a combination use, get_user() copies first then > followed by a copy_from_user() copy. According to our investigation, this > typical > situation works by get_user() firstly copying a field of a specific struct > to check, > then copy_from_user() copies in the whole struct to use. Of course, the > struct > field is fetch twice. For sure, yes. I just split it out initially so we could compare some of the pieces the two scripts do. Getting the size check into the test was important to reduce false positives, so I think we need to just expand the rules a bit more to include the size checks. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
* [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user @ 2017-01-10 19:15 ` Kees Cook 0 siblings, 0 replies; 31+ messages in thread From: Kees Cook @ 2017-01-10 19:15 UTC (permalink / raw) To: cocci On Tue, Jan 10, 2017 at 12:21 AM, Pengfei Wang <wpengfeinudt@gmail.com> wrote: > > ? 2017?1?10????1:05?Vaishali Thakkar <vaishali.thakkar@oracle.com> ??? > > On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > > This is usually a sign of a resized request. This adds a check for > potential races or confusions. The check isn't 100% accurate, so it > needs some manual review. > > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > scripts/coccinelle/tests/reusercopy.cocci | 36 > +++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > create mode 100644 scripts/coccinelle/tests/reusercopy.cocci > > diff --git a/scripts/coccinelle/tests/reusercopy.cocci > b/scripts/coccinelle/tests/reusercopy.cocci > new file mode 100644 > index 000000000000..53645de8ae95 > --- /dev/null > +++ b/scripts/coccinelle/tests/reusercopy.cocci > @@ -0,0 +1,36 @@ > +/// Recopying from the same user buffer frequently indicates a pattern of > +/// Reading a size header, allocating, and then re-reading an entire > +/// structure. If the structure's size is not re-validated, this can lead > +/// to structure or data size confusions. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Comments: > +// Options: -no_includes -include_headers > > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > > + > +virtual report > +virtual org > > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user at p(&dest, src, sizeof(dest)) > > + > + at cfu_twice@ > +position p; > > > Change this to: > > position p != ok.p; > > +identifier src; > +expression dest1, dest2, size1, size2, offset; > +@@ > + > +*copy_from_user(dest1, src, size1) > + ... when != src = offset > + when != src += offset > > > Here, may be we should add few more lines from Pengfei's > script to avoid th potential FPs. > > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. > > > I was also thinking that probably we should also add other user space memory > API functions. May be get_user and strncpy_from_user. Although I'm not sure > how common it is to find such patterns for both of these functions. > > > I strongly recommend you adding get_user() API , which is used pervasively > within the kernel just like copy_from user(). > > In many situations, there is a combination use, get_user() copies first then > followed by a copy_from_user() copy. According to our investigation, this > typical > situation works by get_user() firstly copying a field of a specific struct > to check, > then copy_from_user() copies in the whole struct to use. Of course, the > struct > field is fetch twice. For sure, yes. I just split it out initially so we could compare some of the pieces the two scripts do. Getting the size check into the test was important to reduce false positives, so I think we need to just expand the rules a bit more to include the size checks. -Kees -- Kees Cook Nexus Security ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2017-01-11 13:44 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-04-26 22:24 [PATCH] coccicheck: add a test for repeat copy_from_user Kees Cook 2016-04-26 22:24 ` [Cocci] " Kees Cook 2016-04-26 22:30 ` Kees Cook 2016-04-26 22:30 ` [Cocci] " Kees Cook 2016-12-27 18:21 ` Julia Lawall 2016-12-27 18:21 ` [Cocci] " Julia Lawall 2017-01-02 15:45 ` Pengfei Wang 2017-01-09 17:05 ` Vaishali Thakkar 2017-01-09 17:05 ` Vaishali Thakkar 2017-01-09 19:08 ` Julia Lawall 2017-01-09 19:08 ` Julia Lawall 2017-01-09 20:56 ` Kees Cook 2017-01-09 20:56 ` Kees Cook 2017-01-09 22:02 ` Kees Cook 2017-01-09 22:02 ` Kees Cook 2017-01-10 8:21 ` Pengfei Wang 2017-01-10 8:40 ` Vaishali Thakkar 2017-01-10 8:40 ` Vaishali Thakkar 2017-01-10 9:02 ` Pengfei Wang 2017-01-10 17:46 ` Vaishali Thakkar 2017-01-10 17:46 ` Vaishali Thakkar 2017-01-11 2:10 ` Pengfei Wang 2017-01-11 6:10 ` Vaishali Thakkar 2017-01-11 6:12 ` Julia Lawall 2017-01-11 6:12 ` Julia Lawall 2017-01-11 13:44 ` Pengfei Wang 2017-01-11 13:44 ` Pengfei Wang 2017-01-10 19:16 ` Kees Cook 2017-01-10 19:16 ` Kees Cook 2017-01-10 19:15 ` Kees Cook 2017-01-10 19:15 ` Kees Cook
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.