linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
@ 2020-06-15 10:20 Denis Efremov
  2020-06-15 18:23 ` Kees Cook
                   ` (5 more replies)
  0 siblings, 6 replies; 28+ messages in thread
From: Denis Efremov @ 2020-06-15 10:20 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Denis Efremov, cocci, linux-kernel, Kees Cook

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
 scripts/coccinelle/misc/array_size_dup.cocci | 347 +++++++++++++++++++
 1 file changed, 347 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index 000000000000..08919a938754
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,347 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the same size
+///  2. An opencoded expression is used after array_size() to compute the same size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+      when != \(E1\|E2\|subE1\|subE2\)+=E3
+      when != \(E1\|E2\|subE1\|subE2\)-=E3
+      when != \(E1\|E2\|subE1\|subE2\)*=E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* array_size(E1, E2)@p2
+
+@as_prev@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+      when != \(E1\|E2\|subE1\|subE2\)+=E3
+      when != \(E1\|E2\|subE1\|subE2\)-=E3
+      when != \(E1\|E2\|subE1\|subE2\)*=E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* E1 * E2@p2
+
+@as_dup@
+expression subE1 <= as.E1;
+expression as.E1;
+expression subE2 <= as.E2;
+expression as.E2;
+expression E3;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\)=E3
+      when != \(E1\|E2\|subE1\|subE2\)+=E3
+      when != \(E1\|E2\|subE1\|subE2\)-=E3
+      when != \(E1\|E2\|subE1\|subE2\)*=E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* array_size(E1, E2)@p2
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* E1 * E2 * E3@p2
+
+@as3_dup@
+expression subE1 <= as3.E1;
+expression as3.E1;
+expression subE2 <= as3.E2;
+expression as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E3;
+expression E4;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@ss@
+expression E1, E2, E3;
+@@
+
+struct_size(E1, E2, E3)
+
+@ss_next@
+expression subE1 <= ss.E1;
+expression ss.E1;
+expression subE2 <= ss.E2;
+expression ss.E2;
+expression subE3 <= ss.E3;
+expression ss.E3;
+expression E4;
+position p1, p2;
+@@
+
+* E1 * E2 + E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@ss_prev@
+expression subE1 <= ss.E1;
+expression ss.E1;
+expression subE2 <= ss.E2;
+expression ss.E2;
+expression subE3 <= ss.E3;
+expression ss.E3;
+expression E4;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* E1 * E2 + E3@p2
+
+@ss_dup@
+expression subE1 <= ss.E1;
+expression ss.E1;
+expression subE2 <= ss.E2;
+expression ss.E2;
+expression subE3 <= ss.E3;
+expression ss.E3;
+expression E4;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
+      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@script:python depends on report@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+msg = "WARNING: same array_size (line %s)" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+msg = "WARNING: same array_size (line %s)" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@script:python depends on report@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@script:python depends on report@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+msg = "WARNING: same array3_size (line %s)" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+msg = "WARNING: same array3_size (line %s)" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+
+@script:python depends on report@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+msg = "WARNING: struct_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+msg = "WARNING: struct_size is used down the code (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@script:python depends on report@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@script:python depends on report@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+msg = "WARNING: same struct_size (line %s)" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+msg = "WARNING: same struct_size (line %s)" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
@ 2020-06-15 18:23 ` Kees Cook
  2020-06-15 18:35   ` Denis Efremov
  2020-06-17 18:15 ` Kees Cook
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Kees Cook @ 2020-06-15 18:23 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Julia Lawall, cocci, linux-kernel

On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> Detect an opencoded expression that is used before or after
> array_size()/array3_size()/struct_size() to compute the same size.
> 
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Denis Efremov <efremov@linux.com>

Oh, very cool! How much does this find currently?

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 18:23 ` Kees Cook
@ 2020-06-15 18:35   ` Denis Efremov
  2020-06-15 18:46     ` [Cocci] " Gustavo A. R. Silva
  0 siblings, 1 reply; 28+ messages in thread
From: Denis Efremov @ 2020-06-15 18:35 UTC (permalink / raw)
  To: Kees Cook; +Cc: Julia Lawall, cocci, linux-kernel



On 6/15/20 9:23 PM, Kees Cook wrote:
> On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
>> Detect an opencoded expression that is used before or after
>> array_size()/array3_size()/struct_size() to compute the same size.
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Denis Efremov <efremov@linux.com>
> 
> Oh, very cool! How much does this find currently?
> 

opencoded expression before the function call:
./drivers/net/ethernet/cavium/liquidio/request_manager.c:98:34-59: WARNING: array_size is used down the code (line 103) to compute the same size
./drivers/media/test-drivers/vivid/vivid-core.c:1120:26-34: WARNING: array_size is used down the code (line 1122) to compute the same size
./drivers/scsi/megaraid/megaraid_sas_fusion.c:5184:11-31: WARNING: array_size is used down the code (line 5191) to compute the same size
./drivers/scsi/megaraid/megaraid_sas_fusion.c:5200:2-37: WARNING: array_size is used down the code (line 5207) to compute the same size
./fs/cifs/misc.c:853:17-39: WARNING: array_size is used down the code (line 858) to compute the same size
./fs/cifs/misc.c:863:17-38: WARNING: array_size is used down the code (line 868) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:562:25-48: WARNING: array_size is used down the code (line 566) to compute the same size

opencoded expression after the function call:
./net/ethtool/ioctl.c:1976:55-66: WARNING: array_size is already used (line 1957) to compute the same size
./net/ethtool/ioctl.c:1921:55-66: WARNING: array_size is already used (line 1909) to compute the same size
./drivers/net/ethernet/cavium/liquidio/request_manager.c:111:29-54: WARNING: array_size is already used (line 103) to compute the same size
./drivers/staging/rts5208/ms.c:2309:55-56: WARNING: array_size is already used (line 2305) to compute the same size
./drivers/video/fbdev/core/fbcon.c:642:52-53: WARNING: array3_size is already used (line 638) to compute the same size
./drivers/video/fbdev/core/fbcon.c:679:47-48: WARNING: array3_size is already used (line 638) to compute the same size
./drivers/usb/misc/sisusbvga/sisusb_con.c:1229:54-56: WARNING: array_size is already used (line 1226) to compute the same size
./fs/afs/cmservice.c:271:45-46: WARNING: array3_size is already used (line 267) to compute the same size
./drivers/mtd/ftl.c:270:49-65: WARNING: array_size is already used (line 266) to compute the same size
./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1608:6-42: WARNING: array_size is already used (line 1605) to compute the same size
./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1613:8-44: WARNING: array_size is already used (line 1605) to compute the same size
./drivers/net/ppp/bsd_comp.c:439:13-37: WARNING: array_size is already used (line 409) to compute the same size
./drivers/net/wireless/ath/ath5k/debug.c:957:20-21: WARNING: array_size is already used (line 934) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:575:3-26: WARNING: array_size is already used (line 566) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:592:32-53: WARNING: array_size is already used (line 580) to compute the same size
./drivers/scsi/fnic/fnic_trace.c:504:30-51: WARNING: array_size is already used (line 492) to compute the same size
./drivers/staging/rts5208/rtsx_chip.c:1475:17-18: WARNING: array_size is already used (line 1458) to compute the same size
./kernel/kexec_file.c:917:8-25: WARNING: array_size is already used (line 913) to compute the same size
./drivers/rapidio/devices/rio_mport_cdev.c:984:8-25: WARNING: array_size is already used (line 978) to compute the same size
./fs/reiserfs/bitmap.c:1463:22-37: WARNING: array_size is already used (line 1459) to compute the same size

duplicate calls:
./drivers/media/test-drivers/vivid/vivid-core.c:1125:59-60: WARNING: same array_size (line 1122)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:142:36-37: WARNING: same array_size (line 138)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:129:41-42: WARNING: same array3_size (line 123)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same array3_size (line 123)
./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same array3_size (line 129)
./drivers/net/ethernet/cavium/liquidio/octeon_droq.c:289:27-28: WARNING: same array_size (line 284)
./drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c:857:59-60: WARNING: same struct_size (line 854)
./fs/f2fs/super.c:3478:34-35: WARNING: same array_size (line 3478)
./drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1637:45-46: WARNING: same struct_size (line 1634)
./drivers/net/ethernet/netronome/nfp/flower/cmsg.c:221:49-50: WARNING: same struct_size (line 219)
./drivers/staging/rts5208/rtsx_chip.c:1458:36-37: WARNING: same array_size (line 1454)
./drivers/net/ethernet/neterion/vxge/vxge-config.c:2664:59-60: WARNING: same array_size (line 2654)

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 18:35   ` Denis Efremov
@ 2020-06-15 18:46     ` Gustavo A. R. Silva
  2020-06-17  9:32       ` Denis Efremov
  2020-06-17 10:55       ` Denis Efremov
  0 siblings, 2 replies; 28+ messages in thread
From: Gustavo A. R. Silva @ 2020-06-15 18:46 UTC (permalink / raw)
  To: Denis Efremov, Kees Cook; +Cc: Julia Lawall, cocci, linux-kernel



On 6/15/20 13:35, Denis Efremov wrote:
> 
> 
> On 6/15/20 9:23 PM, Kees Cook wrote:
>> On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
>>> Detect an opencoded expression that is used before or after
>>> array_size()/array3_size()/struct_size() to compute the same size.
>>>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Signed-off-by: Denis Efremov <efremov@linux.com>
>>
>> Oh, very cool! How much does this find currently?
>>
> 
> opencoded expression before the function call:
> ./drivers/net/ethernet/cavium/liquidio/request_manager.c:98:34-59: WARNING: array_size is used down the code (line 103) to compute the same size
> ./drivers/media/test-drivers/vivid/vivid-core.c:1120:26-34: WARNING: array_size is used down the code (line 1122) to compute the same size
> ./drivers/scsi/megaraid/megaraid_sas_fusion.c:5184:11-31: WARNING: array_size is used down the code (line 5191) to compute the same size
> ./drivers/scsi/megaraid/megaraid_sas_fusion.c:5200:2-37: WARNING: array_size is used down the code (line 5207) to compute the same size
> ./fs/cifs/misc.c:853:17-39: WARNING: array_size is used down the code (line 858) to compute the same size
> ./fs/cifs/misc.c:863:17-38: WARNING: array_size is used down the code (line 868) to compute the same size
> ./drivers/scsi/fnic/fnic_trace.c:562:25-48: WARNING: array_size is used down the code (line 566) to compute the same size
> 
> opencoded expression after the function call:
> ./net/ethtool/ioctl.c:1976:55-66: WARNING: array_size is already used (line 1957) to compute the same size
> ./net/ethtool/ioctl.c:1921:55-66: WARNING: array_size is already used (line 1909) to compute the same size
> ./drivers/net/ethernet/cavium/liquidio/request_manager.c:111:29-54: WARNING: array_size is already used (line 103) to compute the same size
> ./drivers/staging/rts5208/ms.c:2309:55-56: WARNING: array_size is already used (line 2305) to compute the same size
> ./drivers/video/fbdev/core/fbcon.c:642:52-53: WARNING: array3_size is already used (line 638) to compute the same size
> ./drivers/video/fbdev/core/fbcon.c:679:47-48: WARNING: array3_size is already used (line 638) to compute the same size
> ./drivers/usb/misc/sisusbvga/sisusb_con.c:1229:54-56: WARNING: array_size is already used (line 1226) to compute the same size
> ./fs/afs/cmservice.c:271:45-46: WARNING: array3_size is already used (line 267) to compute the same size
> ./drivers/mtd/ftl.c:270:49-65: WARNING: array_size is already used (line 266) to compute the same size
> ./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1608:6-42: WARNING: array_size is already used (line 1605) to compute the same size
> ./drivers/scsi/qla2xxx/tcm_qla2xxx.c:1613:8-44: WARNING: array_size is already used (line 1605) to compute the same size
> ./drivers/net/ppp/bsd_comp.c:439:13-37: WARNING: array_size is already used (line 409) to compute the same size
> ./drivers/net/wireless/ath/ath5k/debug.c:957:20-21: WARNING: array_size is already used (line 934) to compute the same size
> ./drivers/scsi/fnic/fnic_trace.c:575:3-26: WARNING: array_size is already used (line 566) to compute the same size
> ./drivers/scsi/fnic/fnic_trace.c:592:32-53: WARNING: array_size is already used (line 580) to compute the same size
> ./drivers/scsi/fnic/fnic_trace.c:504:30-51: WARNING: array_size is already used (line 492) to compute the same size
> ./drivers/staging/rts5208/rtsx_chip.c:1475:17-18: WARNING: array_size is already used (line 1458) to compute the same size
> ./kernel/kexec_file.c:917:8-25: WARNING: array_size is already used (line 913) to compute the same size
> ./drivers/rapidio/devices/rio_mport_cdev.c:984:8-25: WARNING: array_size is already used (line 978) to compute the same size
> ./fs/reiserfs/bitmap.c:1463:22-37: WARNING: array_size is already used (line 1459) to compute the same size
> 
> duplicate calls:
> ./drivers/media/test-drivers/vivid/vivid-core.c:1125:59-60: WARNING: same array_size (line 1122)
> ./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:142:36-37: WARNING: same array_size (line 138)
> ./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:129:41-42: WARNING: same array3_size (line 123)
> ./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same array3_size (line 123)
> ./drivers/media/common/v4l2-tpg/v4l2-tpg-core.c:146:40-41: WARNING: same array3_size (line 129)
> ./drivers/net/ethernet/cavium/liquidio/octeon_droq.c:289:27-28: WARNING: same array_size (line 284)
> ./drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c:857:59-60: WARNING: same struct_size (line 854)
> ./fs/f2fs/super.c:3478:34-35: WARNING: same array_size (line 3478)
> ./drivers/net/wireless/zydas/zd1211rw/zd_usb.c:1637:45-46: WARNING: same struct_size (line 1634)
> ./drivers/net/ethernet/netronome/nfp/flower/cmsg.c:221:49-50: WARNING: same struct_size (line 219)
> ./drivers/staging/rts5208/rtsx_chip.c:1458:36-37: WARNING: same array_size (line 1454)
> ./drivers/net/ethernet/neterion/vxge/vxge-config.c:2664:59-60: WARNING: same array_size (line 2654)


Awesome! I'll take a look into this. :)

Thanks, Denis!
--
Gustavo


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 18:46     ` [Cocci] " Gustavo A. R. Silva
@ 2020-06-17  9:32       ` Denis Efremov
  2020-06-17 10:55       ` Denis Efremov
  1 sibling, 0 replies; 28+ messages in thread
From: Denis Efremov @ 2020-06-17  9:32 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Kees Cook; +Cc: Julia Lawall, cocci, linux-kernel

> 
> 
> Awesome! I'll take a look into this. :)
> 

It would be helpful to get a feedback from you after.
What kind of warnings are helpful and what are not?
"duplicate calls" and "opencoded expression after array_size()" look doubtful to me.
I think that maintainers will not like these patches.

Thanks,
Denis

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 18:46     ` [Cocci] " Gustavo A. R. Silva
  2020-06-17  9:32       ` Denis Efremov
@ 2020-06-17 10:55       ` Denis Efremov
  2020-06-17 20:08         ` Julia Lawall
  1 sibling, 1 reply; 28+ messages in thread
From: Denis Efremov @ 2020-06-17 10:55 UTC (permalink / raw)
  To: Gustavo A. R. Silva, Kees Cook; +Cc: Julia Lawall, cocci, linux-kernel


> 
> Awesome! I'll take a look into this. :)
> 
Here is another script for your #83 ticket.
Currently, it issues 598 warnings.

// SPDX-License-Identifier: GPL-2.0-only
///
/// Check for missing overflow checks in allocation functions.
/// Low confidence because it's pointless to check for overflow
/// relatively small allocations.
///
// Confidence: Low
// Copyright: (C) 2020 Denis Efremov ISPRAS
// Options: --no-includes --include-headers

virtual patch
virtual context
virtual org
virtual report

@depends on patch@
expression E1, E2, E3, E4, size;
@@

(
- size = E1 * E2;
+ size = array_size(E1, E2);
|
- size = E1 * E2 * E3;
+ size = array3_size(E1, E2, E3);
|
- size = E1 * E2 + E3;
+ size = struct_size(E1, E2, E3);
)
  ... when != size = E4
      when != size += E4
      when != size -= E4
      when != size *= E4
      when != &size
  \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
    vmalloc\|vzalloc\|vzalloc_node\|
    kvmalloc\|kvzalloc\|kvzalloc_node\|
    sock_kmalloc\|
    f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
    devm_kmalloc\|devm_kzalloc\)
  (..., size, ...)

@r depends on !patch@
expression E1, E2, E3, E4, size;
position p;
@@

(
* size = E1 * E2;@p
|
* size = E1 * E2 * E3;@p
|
* size = E1 * E2 + E3;@p
)
  ... when != size = E4
      when != size += E4
      when != size -= E4
      when != size *= E4
      when != &size
* \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
    vmalloc\|vzalloc\|vzalloc_node\|
    kvmalloc\|kvzalloc\|kvzalloc_node\|
    sock_kmalloc\|
    f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
    devm_kmalloc\|devm_kzalloc\)
  (..., size, ...)

@script:python depends on report@
p << r.p;
@@

coccilib.report.print_report(p[0], "WARNING: missing overflow check")

@script:python depends on org@
p << r.p;
@@

coccilib.org.print_todo(p[0], "WARNING: missing overflow check")

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
  2020-06-15 18:23 ` Kees Cook
@ 2020-06-17 18:15 ` Kees Cook
  2020-06-17 18:54   ` [Cocci] " Julia Lawall
  2020-06-17 20:30 ` Julia Lawall
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Kees Cook @ 2020-06-17 18:15 UTC (permalink / raw)
  To: Julia Lawall; +Cc: cocci, linux-kernel, Denis Efremov, Gustavo A. R. Silva

On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> +@as@
> +expression E1, E2;
> +@@
> +
> +array_size(E1, E2)

BTW, is there a way yet in Coccinelle to match a fully qualified (?)
identifier? For example, if I have two lines in C:

A)
	array_size(variable, 5);
B)
	array_size(instance->member.size, 5);
C)
	array_size(instance->member.size + 1, 5);
D)
	array_size(function_call(variable), 5);


This matches A, B, C, and D:

@@
expression ARG1;
expression ARG2;
@@

array_size(ARG1, ARG2);


This matches only A:

@@
identifier ARG1;
expression ARG2;
@@

array_size(ARG1, ARG2);


How do I get something to match A and B but not C and D (i.e. I do not
want to match any operations, function calls, etc, only a variable,
which may be identified through dereference, array index, or struct
member access.)


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 18:15 ` Kees Cook
@ 2020-06-17 18:54   ` Julia Lawall
  2020-06-18 19:52     ` Kees Cook
  0 siblings, 1 reply; 28+ messages in thread
From: Julia Lawall @ 2020-06-17 18:54 UTC (permalink / raw)
  To: Kees Cook; +Cc: Julia Lawall, cocci, linux-kernel



On Wed, 17 Jun 2020, Kees Cook wrote:

> On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> > +@as@
> > +expression E1, E2;
> > +@@
> > +
> > +array_size(E1, E2)
>
> BTW, is there a way yet in Coccinelle to match a fully qualified (?)
> identifier? For example, if I have two lines in C:
>
> A)
> 	array_size(variable, 5);
> B)
> 	array_size(instance->member.size, 5);
> C)
> 	array_size(instance->member.size + 1, 5);
> D)
> 	array_size(function_call(variable), 5);
>
>
> This matches A, B, C, and D:
>
> @@
> expression ARG1;
> expression ARG2;
> @@
>
> array_size(ARG1, ARG2);
>
>
> This matches only A:
>
> @@
> identifier ARG1;
> expression ARG2;
> @@
>
> array_size(ARG1, ARG2);
>
>
> How do I get something to match A and B but not C and D (i.e. I do not
> want to match any operations, function calls, etc, only a variable,
> which may be identified through dereference, array index, or struct
> member access.)

