[PATCH] cocci: drm: report devm-allocated arguments and fields

Oleg Petrov posted 1 patch 1 week ago
.../coccinelle/api/drm_dont_devm_alloc.cocci  | 124 ++++++++++++++++++
1 file changed, 124 insertions(+)
create mode 100644 scripts/coccinelle/api/drm_dont_devm_alloc.cocci
[PATCH] cocci: drm: report devm-allocated arguments and fields
Posted by Oleg Petrov 1 week ago
Add two Coccinelle rules: (1) flag a devres-managed pointer
passed to a drm-init function, and (2) flag a devres-managed
pointer assigned to a field of drm-managed struct. The mismatch
of the lifespan between devres- and drm-managed memory can cause
a use-after-free error. The rules' have high confidence.

badarg: Report when a pointer to devres-managed memory is passed
   as the second argument to several drm_*_init* functions that
   (a) have such rule documented or (b) are some simple wrappers
   to such a function.

badarg2: The same, but pass the devm-allocated pointer through
   an intermediate variable before passing to a drm-init function.

badfield: Report when a devm-allocated pointer is assigned to a
   field of a drm-managed struct.

Found by Linux Verification Center (linuxtesting.org).

Signed-off-by: Oleg Petrov <o.petrov@ispras.ru>
---
v1: Among drivers/gpu/drm, Coccinelle finds 7 functions (b) that call
functions (a). I included 5 of those in the rule. I did not check other files.
The functions are the same for v6.1+.
Patch https://lore.kernel.org/all/20240216125040.8968-1-e.orlova@ispras.ru/
fixes such violations in stm module. badargs are obvious from diff,
and the badfield is fixed in first lines where ldev is allocated.
 .../coccinelle/api/drm_dont_devm_alloc.cocci  | 124 ++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 scripts/coccinelle/api/drm_dont_devm_alloc.cocci

diff --git a/scripts/coccinelle/api/drm_dont_devm_alloc.cocci b/scripts/coccinelle/api/drm_dont_devm_alloc.cocci
new file mode 100644
index 000000000000..693977e98666
--- /dev/null
+++ b/scripts/coccinelle/api/drm_dont_devm_alloc.cocci
@@ -0,0 +1,124 @@
+// SPDX-License-Identifier: GPL-2.0-only
+///
+/// Find (1) devres-allocated (usually by devm_kzalloc) argument to drm_*_init
+/// functions or (2) assignment of devres-allocated pointer to a field of a drm-
+/// allocated struct (usually dev_private of drm_device). The mismatch of the
+/// lifespan between devres- and drm-managed memory can cause a use-after-free.
+//
+// Confidence: High
+// Copyright: (C) 2025 Oleg Petrov ISPRAS
+// Options: --no-includes --include-headers
+//
+
+virtual report
+virtual org
+
+// find devm-allocated (devres-managed) second arg for drm*init functions
+@badarg exists@
+position p;
+expression devm,e;
+@@
+// only devm_kzalloc is really used
+devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
+...
+// The kernel-doc comments (v6+) for these 5 functions
+// forbid them to use devm-allocated argument.
+( drm_connector_init
+| drm_crtc_init_with_planes
+| drm_connector_init_with_ddc
+| drm_encoder_init
+| drm_universal_plane_init
+// These are the wrappers found in drivers/gpu/drm/*.c
+// i.e. these call those above and just pass the second argument.
+| drm_bridge_connector_init
+| drm_crtc_init
+| drm_plane_init
+// drm_simple_display_pipe_init // does not apply
+| drm_simple_encoder_init
+| drm_writeback_connector_init
+// mipi_dbi_dev_init_with_formats // does not apply
+) ( e,<+...devm@p...+>,...)
+
+// same as above, but with an intermediate local variable
+@badarg2 exists@
+position p;
+expression devm,e;
+identifier vitm;
+@@
+// only devm_kzalloc is really used
+devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
+...
+vitm = <+...devm...+>;
+...
+// The kernel-doc comments (v6+) for these 5 functions
+// forbid them to use devm-allocated argument.
+( drm_connector_init
+| drm_crtc_init_with_planes
+| drm_connector_init_with_ddc
+| drm_encoder_init
+| drm_universal_plane_init
+// These are the wrappers found in drivers/gpu/drm/*.c
+// i.e. these call those above and just pass the second argument.
+| drm_bridge_connector_init
+| drm_crtc_init
+| drm_plane_init
+// drm_simple_display_pipe_init // does not apply
+| drm_simple_encoder_init
+| drm_writeback_connector_init
+// mipi_dbi_dev_init_with_formats // does not apply
+) ( e,<+...devm@p...+>,...)
+
+// find direct assignment of devres-managed memory to drm device
+@badfield exists@
+position p;
+expression drm,devm;
+identifier f;
+@@
+(
+drm = \(drm_dev_alloc\|drmm_kzalloc\|drmm_kcalloc\|drmm_kmalloc\|drmm_kmalloc_array\)(...);
+...
+devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
+|
+devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
+...
+drm = \(drm_dev_alloc\|drmm_kzalloc\|drmm_kcalloc\|drmm_kmalloc\|drmm_kmalloc_array\)(...);
+)
+...
+drm->f =@p <+...devm...+>;
+
+
+@script:python depends on report@
+p << badarg.p;
+@@
+msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
+coccilib.report.print_report(p[0], msg)
+
+@script:python depends on org@
+p << badarg.p;
+@@
+msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
+coccilib.org.print_report(p[0], msg)
+
+@script:python depends on report@
+p << badarg2.p;
+@@
+msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
+coccilib.report.print_report(p[0], msg)
+
+@script:python depends on org@
+p << badarg2.p;
+@@
+msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
+coccilib.org.print_report(p[0], msg)
+
+@script:python depends on report@
+p << badfield.p;
+@@
+msg = "WARNING devm-allocated field in a drmm-allocated struct; consider drmm-init family or use drmm-alloc."
+coccilib.report.print_report(p[0], msg)
+
+@script:python depends on org@
+p << badfield.p;
+@@
+msg = "WARNING devm-allocated field in a drmm-allocated struct; consider drmm-init family or use drmm-alloc."
+coccilib.org.print_report(p[0], msg)
-- 
2.34.1
Re: [cocci] [PATCH] cocci: drm: report devm-allocated arguments and fields
Posted by Markus Elfring 4 days, 14 hours ago
…> +// find devm-allocated (devres-managed) second arg for drm*init functions
> +@badarg exists@
> +position p;
> +expression devm,e;
> +@@
> +// only devm_kzalloc is really used
> +devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
> +...
…> +// same as above, but with an intermediate local variable
> +@badarg2 exists@
> +position p;
> +expression devm,e;
> +identifier vitm;
> +@@
> +// only devm_kzalloc is really used
> +devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);
> +...
> +vitm = <+...devm...+>;
…

