From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/logicvc: Avoid use-after-free with devm_kzalloc()
Date: Thu, 04 Jun 2026 14:34:10 +1000
Message-ID: <review-patch1-20260601-logicvc-uaf-v1-1-8c9ca5b3429c@bootlin.com>
In-Reply-To: <20260601-logicvc-uaf-v1-1-8c9ca5b3429c@bootlin.com>
References: <20260601-logicvc-uaf-v1-1-8c9ca5b3429c@bootlin.com> <20260601-logicvc-uaf-v1-1-8c9ca5b3429c@bootlin.com>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**logicvc_crtc.c** =E2=80=94 Clean and straightforward.

The conversion from `devm_kzalloc` + `drm_crtc_init_with_planes` to `drmm_c=
rtc_alloc_with_planes` is correct. The macro arguments match the expected s=
ignature:

```c
crtc =3D drmm_crtc_alloc_with_planes(drm_dev, struct logicvc_crtc,
                                    drm_crtc, &layer_primary->drm_plane,
                                    NULL, &logicvc_crtc_funcs, NULL);
```

Removal of `.destroy =3D drm_crtc_cleanup` is correct =E2=80=94 `drmm_crtc_=
alloc_with_planes` registers cleanup automatically.

No issues.

---

**logicvc_interface.c** =E2=80=94 Also clean.

Smart reordering: the patch moves `drm_of_find_panel_or_bridge()` before th=
e allocation by using local `panel`/`bridge` temporaries, then copies them =
into the `interface` struct after `drmm_encoder_alloc`. This avoids needing=
 the struct before it exists.

```c
struct drm_bridge *bridge;
struct drm_panel *panel;
...
ret =3D drm_of_find_panel_or_bridge(of_node, 0, 0, &panel, &bridge);
...
interface =3D drmm_encoder_alloc(drm_dev, struct logicvc_interface, drm_enc=
oder,
                               NULL, encoder_type, NULL);
...
interface->drm_panel =3D panel;
interface->drm_bridge =3D bridge;
```

The `drmm_connector_init` call correctly adds the `ddc` parameter (`NULL`):

```c
ret =3D drmm_connector_init(drm_dev, &interface->drm_connector,
                           &logicvc_connector_funcs,
                           connector_type, NULL);
```

Removal of `logicvc_encoder_funcs` (which only had `.destroy =3D drm_encode=
r_cleanup`) and `.destroy =3D drm_connector_cleanup` from `logicvc_connecto=
r_funcs` are both correct.

Error path simplification from goto-labels to direct returns is safe since =
all DRM-managed resources will be cleaned up automatically on DRM device re=
lease during probe failure.

No issues.

---

**logicvc_layer.c** =E2=80=94 More involved refactoring, but well done.

The key challenge here is that config must be parsed *before* allocation (f=
ormat lookup depends on config, and format is needed for `drmm_universal_pl=
ane_alloc`). The patch handles this by parsing into a temporary stack struc=
t:

```c
struct logicvc_layer_config config =3D { 0 };
...
ret =3D logicvc_layer_config_parse(logicvc, of_node, &config);
...
formats =3D logicvc_layer_formats_lookup(&config);
...
layer =3D drmm_universal_plane_alloc(drm_dev, ...);
...
logicvc_layer_set_config(layer, &config);
```

The refactoring of `logicvc_layer_config_parse` and `logicvc_layer_formats_=
lookup` to take direct parameters instead of a `layer` pointer is clean.

The `drmm_add_action_or_reset` for `list_del` is correct =E2=80=94 the `_or=
_reset` variant calls the action if registration fails, so the list entry i=
s removed in all cases. LIFO ordering guarantees `list_del` runs before the=
 plane cleanup (which frees the layer memory).

The background layer early-return path is correct: since `layer` hasn't bee=
n allocated yet at that point (config is in a local), no cleanup is needed.

**Minor nit 1:** `logicvc_layer_set_config` does a field-by-field copy that=
 could be a simple struct assignment:

```c
// Current:
static void logicvc_layer_set_config(struct logicvc_layer *layer,
                                     struct logicvc_layer_config *config)
{
    layer->config.colorspace =3D config->colorspace;
    layer->config.depth =3D config->depth;
    ...
}

// Simpler alternative:
layer->config =3D *config;
```

This would be shorter and less error-prone (no risk of forgetting a field i=
f `logicvc_layer_config` gains new members).

**Minor nit 2:** Extra blank line before `return 0;` at the end of `logicvc=
_layer_init`:

```c
	ret =3D drmm_add_action_or_reset(drm_dev, logicvc_layer_fini,
				       layer);
	if (ret)
		return ret;


	return 0;
```

One blank line would be sufficient.

**Pre-existing issue (not introduced by this patch):** When `logicvc_layer_=
init` fails inside the `for_each_child_of_node` loop, neither the old nor n=
ew code calls `of_node_put(layer_node)`, leaking the OF node reference. Thi=
s isn't a regression from this patch, but worth noting for a future fix.

---
Generated by Claude Code Patch Reviewer