\(i\|e.fld\|e->fld\)

would probably do what you want.  It will also match cases where e is a
function/macr call, but that is unlikely.

If you want a single metavariable that contains the whole thing, you can
have an expression metavariable E and then write:

\(\(i\|e.fld\|e->fld\) \& E\)

julia



>
>
> --
> Kees Cook
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 10:55       ` Denis Efremov
@ 2020-06-17 20:08         ` Julia Lawall
  2020-06-17 20:15           ` Julia Lawall
  0 siblings, 1 reply; 28+ messages in thread
From: Julia Lawall @ 2020-06-17 20:08 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Gustavo A. R. Silva, Kees Cook, cocci, linux-kernel



On Wed, 17 Jun 2020, Denis Efremov wrote:

>
> >
> > Awesome! I'll take a look into this. :)
> >
> Here is another script for your #83 ticket.
> Currently, it issues 598 warnings.
>
> // SPDX-License-Identifier: GPL-2.0-only
> ///
> /// Check for missing overflow checks in allocation functions.
> /// Low confidence because it's pointless to check for overflow
> /// relatively small allocations.
> ///
> // Confidence: Low
> // Copyright: (C) 2020 Denis Efremov ISPRAS
> // Options: --no-includes --include-headers
>
> virtual patch
> virtual context
> virtual org
> virtual report
>
> @depends on patch@
> expression E1, E2, E3, E4, size;
> @@
>
> (
> - size = E1 * E2;
> + size = array_size(E1, E2);
> |
> - size = E1 * E2 * E3;
> + size = array3_size(E1, E2, E3);
> |
> - size = E1 * E2 + E3;
> + size = struct_size(E1, E2, E3);

Should the arguments be checked to see if they have something to do with
arrays and structures?

> )
>   ... when != size = E4
>       when != size += E4
>       when != size -= E4
>       when != size *= E4

Here you can have a metavariable

assignment operator aop;

and then say size aop E4

It doesn't really look like an assignment any more, but it could be a
little safer.

julia

>       when != &size
>   \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
>     vmalloc\|vzalloc\|vzalloc_node\|
>     kvmalloc\|kvzalloc\|kvzalloc_node\|
>     sock_kmalloc\|
>     f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
>     devm_kmalloc\|devm_kzalloc\)
>   (..., size, ...)
>
> @r depends on !patch@
> expression E1, E2, E3, E4, size;
> position p;
> @@
>
> (
> * size = E1 * E2;@p
> |
> * size = E1 * E2 * E3;@p
> |
> * size = E1 * E2 + E3;@p
> )
>   ... when != size = E4
>       when != size += E4
>       when != size -= E4
>       when != size *= E4
>       when != &size
> * \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
>     vmalloc\|vzalloc\|vzalloc_node\|
>     kvmalloc\|kvzalloc\|kvzalloc_node\|
>     sock_kmalloc\|
>     f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
>     devm_kmalloc\|devm_kzalloc\)
>   (..., size, ...)
>
> @script:python depends on report@
> p << r.p;
> @@
>
> coccilib.report.print_report(p[0], "WARNING: missing overflow check")
>
> @script:python depends on org@
> p << r.p;
> @@
>
> coccilib.org.print_todo(p[0], "WARNING: missing overflow check")
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 20:08         ` Julia Lawall
@ 2020-06-17 20:15           ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-17 20:15 UTC (permalink / raw)
  To: Denis Efremov; +Cc: linux-kernel, Kees Cook, cocci