Your source code search approach affects recurring development concerns.

The shown rules start with the same SmPL code.
Thus I would find it nicer if such duplicate code can be avoided.
I imagine that software run time characteristics would be more desirable
if selected data processing does not need to be repeated.

The search should obviously be continued after a relevant source code place was found.


…> +@script:python depends on report@
> +p << badarg.p;
…> +@script:python depends on report@
> +p << badarg2.p;
…> +@script:python depends on report@
> +p << badfield.p;
…

Will development interests grow for another clarification approach?

Support for SmPL rule groups
2019-04-07
https://github.com/coccinelle/coccinelle/issues/164

Regards,
Markus
Re: [cocci] [PATCH] cocci: drm: report devm-allocated arguments and fields
Posted by Markus Elfring 1 week ago
> Add two Coccinelle rules: (1) flag a devres-managed pointer
> passed to a drm-init function, and (2) flag a devres-managed
> pointer assigned to a field of drm-managed struct. …

I suggest to start each enumeration item on a separate text line.


Would you like to refer to “Coccinelle: api:” in the patch prefix?


…> +// Copyright: (C) 2025 Oleg Petrov ISPRAS

Would another delimiter be helpful between the personal name and the organisation identifier?


…> +virtual report
> +virtual org

The restriction on the support for two operation modes will need further development considerations.


…> +devm = \(devm_kzalloc\|devm_kcalloc\|devm_kmalloc\|devm_kmalloc_array\)(...);

Would it be nicer to write such SmPL disjunctions on multiple lines?


…> +@script:python depends on report@
> +p << badarg.p;
> +@@
> +msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
> +coccilib.report.print_report(p[0], msg)

I would like to point out once more that such data can be printed
also without an extra variable like “msg”.


> +@script:python depends on org@
> +p << badarg.p;
> +@@
> +msg = "WARNING devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc)."
> +coccilib.org.print_report(p[0], msg)

Would the following method call be more appropriate?

coccilib.org.print_todo(p[0],
                        "WARNING: devm-allocated argument in a drm-init; use drmm-init family (or drmm-alloc).")


Regards,
Markus