On Wed, 17 Jun 2020, Julia Lawall wrote:

>
>
> On Wed, 17 Jun 2020, Denis Efremov wrote:
>
> >
> > >
> > > Awesome! I'll take a look into this. :)
> > >
> > Here is another script for your #83 ticket.
> > Currently, it issues 598 warnings.
> >
> > // SPDX-License-Identifier: GPL-2.0-only
> > ///
> > /// Check for missing overflow checks in allocation functions.
> > /// Low confidence because it's pointless to check for overflow
> > /// relatively small allocations.
> > ///
> > // Confidence: Low
> > // Copyright: (C) 2020 Denis Efremov ISPRAS
> > // Options: --no-includes --include-headers
> >
> > virtual patch
> > virtual context
> > virtual org
> > virtual report
> >
> > @depends on patch@
> > expression E1, E2, E3, E4, size;
> > @@
> >
> > (
> > - size = E1 * E2;
> > + size = array_size(E1, E2);
> > |
> > - size = E1 * E2 * E3;
> > + size = array3_size(E1, E2, E3);
> > |
> > - size = E1 * E2 + E3;
> > + size = struct_size(E1, E2, E3);
>
> Should the arguments be checked to see if they have something to do with
> arrays and structures?

Sorry for the noise, I see that this comment makes no sense.

julia

>
> > )
> >   ... when != size = E4
> >       when != size += E4
> >       when != size -= E4
> >       when != size *= E4
>
> Here you can have a metavariable
>
> assignment operator aop;
>
> and then say size aop E4
>
> It doesn't really look like an assignment any more, but it could be a
> little safer.
>
> julia
>
> >       when != &size
> >   \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
> >     vmalloc\|vzalloc\|vzalloc_node\|
> >     kvmalloc\|kvzalloc\|kvzalloc_node\|
> >     sock_kmalloc\|
> >     f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
> >     devm_kmalloc\|devm_kzalloc\)
> >   (..., size, ...)
> >
> > @r depends on !patch@
> > expression E1, E2, E3, E4, size;
> > position p;
> > @@
> >
> > (
> > * size = E1 * E2;@p
> > |
> > * size = E1 * E2 * E3;@p
> > |
> > * size = E1 * E2 + E3;@p
> > )
> >   ... when != size = E4
> >       when != size += E4
> >       when != size -= E4
> >       when != size *= E4
> >       when != &size
> > * \(kmalloc\|krealloc\|kzalloc\|kzalloc_node\|
> >     vmalloc\|vzalloc\|vzalloc_node\|
> >     kvmalloc\|kvzalloc\|kvzalloc_node\|
> >     sock_kmalloc\|
> >     f2fs_kmalloc\|f2fs_kzalloc\|f2fs_kvmalloc\|f2fs_kvzalloc\|
> >     devm_kmalloc\|devm_kzalloc\)
> >   (..., size, ...)
> >
> > @script:python depends on report@
> > p << r.p;
> > @@
> >
> > coccilib.report.print_report(p[0], "WARNING: missing overflow check")
> >
> > @script:python depends on org@
> > p << r.p;
> > @@
> >
> > coccilib.org.print_todo(p[0], "WARNING: missing overflow check")
> > _______________________________________________
> > Cocci mailing list
> > Cocci@systeme.lip6.fr
> > https://systeme.lip6.fr/mailman/listinfo/cocci
> >
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
  2020-06-15 18:23 ` Kees Cook
  2020-06-17 18:15 ` Kees Cook
@ 2020-06-17 20:30 ` Julia Lawall
  2020-06-17 20:50   ` Denis Efremov
  2020-06-18 10:23 ` [PATCH v2] " Denis Efremov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 28+ messages in thread
From: Julia Lawall @ 2020-06-17 20:30 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Kees Cook, cocci, linux-kernel



On Mon, 15 Jun 2020, Denis Efremov wrote:

> Detect an opencoded expression that is used before or after
> array_size()/array3_size()/struct_size() to compute the same size.

This would benefit from the assignemnt operator metavariables as well.

Also, it could be better to put the python rules up next the SmPL pattern
matching rules that they are associated with.

julia


>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  scripts/coccinelle/misc/array_size_dup.cocci | 347 +++++++++++++++++++
>  1 file changed, 347 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci
>
> diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
> new file mode 100644
> index 000000000000..08919a938754
> --- /dev/null
> +++ b/scripts/coccinelle/misc/array_size_dup.cocci
> @@ -0,0 +1,347 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for array_size(), array3_size(), struct_size() duplicates.
> +/// Three types of patterns for these functions:
> +///  1. An opencoded expression is used before array_size() to compute the same size
> +///  2. An opencoded expression is used after array_size() to compute the same size
> +///  3. Consecutive calls of array_size() with the same values
> +/// From security point of view only first case is relevant. These functions
> +/// perform arithmetic overflow check. Thus, if we use an opencoded expression
> +/// before a call to the *_size() function we can miss an overflow.
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers --no-loops
> +
> +virtual context
> +virtual report
> +virtual org
> +
> +@as@
> +expression E1, E2;
> +@@
> +
> +array_size(E1, E2)
> +
> +@as_next@
> +expression subE1 <= as.E1;
> +expression as.E1;
> +expression subE2 <= as.E2;
> +expression as.E2;
> +expression E3;
> +position p1, p2;
> +@@
> +
> +* E1 * E2@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\)=E3
> +      when != \(E1\|E2\|subE1\|subE2\)+=E3
> +      when != \(E1\|E2\|subE1\|subE2\)-=E3
> +      when != \(E1\|E2\|subE1\|subE2\)*=E3
> +      when != \(&E1\|&E2\|&subE1\|&subE2\)
> +* array_size(E1, E2)@p2
> +
> +@as_prev@
> +expression subE1 <= as.E1;
> +expression as.E1;
> +expression subE2 <= as.E2;
> +expression as.E2;
> +expression E3;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\)=E3
> +      when != \(E1\|E2\|subE1\|subE2\)+=E3
> +      when != \(E1\|E2\|subE1\|subE2\)-=E3
> +      when != \(E1\|E2\|subE1\|subE2\)*=E3
> +      when != \(&E1\|&E2\|&subE1\|&subE2\)
> +* E1 * E2@p2
> +
> +@as_dup@
> +expression subE1 <= as.E1;
> +expression as.E1;
> +expression subE2 <= as.E2;
> +expression as.E2;
> +expression E3;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\)=E3
> +      when != \(E1\|E2\|subE1\|subE2\)+=E3
> +      when != \(E1\|E2\|subE1\|subE2\)-=E3
> +      when != \(E1\|E2\|subE1\|subE2\)*=E3
> +      when != \(&E1\|&E2\|&subE1\|&subE2\)
> +* array_size(E1, E2)@p2
> +
> +@as3@
> +expression E1, E2, E3;
> +@@
> +
> +array3_size(E1, E2, E3)
> +
> +@as3_next@
> +expression subE1 <= as3.E1;
> +expression as3.E1;
> +expression subE2 <= as3.E2;
> +expression as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 * E3@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@as3_prev@
> +expression subE1 <= as3.E1;
> +expression as3.E1;
> +expression subE2 <= as3.E2;
> +expression as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* E1 * E2 * E3@p2
> +
> +@as3_dup@
> +expression subE1 <= as3.E1;
> +expression as3.E1;
> +expression subE2 <= as3.E2;
> +expression as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@ss@
> +expression E1, E2, E3;
> +@@
> +
> +struct_size(E1, E2, E3)
> +
> +@ss_next@
> +expression subE1 <= ss.E1;
> +expression ss.E1;
> +expression subE2 <= ss.E2;
> +expression ss.E2;
> +expression subE3 <= ss.E3;
> +expression ss.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 + E3@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@ss_prev@
> +expression subE1 <= ss.E1;
> +expression ss.E1;
> +expression subE2 <= ss.E2;
> +expression ss.E2;
> +expression subE3 <= ss.E3;
> +expression ss.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* E1 * E2 + E3@p2
> +
> +@ss_dup@
> +expression subE1 <= ss.E1;
> +expression ss.E1;
> +expression subE2 <= ss.E2;
> +expression ss.E2;
> +expression subE3 <= ss.E3;
> +expression ss.E3;
> +expression E4;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)+=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)-=E4
> +      when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\)*=E4
> +      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +msg = "WARNING: array_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +msg = "WARNING: array_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@script:python depends on report@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +@script:python depends on report@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +msg = "WARNING: same array_size (line %s)" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +msg = "WARNING: same array_size (line %s)" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +
> +@script:python depends on report@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +msg = "WARNING: array3_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +msg = "WARNING: array3_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@script:python depends on report@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +@script:python depends on report@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +msg = "WARNING: same array3_size (line %s)" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +msg = "WARNING: same array3_size (line %s)" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +
> +@script:python depends on report@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +msg = "WARNING: struct_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +msg = "WARNING: struct_size is used down the code (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@script:python depends on report@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +@script:python depends on report@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +msg = "WARNING: same struct_size (line %s)" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +msg = "WARNING: same struct_size (line %s)" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 20:30 ` Julia Lawall
@ 2020-06-17 20:50   ` Denis Efremov
  2020-06-17 20:52     ` Julia Lawall
  0 siblings, 1 reply; 28+ messages in thread
From: Denis Efremov @ 2020-06-17 20:50 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Kees Cook, cocci, linux-kernel



On 6/17/20 11:30 PM, Julia Lawall wrote:
> 
> 
> On Mon, 15 Jun 2020, Denis Efremov wrote:
> 
>> Detect an opencoded expression that is used before or after
>> array_size()/array3_size()/struct_size() to compute the same size.
> 
> This would benefit from the assignemnt operator metavariables as well.
> 
> Also, it could be better to put the python rules up next the SmPL pattern
> matching rules that they are associated with.
> 

Thanks, I will send v2.
Here is the KSPP ticket with patches https://github.com/KSPP/linux/issues/83 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 20:50   ` Denis Efremov
@ 2020-06-17 20:52     ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-17 20:52 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Kees Cook, cocci, linux-kernel



On Wed, 17 Jun 2020, Denis Efremov wrote:

>
>
> On 6/17/20 11:30 PM, Julia Lawall wrote:
> >
> >
> > On Mon, 15 Jun 2020, Denis Efremov wrote:
> >
> >> Detect an opencoded expression that is used before or after
> >> array_size()/array3_size()/struct_size() to compute the same size.
> >
> > This would benefit from the assignemnt operator metavariables as well.
> >
> > Also, it could be better to put the python rules up next the SmPL pattern
> > matching rules that they are associated with.
> >
>
> Thanks, I will send v2.
> Here is the KSPP ticket with patches https://github.com/KSPP/linux/issues/83

Thanks!

julia

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
                   ` (2 preceding siblings ...)
  2020-06-17 20:30 ` Julia Lawall
@ 2020-06-18 10:23 ` Denis Efremov
  2020-06-19 13:13 ` [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks Denis Efremov
  2020-06-22 22:10 ` [PATCH v4] " Denis Efremov
  5 siblings, 0 replies; 28+ messages in thread
From: Denis Efremov @ 2020-06-18 10:23 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Denis Efremov, cocci, linux-kernel, Gustavo A . R . Silva, Kees Cook

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure

 scripts/coccinelle/misc/array_size_dup.cocci | 309 +++++++++++++++++++
 1 file changed, 309 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index 000000000000..c5214310285c
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,309 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the same size
+///  2. An opencoded expression is used after array_size() to compute the same size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@as_dup@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != \(&E1\|&E2\|&subE1\|&subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* E1 * E2 * E3@p2
+
+@script:python depends on report@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array3_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array3_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@as3_dup@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != \(&E1\|&E2\|&E3\|&subE1\|&subE2\|&subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array3_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array3_size (line {p1[0].line})")
+
+@ss@
+expression E1, E2, E3;
+@@
+
+struct_size(E1, E2, E3)
+
+@ss_next@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 + E3@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != \(&E3\|&subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute \
+the same size")
+
+@ss_prev@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != \(&E3\|&subE3\)
+* E1 * E2 + E3@p2
+
+@script:python depends on report@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: struct_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@script:python depends on org@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: struct_size is already used (line {p1[0].line}) to compute \
+the same size")
+
+@ss_dup@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != \(&E3\|&subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same struct_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same struct_size (line {p1[0].line})")
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-17 18:54   ` [Cocci] " Julia Lawall
@ 2020-06-18 19:52     ` Kees Cook
  2020-06-18 19:56       ` Julia Lawall
  0 siblings, 1 reply; 28+ messages in thread
From: Kees Cook @ 2020-06-18 19:52 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Julia Lawall, cocci, linux-kernel

On Wed, Jun 17, 2020 at 08:54:03PM +0200, Julia Lawall wrote:
> 
> 
> On Wed, 17 Jun 2020, Kees Cook wrote:
> 
> > On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> > > +@as@
> > > +expression E1, E2;
> > > +@@
> > > +
> > > +array_size(E1, E2)
> >
> > BTW, is there a way yet in Coccinelle to match a fully qualified (?)
> > identifier? For example, if I have two lines in C:
> >
> > A)
> > 	array_size(variable, 5);
> > B)
> > 	array_size(instance->member.size, 5);
> > C)
> > 	array_size(instance->member.size + 1, 5);
> > D)
> > 	array_size(function_call(variable), 5);
> >
> >
> > This matches A, B, C, and D:
> >
> > @@
> > expression ARG1;
> > expression ARG2;
> > @@
> >
> > array_size(ARG1, ARG2);
> >
> >
> > This matches only A:
> >
> > @@
> > identifier ARG1;
> > expression ARG2;
> > @@
> >
> > array_size(ARG1, ARG2);
> >
> >
> > How do I get something to match A and B but not C and D (i.e. I do not
> > want to match any operations, function calls, etc, only a variable,
> > which may be identified through dereference, array index, or struct
> > member access.)
> 
> \(i\|e.fld\|e->fld\)
> 
> would probably do what you want.  It will also match cases where e is a
> function/macr call, but that is unlikely.
> 
> If you want a single metavariable that contains the whole thing, you can
> have an expression metavariable E and then write:
> 
> \(\(i\|e.fld\|e->fld\) \& E\)

Can you give an example of how that would look for an @@ section?

The problem I have is that I don't know the depth or combination of such
metavariables. There are a lot of combinations:

a
	a.b
		a.b.c
			a.b.c.d
			a.b.c->d
		a.b->c
			a.b->c.d
			a.b->c->d
	a->b
		a->b.c
			a->b.c.d
			a->b.c->d
		a->b->c
			a->b->c.d
			a->b->c->d
...


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-18 19:52     ` Kees Cook
@ 2020-06-18 19:56       ` Julia Lawall
  2020-06-18 20:48         ` Kees Cook
  0 siblings, 1 reply; 28+ messages in thread
From: Julia Lawall @ 2020-06-18 19:56 UTC (permalink / raw)
  To: Kees Cook; +Cc: cocci, linux-kernel



On Thu, 18 Jun 2020, Kees Cook wrote:

> On Wed, Jun 17, 2020 at 08:54:03PM +0200, Julia Lawall wrote:
> >
> >
> > On Wed, 17 Jun 2020, Kees Cook wrote:
> >
> > > On Mon, Jun 15, 2020 at 01:20:45PM +0300, Denis Efremov wrote:
> > > > +@as@
> > > > +expression E1, E2;
> > > > +@@
> > > > +
> > > > +array_size(E1, E2)
> > >
> > > BTW, is there a way yet in Coccinelle to match a fully qualified (?)
> > > identifier? For example, if I have two lines in C:
> > >
> > > A)
> > > 	array_size(variable, 5);
> > > B)
> > > 	array_size(instance->member.size, 5);
> > > C)
> > > 	array_size(instance->member.size + 1, 5);
> > > D)
> > > 	array_size(function_call(variable), 5);
> > >
> > >
> > > This matches A, B, C, and D:
> > >
> > > @@
> > > expression ARG1;
> > > expression ARG2;
> > > @@
> > >
> > > array_size(ARG1, ARG2);
> > >
> > >
> > > This matches only A:
> > >
> > > @@
> > > identifier ARG1;
> > > expression ARG2;
> > > @@
> > >
> > > array_size(ARG1, ARG2);
> > >
> > >
> > > How do I get something to match A and B but not C and D (i.e. I do not
> > > want to match any operations, function calls, etc, only a variable,
> > > which may be identified through dereference, array index, or struct
> > > member access.)
> >
> > \(i\|e.fld\|e->fld\)
> >
> > would probably do what you want.  It will also match cases where e is a
> > function/macr call, but that is unlikely.
> >
> > If you want a single metavariable that contains the whole thing, you can
> > have an expression metavariable E and then write:
> >
> > \(\(i\|e.fld\|e->fld\) \& E\)
>
> Can you give an example of how that would look for an @@ section?
>
> The problem I have is that I don't know the depth or combination of such
> metavariables. There are a lot of combinations:
>
> a
> 	a.b
> 		a.b.c
> 			a.b.c.d
> 			a.b.c->d
> 		a.b->c
> 			a.b->c.d
> 			a.b->c->d
> 	a->b
> 		a->b.c
> 			a->b.c.d
> 			a->b.c->d
> 		a->b->c
> 			a->b->c.d
> 			a->b->c->d
> ...


@@
identifier i,fld;
expression e;
@@

\(\(i\|e.fld\|e->fld\) \& E\)

The e will match all of the variants you are concerned about.

julia



>
>
> --
> Kees Cook
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-18 19:56       ` Julia Lawall
@ 2020-06-18 20:48         ` Kees Cook
  2020-06-18 21:08           ` Julia Lawall
  0 siblings, 1 reply; 28+ messages in thread
From: Kees Cook @ 2020-06-18 20:48 UTC (permalink / raw)
  To: Julia Lawall; +Cc: cocci, linux-kernel

On Thu, Jun 18, 2020 at 09:56:18PM +0200, Julia Lawall wrote:
> @@
> identifier i,fld;
> expression e;
> @@
> 
> \(\(i\|e.fld\|e->fld\) \& E\)
> 
> The e will match all of the variants you are concerned about.

Ah, I see! Okay, that's good. And the "& E" part is to effectively
collect it into E (as in, both the left and right of the & must match).

So to do the matching from earlier:

@@
identifier i, fld;
expression e, ARG1, ARG2;
@@

array_size(\(\(i\|e.fld\|e->fld\) \& ARG1\), ARG2);


?


-- 
Kees Cook

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks
  2020-06-18 20:48         ` Kees Cook
@ 2020-06-18 21:08           ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-18 21:08 UTC (permalink / raw)
  To: Kees Cook; +Cc: cocci, linux-kernel



On Thu, 18 Jun 2020, Kees Cook wrote:

> On Thu, Jun 18, 2020 at 09:56:18PM +0200, Julia Lawall wrote:
> > @@
> > identifier i,fld;
> > expression e;
> > @@
> >
> > \(\(i\|e.fld\|e->fld\) \& E\)
> >
> > The e will match all of the variants you are concerned about.
>
> Ah, I see! Okay, that's good. And the "& E" part is to effectively
> collect it into E (as in, both the left and right of the & must match).

Yes

> So to do the matching from earlier:
>
> @@
> identifier i, fld;
> expression e, ARG1, ARG2;
> @@
>
> array_size(\(\(i\|e.fld\|e->fld\) \& ARG1\), ARG2);

Yes, that would be fine.

julia

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
                   ` (3 preceding siblings ...)
  2020-06-18 10:23 ` [PATCH v2] " Denis Efremov
@ 2020-06-19 13:13 ` Denis Efremov
  2020-06-21 20:53   ` [Cocci] " Julia Lawall
  2020-06-21 20:56   ` Julia Lawall
  2020-06-22 22:10 ` [PATCH v4] " Denis Efremov
  5 siblings, 2 replies; 28+ messages in thread
From: Denis Efremov @ 2020-06-19 13:13 UTC (permalink / raw)
  To: Julia Lawall
  Cc: Denis Efremov, cocci, linux-kernel, Gustavo A . R . Silva, Kees Cook

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Cc: Gustavo A. R. Silva <garsilva@embeddedor.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Denis Efremov <efremov@linux.com>
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure
Changes in v3:
 - s/overlow/overflow/ typo fixed (thanks, Markus)
 - \(&E1\|&E2\) changed to &\(E1\|E2\)
 - print strings breaks removed

 scripts/coccinelle/misc/array_size_dup.cocci | 297 +++++++++++++++++++
 1 file changed, 297 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index 000000000000..d03740257e97
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the same size
+///  2. An opencoded expression is used after array_size() to compute the same size
+///  3. Consecutive calls of array_size() with the same values
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != &\(E1\|E2\|subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != &\(E1\|E2\|subE1\|subE2\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
+
+@as_dup@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
+      when != &\(E1\|E2\|subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as_dup.p1;
+p2 << as_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array_size (line {p1[0].line})")
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
+* E1 * E2 * E3@p2
+
+@script:python depends on report@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
+
+@as3_dup@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
+      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same array3_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << as3_dup.p1;
+p2 << as3_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same array3_size (line {p1[0].line})")
+
+@ss@
+expression E1, E2, E3;
+@@
+
+struct_size(E1, E2, E3)
+
+@ss_next@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 + E3@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != &\(E3\|subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+coccilib.report.print_report(p1[0],
+f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+coccilib.org.print_todo(p1[0],
+f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
+
+@ss_prev@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != &\(E3\|subE3\)
+* E1 * E2 + E3@p2
+
+@script:python depends on report@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
+
+@script:python depends on org@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
+
+@ss_dup@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != \(E3\|subE3\) aop E4
+      when != &\(E3\|subE3\)
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+f"WARNING: same struct_size (line {p1[0].line})")
+
+@script:python depends on org@
+p1 << ss_dup.p1;
+p2 << ss_dup.p2;
+@@
+
+coccilib.org.print_todo(p2[0],
+f"WARNING: same struct_size (line {p1[0].line})")
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to     detect missed overflow checks
  2020-06-19 13:13 ` [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks Denis Efremov
@ 2020-06-21 20:53   ` Julia Lawall
  2020-06-21 20:56   ` Julia Lawall
  1 sibling, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-21 20:53 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Kees Cook, cocci, linux-kernel

> +@as_next@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> +      when != &\(E1\|E2\|subE1\|subE2\)

You don't need E1 and E2 in the above two lines.  subE1 and subE2 will
match them.  Likewise for the other rules.

> +* array_size(E1, E2)@p2
> +
> +@script:python depends on report@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")

I don't think that "down the code" is very understandable.  "Later in the
code" would be fine.  Even just "later" would be fine.

julia

> +@script:python depends on org@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@as_prev@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> +      when != &\(E1\|E2\|subE1\|subE2\)
> +* E1 * E2@p2
> +
> +@script:python depends on report@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@as_dup@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> +      when != &\(E1\|E2\|subE1\|subE2\)
> +* array_size(E1, E2)@p2
> +
> +@script:python depends on report@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same array_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same array_size (line {p1[0].line})")
> +
> +@as3@
> +expression E1, E2, E3;
> +@@
> +
> +array3_size(E1, E2, E3)
> +
> +@as3_next@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 * E3@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@as3_prev@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* E1 * E2 * E3@p2
> +
> +@script:python depends on report@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@as3_dup@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same array3_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same array3_size (line {p1[0].line})")
> +
> +@ss@
> +expression E1, E2, E3;
> +@@
> +
> +struct_size(E1, E2, E3)
> +
> +@ss_next@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 + E3@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@ss_prev@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* E1 * E2 + E3@p2
> +
> +@script:python depends on report@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@ss_dup@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same struct_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same struct_size (line {p1[0].line})")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to     detect missed overflow checks
  2020-06-19 13:13 ` [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks Denis Efremov
  2020-06-21 20:53   ` [Cocci] " Julia Lawall
@ 2020-06-21 20:56   ` Julia Lawall
  2020-06-22 12:12     ` Denis Efremov
  2020-06-22 12:16     ` Denis Efremov
  1 sibling, 2 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-21 20:56 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Kees Cook, cocci, linux-kernel

> +@script:python depends on report@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")

I get python failures for all of these messages.  I know nothing about
python.  How is this supposed to work?  Is it a python 2 vs python 3
thing?

julia


> +
> +@script:python depends on org@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: array_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@as_prev@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> +      when != &\(E1\|E2\|subE1\|subE2\)
> +* E1 * E2@p2
> +
> +@script:python depends on report@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: array_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@as_dup@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> +      when != &\(E1\|E2\|subE1\|subE2\)
> +* array_size(E1, E2)@p2
> +
> +@script:python depends on report@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same array_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << as_dup.p1;
> +p2 << as_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same array_size (line {p1[0].line})")
> +
> +@as3@
> +expression E1, E2, E3;
> +@@
> +
> +array3_size(E1, E2, E3)
> +
> +@as3_next@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 * E3@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: array3_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@as3_prev@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* E1 * E2 * E3@p2
> +
> +@script:python depends on report@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: array3_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@as3_dup@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(E1\|E2\|E3\|subE1\|subE2\|subE3\) aop E4
> +      when != &\(E1\|E2\|E3\|subE1\|subE2\|subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same array3_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << as3_dup.p1;
> +p2 << as3_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same array3_size (line {p1[0].line})")
> +
> +@ss@
> +expression E1, E2, E3;
> +@@
> +
> +struct_size(E1, E2, E3)
> +
> +@ss_next@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 + E3@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +coccilib.report.print_report(p1[0],
> +f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +coccilib.org.print_todo(p1[0],
> +f"WARNING: struct_size is used down the code (line {p2[0].line}) to compute the same size")
> +
> +@ss_prev@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* E1 * E2 + E3@p2
> +
> +@script:python depends on report@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@script:python depends on org@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: struct_size is already used (line {p1[0].line}) to compute the same size")
> +
> +@ss_dup@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != \(E3\|subE3\) aop E4
> +      when != &\(E3\|subE3\)
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> +f"WARNING: same struct_size (line {p1[0].line})")
> +
> +@script:python depends on org@
> +p1 << ss_dup.p1;
> +p2 << ss_dup.p2;
> +@@
> +
> +coccilib.org.print_todo(p2[0],
> +f"WARNING: same struct_size (line {p1[0].line})")
> --
> 2.26.2
>
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-21 20:56   ` Julia Lawall
@ 2020-06-22 12:12     ` Denis Efremov
  2020-06-22 12:16     ` Denis Efremov
  1 sibling, 0 replies; 28+ messages in thread
From: Denis Efremov @ 2020-06-22 12:12 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Kees Cook, cocci, linux-kernel



On 6/21/20 11:56 PM, Julia Lawall wrote:
> Is it a python 2 vs python 3 thing?

Yes, python2 is no longer supported and I
thought it would be safe to use this syntax.

Ok, I will make it portable in v4.

Denis

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-21 20:56   ` Julia Lawall
  2020-06-22 12:12     ` Denis Efremov
@ 2020-06-22 12:16     ` Denis Efremov
  2020-06-22 12:19       ` Julia Lawall
  1 sibling, 1 reply; 28+ messages in thread
From: Denis Efremov @ 2020-06-22 12:16 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Kees Cook, cocci, linux-kernel

What do you think about removing duplicates warning from the rule?

I mean this kind of warnings: "WARNING: same array_size (line {p1[0].line})"

As for now, I think it's better to not disturb developers with this kind
of things.

Thanks,
Denis

>> +@as_dup@
>> +expression subE1 <= as.E1;
>> +expression subE2 <= as.E2;
>> +expression as.E1, as.E2, E3;
>> +assignment operator aop;
>> +position p1, p2;
>> +@@
>> +
>> +* array_size(E1, E2)@p1
>> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
>> +      when != &\(E1\|E2\|subE1\|subE2\)
>> +* array_size(E1, E2)@p2
>> +
>> +@script:python depends on report@
>> +p1 << as_dup.p1;
>> +p2 << as_dup.p2;
>> +@@
>> +
>> +coccilib.report.print_report(p2[0],
>> +f"WARNING: same array_size (line {p1[0].line})")
>> +
>> +@script:python depends on org@
>> +p1 << as_dup.p1;
>> +p2 << as_dup.p2;
>> +@@
>> +
>> +coccilib.org.print_todo(p2[0],
>> +f"WARNING: same array_size (line {p1[0].line})")
>> +


^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [Cocci] [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-22 12:16     ` Denis Efremov
@ 2020-06-22 12:19       ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-22 12:19 UTC (permalink / raw)
  To: Denis Efremov; +Cc: Kees Cook, cocci, linux-kernel



On Mon, 22 Jun 2020, Denis Efremov wrote:

> What do you think about removing duplicates warning from the rule?
>
> I mean this kind of warnings: "WARNING: same array_size (line {p1[0].line})"
>
> As for now, I think it's better to not disturb developers with this kind
> of things.

I agree.  I guess there is a small overhead for the test, but if the code
is not performance critical, being simpler is probably better.

julia


>
> Thanks,
> Denis
>
> >> +@as_dup@
> >> +expression subE1 <= as.E1;
> >> +expression subE2 <= as.E2;
> >> +expression as.E1, as.E2, E3;
> >> +assignment operator aop;
> >> +position p1, p2;
> >> +@@
> >> +
> >> +* array_size(E1, E2)@p1
> >> +  ... when != \(E1\|E2\|subE1\|subE2\) aop E3
> >> +      when != &\(E1\|E2\|subE1\|subE2\)
> >> +* array_size(E1, E2)@p2
> >> +
> >> +@script:python depends on report@
> >> +p1 << as_dup.p1;
> >> +p2 << as_dup.p2;
> >> +@@
> >> +
> >> +coccilib.report.print_report(p2[0],
> >> +f"WARNING: same array_size (line {p1[0].line})")
> >> +
> >> +@script:python depends on org@
> >> +p1 << as_dup.p1;
> >> +p2 << as_dup.p2;
> >> +@@
> >> +
> >> +coccilib.org.print_todo(p2[0],
> >> +f"WARNING: same array_size (line {p1[0].line})")
> >> +
>
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v4] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
                   ` (4 preceding siblings ...)
  2020-06-19 13:13 ` [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks Denis Efremov
@ 2020-06-22 22:10 ` Denis Efremov
  2020-06-24 19:42   ` Julia Lawall
  5 siblings, 1 reply; 28+ messages in thread
From: Denis Efremov @ 2020-06-22 22:10 UTC (permalink / raw)
  To: Julia Lawall; +Cc: Denis Efremov, cocci, linux-kernel

Detect an opencoded expression that is used before or after
array_size()/array3_size()/struct_size() to compute the same size.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
Changes in v2:
 - python rules moved next to SmPL patterns
 - assignment operator used
 - struct_size patterns fixed to check only E3, since
   E1, E2 are sizeofs of a structure and a member
   of a structure
Changes in v3:
 - s/overlow/overflow/ typo fixed (thanks, Markus)
 - \(&E1\|&E2\) changed to &\(E1\|E2\)
 - print strings breaks removed
Changes in v4:
 - duplicates warning removed
 - python2 compatability in report&&org modes added
 - s/down the code/later/ warning changed
 - \(E1\|E2\|subE1\|subE2\) patterns simplified to \(subE1\|subE2\)

 scripts/coccinelle/misc/array_size_dup.cocci | 209 +++++++++++++++++++
 1 file changed, 209 insertions(+)
 create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci

diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
new file mode 100644
index 000000000000..d3d635b2d4fc
--- /dev/null
+++ b/scripts/coccinelle/misc/array_size_dup.cocci
@@ -0,0 +1,209 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Check for array_size(), array3_size(), struct_size() duplicates.
+/// Three types of patterns for these functions:
+///  1. An opencoded expression is used before array_size() to compute the same size
+///  2. An opencoded expression is used after array_size() to compute the same size
+/// From security point of view only first case is relevant. These functions
+/// perform arithmetic overflow check. Thus, if we use an opencoded expression
+/// before a call to the *_size() function we can miss an overflow.
+///
+// Confidence: High
+// Copyright: (C) 2020 Denis Efremov ISPRAS
+// Options: --no-includes --include-headers --no-loops
+
+virtual context
+virtual report
+virtual org
+
+@as@
+expression E1, E2;
+@@
+
+array_size(E1, E2)
+
+@as_next@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2@p1
+  ... when != \(subE1\|subE2\) aop E3
+      when != &\(subE1\|subE2\)
+* array_size(E1, E2)@p2
+
+@script:python depends on report@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as_next.p1;
+p2 << as_next.p2;
+@@
+
+msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@as_prev@
+expression subE1 <= as.E1;
+expression subE2 <= as.E2;
+expression as.E1, as.E2, E3;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array_size(E1, E2)@p1
+  ... when != \(subE1\|subE2\) aop E3
+      when != &\(subE1\|subE2\)
+* E1 * E2@p2
+
+@script:python depends on report@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as_prev.p1;
+p2 << as_prev.p2;
+@@
+
+msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@as3@
+expression E1, E2, E3;
+@@
+
+array3_size(E1, E2, E3)
+
+@as3_next@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 * E3@p1
+  ... when != \(subE1\|subE2\|subE3\) aop E4
+      when != &\(subE1\|subE2\|subE3\)
+* array3_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << as3_next.p1;
+p2 << as3_next.p2;
+@@
+
+msg = "WARNING: array3_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@as3_prev@
+expression subE1 <= as3.E1;
+expression subE2 <= as3.E2;
+expression subE3 <= as3.E3;
+expression as3.E1, as3.E2, as3.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* array3_size(E1, E2, E3)@p1
+  ... when != \(subE1\|subE2\|subE3\) aop E4
+      when != &\(subE1\|subE2\|subE3\)
+* E1 * E2 * E3@p2
+
+@script:python depends on report@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << as3_prev.p1;
+p2 << as3_prev.p2;
+@@
+
+msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
+
+@ss@
+expression E1, E2, E3;
+@@
+
+struct_size(E1, E2, E3)
+
+@ss_next@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* E1 * E2 + E3@p1
+  ... when != subE3 aop E4
+      when != &subE3
+* struct_size(E1, E2, E3)@p2
+
+@script:python depends on report@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+msg = "WARNING: struct_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.report.print_report(p1[0], msg)
+
+@script:python depends on org@
+p1 << ss_next.p1;
+p2 << ss_next.p2;
+@@
+
+msg = "WARNING: struct_size is used later (line %s) to compute the same size" % (p2[0].line)
+coccilib.org.print_todo(p1[0], msg)
+
+@ss_prev@
+expression subE3 <= ss.E3;
+expression ss.E1, ss.E2, ss.E3, E4;
+assignment operator aop;
+position p1, p2;
+@@
+
+* struct_size(E1, E2, E3)@p1
+  ... when != subE3 aop E4
+      when != &subE3
+* E1 * E2 + E3@p2
+
+@script:python depends on report@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.report.print_report(p2[0], msg)
+
+@script:python depends on org@
+p1 << ss_prev.p1;
+p2 << ss_prev.p2;
+@@
+
+msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
+coccilib.org.print_todo(p2[0], msg)
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v4] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-22 22:10 ` [PATCH v4] " Denis Efremov
@ 2020-06-24 19:42   ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-24 19:42 UTC (permalink / raw)
  To: Denis Efremov; +Cc: cocci, linux-kernel



On Tue, 23 Jun 2020, Denis Efremov wrote:

> Detect an opencoded expression that is used before or after
> array_size()/array3_size()/struct_size() to compute the same size.
>
> Signed-off-by: Denis Efremov <efremov@linux.com>

Applied, thanks.

julia

> ---
> Changes in v2:
>  - python rules moved next to SmPL patterns
>  - assignment operator used
>  - struct_size patterns fixed to check only E3, since
>    E1, E2 are sizeofs of a structure and a member
>    of a structure
> Changes in v3:
>  - s/overlow/overflow/ typo fixed (thanks, Markus)
>  - \(&E1\|&E2\) changed to &\(E1\|E2\)
>  - print strings breaks removed
> Changes in v4:
>  - duplicates warning removed
>  - python2 compatability in report&&org modes added
>  - s/down the code/later/ warning changed
>  - \(E1\|E2\|subE1\|subE2\) patterns simplified to \(subE1\|subE2\)
>
>  scripts/coccinelle/misc/array_size_dup.cocci | 209 +++++++++++++++++++
>  1 file changed, 209 insertions(+)
>  create mode 100644 scripts/coccinelle/misc/array_size_dup.cocci
>
> diff --git a/scripts/coccinelle/misc/array_size_dup.cocci b/scripts/coccinelle/misc/array_size_dup.cocci
> new file mode 100644
> index 000000000000..d3d635b2d4fc
> --- /dev/null
> +++ b/scripts/coccinelle/misc/array_size_dup.cocci
> @@ -0,0 +1,209 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +///
> +/// Check for array_size(), array3_size(), struct_size() duplicates.
> +/// Three types of patterns for these functions:
> +///  1. An opencoded expression is used before array_size() to compute the same size
> +///  2. An opencoded expression is used after array_size() to compute the same size
> +/// From security point of view only first case is relevant. These functions
> +/// perform arithmetic overflow check. Thus, if we use an opencoded expression
> +/// before a call to the *_size() function we can miss an overflow.
> +///
> +// Confidence: High
> +// Copyright: (C) 2020 Denis Efremov ISPRAS
> +// Options: --no-includes --include-headers --no-loops
> +
> +virtual context
> +virtual report
> +virtual org
> +
> +@as@
> +expression E1, E2;
> +@@
> +
> +array_size(E1, E2)
> +
> +@as_next@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2@p1
> +  ... when != \(subE1\|subE2\) aop E3
> +      when != &\(subE1\|subE2\)
> +* array_size(E1, E2)@p2
> +
> +@script:python depends on report@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << as_next.p1;
> +p2 << as_next.p2;
> +@@
> +
> +msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@as_prev@
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array_size(E1, E2)@p1
> +  ... when != \(subE1\|subE2\) aop E3
> +      when != &\(subE1\|subE2\)
> +* E1 * E2@p2
> +
> +@script:python depends on report@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as_prev.p1;
> +p2 << as_prev.p2;
> +@@
> +
> +msg = "WARNING: array_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +@as3@
> +expression E1, E2, E3;
> +@@
> +
> +array3_size(E1, E2, E3)
> +
> +@as3_next@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 * E3@p1
> +  ... when != \(subE1\|subE2\|subE3\) aop E4
> +      when != &\(subE1\|subE2\|subE3\)
> +* array3_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +msg = "WARNING: array3_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << as3_next.p1;
> +p2 << as3_next.p2;
> +@@
> +
> +msg = "WARNING: array3_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@as3_prev@
> +expression subE1 <= as3.E1;
> +expression subE2 <= as3.E2;
> +expression subE3 <= as3.E3;
> +expression as3.E1, as3.E2, as3.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* array3_size(E1, E2, E3)@p1
> +  ... when != \(subE1\|subE2\|subE3\) aop E4
> +      when != &\(subE1\|subE2\|subE3\)
> +* E1 * E2 * E3@p2
> +
> +@script:python depends on report@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << as3_prev.p1;
> +p2 << as3_prev.p2;
> +@@
> +
> +msg = "WARNING: array3_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> +
> +@ss@
> +expression E1, E2, E3;
> +@@
> +
> +struct_size(E1, E2, E3)
> +
> +@ss_next@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* E1 * E2 + E3@p1
> +  ... when != subE3 aop E4
> +      when != &subE3
> +* struct_size(E1, E2, E3)@p2
> +
> +@script:python depends on report@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +msg = "WARNING: struct_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)
> +
> +@script:python depends on org@
> +p1 << ss_next.p1;
> +p2 << ss_next.p2;
> +@@
> +
> +msg = "WARNING: struct_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.org.print_todo(p1[0], msg)
> +
> +@ss_prev@
> +expression subE3 <= ss.E3;
> +expression ss.E1, ss.E2, ss.E3, E4;
> +assignment operator aop;
> +position p1, p2;
> +@@
> +
> +* struct_size(E1, E2, E3)@p1
> +  ... when != subE3 aop E4
> +      when != &subE3
> +* E1 * E2 + E3@p2
> +
> +@script:python depends on report@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.report.print_report(p2[0], msg)
> +
> +@script:python depends on org@
> +p1 << ss_prev.p1;
> +p2 << ss_prev.p2;
> +@@
> +
> +msg = "WARNING: struct_size is already used (line %s) to compute the same size" % (p1[0].line)
> +coccilib.org.print_todo(p2[0], msg)
> --
> 2.26.2
>
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4] coccinelle: misc: add array_size_dup script to detect missed overflow checks
  2020-06-23  6:12 Markus Elfring
@ 2020-06-23  7:02 ` Julia Lawall
  0 siblings, 0 replies; 28+ messages in thread
From: Julia Lawall @ 2020-06-23  7:02 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Denis Efremov, Coccinelle, Gilles Muller, Julia Lawall,
	Masahiro Yamada, Michal Marek, Nicolas Palix, linux-kernel,
	kernel-janitors, Gustavo A. R. Silva, Kees Cook

[-- Attachment #1: Type: text/plain, Size: 1358 bytes --]

I don't agree with any of these comments.

julia

On Tue, 23 Jun 2020, Markus Elfring wrote:

> > Changes in v2:
> …
> > - assignment operator used
>
> I prefer the distinction for the application of corresponding metavariables.
>
>
> > Changes in v3:
> …
> >  - \(&E1\|&E2\) changed to &\(E1\|E2\)
>
> Would it be more helpful to mention the movement of the ampersand
> before SmPL disjunctions?
>
>
> …
> >+/// Three types of patterns for these functions:
>
> Will another adjustment be needed according to your information “duplicates warning removed”?
>
>
> …
> > +virtual context
> > +virtual report
> > +virtual org
>
> Can the following SmPL code variant ever become more attractive?
>
> +virtual context, report, org
>
>
> …
> > +expression subE1 <= as.E1;
> > +expression subE2 <= as.E2;
> > +expression as.E1, as.E2, E3;
>
> How do you think about the following SmPL code variant?
>
> +expression subE1 <= as.E1,
> +           subE2 <= as.E2,
> +           as.E1, as.E2, E3;
>
>
> …
> > +msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
> > +coccilib.report.print_report(p1[0], msg)
>
> Please omit the extra Python variable “msg” for the passing of such simple message objects.
>
> What does hinder you to take the proposed script variants better into account?
>
> Regards,
> Markus
>

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v4] coccinelle: misc: add array_size_dup script to detect missed overflow checks
@ 2020-06-23  6:12 Markus Elfring
  2020-06-23  7:02 ` Julia Lawall
  0 siblings, 1 reply; 28+ messages in thread
From: Markus Elfring @ 2020-06-23  6:12 UTC (permalink / raw)
  To: Denis Efremov, Coccinelle, Gilles Muller, Julia Lawall,
	Masahiro Yamada, Michal Marek, Nicolas Palix
  Cc: linux-kernel, kernel-janitors, Gustavo A. R. Silva, Kees Cook

> Changes in v2:
> - assignment operator used

I prefer the distinction for the application of corresponding metavariables.


> Changes in v3:
>  - \(&E1\|&E2\) changed to &\(E1\|E2\)

Would it be more helpful to mention the movement of the ampersand
before SmPL disjunctions?


…
>+/// Three types of patterns for these functions:

Will another adjustment be needed according to your information “duplicates warning removed”?


…
> +virtual context
> +virtual report
> +virtual org

Can the following SmPL code variant ever become more attractive?

+virtual context, report, org


…
> +expression subE1 <= as.E1;
> +expression subE2 <= as.E2;
> +expression as.E1, as.E2, E3;

How do you think about the following SmPL code variant?

+expression subE1 <= as.E1,
+           subE2 <= as.E2,
+           as.E1, as.E2, E3;


…
> +msg = "WARNING: array_size is used later (line %s) to compute the same size" % (p2[0].line)
> +coccilib.report.print_report(p1[0], msg)

Please omit the extra Python variable “msg” for the passing of such simple message objects.

What does hinder you to take the proposed script variants better into account?

Regards,
Markus

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-06-24 19:42 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 10:20 [PATCH] coccinelle: misc: add array_size_dup script to detect missed overlow checks Denis Efremov
2020-06-15 18:23 ` Kees Cook
2020-06-15 18:35   ` Denis Efremov
2020-06-15 18:46     ` [Cocci] " Gustavo A. R. Silva
2020-06-17  9:32       ` Denis Efremov
2020-06-17 10:55       ` Denis Efremov
2020-06-17 20:08         ` Julia Lawall
2020-06-17 20:15           ` Julia Lawall
2020-06-17 18:15 ` Kees Cook
2020-06-17 18:54   ` [Cocci] " Julia Lawall
2020-06-18 19:52     ` Kees Cook
2020-06-18 19:56       ` Julia Lawall
2020-06-18 20:48         ` Kees Cook
2020-06-18 21:08           ` Julia Lawall
2020-06-17 20:30 ` Julia Lawall
2020-06-17 20:50   ` Denis Efremov
2020-06-17 20:52     ` Julia Lawall
2020-06-18 10:23 ` [PATCH v2] " Denis Efremov
2020-06-19 13:13 ` [PATCH v3] coccinelle: misc: add array_size_dup script to detect missed overflow checks Denis Efremov
2020-06-21 20:53   ` [Cocci] " Julia Lawall
2020-06-21 20:56   ` Julia Lawall
2020-06-22 12:12     ` Denis Efremov
2020-06-22 12:16     ` Denis Efremov
2020-06-22 12:19       ` Julia Lawall
2020-06-22 22:10 ` [PATCH v4] " Denis Efremov
2020-06-24 19:42   ` Julia Lawall
2020-06-23  6:12 Markus Elfring
2020-06-23  7:02 ` Julia Lawall

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).