[PATCH AUTOSEL 6.18-6.6] drivers/of: validate status properties in reconfig state changes

[Sasha Levin]

From: Pengpeng Hou <pengpeng@xxxxxxxxxxx>

[ Upstream commit 0b6b12c5dcce16e604d4cde953bef46531b98571 ]

Live-tree reconfiguration properties also carry raw values plus explicit
lengths. `of_reconfig_get_state_change()` currently treats `status`
property values as NUL-terminated strings and feeds them straight into
`strcmp()`.

Factor the `"okay"` / `"ok"` check out into a helper that first verifies
that the property contains a bounded C string within `prop->length`.
Malformed `status` updates should be treated as not enabling the node.

Signed-off-by: Pengpeng Hou <pengpeng@xxxxxxxxxxx>
Link: https://patch.msgid.link/20260507081812.91838-2-pengpeng@xxxxxxxxxxx
Signed-off-by: Rob Herring (Arm) <robh@xxxxxxxxxx>
Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
---

LLM Generated explanations, may be completely bogus:

## Phase 1: Commit Message Forensics

### Step 1.1: Subject Line
**Record:** `[drivers/of]` `[validate]` — validate `status` properties
during live-tree reconfiguration state-change detection.

### Step 1.2: Tags
**Record:**
- **Link:**
  `https://patch.msgid.link/20260507081812.91838-2-pengpeng@xxxxxxxxxxx`
  (v3, patch 2/2)
- **Signed-off-by:** Pengpeng Hou `<pengpeng@xxxxxxxxxxx>`
- **Signed-off-by:** Rob Herring (Arm) `<robh@xxxxxxxxxx>` (OF
  maintainer)
- No Fixes:, Reported-by:, Tested-by:, Cc: stable, or syzbot tags
- Notable: patch **2/2** in a series; v3 changelog says "no code change;
  carried with patch 1/2"

### Step 1.3: Body Analysis
**Record:**
- **Bug:** `of_reconfig_get_state_change()` uses `strcmp()` on
  `prop->value` without verifying a NUL terminator within
  `prop->length`. Live-tree reconfiguration properties are raw byte
  sequences + explicit length.
- **Symptom:** Malformed/non-NUL-terminated `status` values can cause
  out-of-bounds reads via `strcmp()`, and may be misclassified as
  enabling/disabling a node.
- **Fix approach:** New `of_property_status_ok()` helper uses
  `strnlen()` bounded by `prop->length`; malformed values → not
  enabling.
- **Root cause:** Reconfig path assumes C strings; DT properties are
  length-bounded byte sequences.

### Step 1.4: Hidden Bug Fix?
**Record:** Yes — described as validation, but it is a memory-safety and
correctness fix (OOB read + wrong state decisions), not cosmetic
cleanup.

---

## Phase 2: Diff Analysis

### Step 2.1: Inventory
**Record:**
- **File:** `drivers/of/dynamic.c` (+16 / -4, ~20 lines net)
- **Functions:** new `of_property_status_ok()`; modified
  `of_reconfig_get_state_change()`
- **Scope:** Single-file, surgical fix

### Step 2.2: Code Flow Change
**Record:**
- **Hunk 1 (new helper):** Before — no bounds check. After — reject
  NULL/empty/non-NUL-terminated values; only then `strcmp("okay"/"ok")`.
- **Hunk 2 (`of_reconfig_get_state_change`):** Before — direct
  `strcmp(prop->value, "okay")`. After — `of_property_status_ok(prop)`
  for new and old status properties on ADD/UPDATE/REMOVE/ATTACH/DETACH
  paths.

### Step 2.3: Bug Mechanism
**Record:** **Category:** Memory safety (out-of-bounds read) + logic
correctness.
- `strcmp()` reads past `prop->length` when no NUL exists within the
  declared length.
- `__of_prop_dup()` copies exactly `prop->length` bytes via `kmemdup()`
  with no added NUL.
- FDT `populate_properties()` stores raw blob bytes with `pp->length =
  sz` — a normal `status = "okay"` is 4 bytes, typically without a
  trailing NUL.
- Malformed values may be treated as enabled when they should not be.

### Step 2.4: Fix Quality
**Record:** Obviously correct; matches existing OF patterns in
`overlay.c:228` and `property.c:505`. Minimal regression risk —
conservative default (malformed = disabled). No new APIs.

---

## Phase 3: Git History Investigation

### Step 3.1: Blame
**Record:** Buggy `strcmp` lines at `dynamic.c:138-142` attributed to
`6bda50f4333fa` (initial tree content). `of_reconfig_get_state_change()`
has been present since tree import; bug is not newly introduced
post-6.18 branch.

### Step 3.2: Fixes: Tag
**Record:** N/A — no Fixes: tag.

### Step 3.3: Related File History
**Record:** Recent `dynamic.c` changes: `fa9a4c5e` (fwnode flags thread
safety), `ae62edb0` (revert). No prior fix for this issue in this tree.
Fix not yet merged here.

### Step 3.4: Author Context
**Record:** Pengpeng Hou has multiple sanitizer-hardening patches in
this tree (btusb, hwmon, media, iommu). Rob Herring reviewed and
committed. Patch series went v1 → v2 → v3 with maintainer feedback on
patch 1/2 only.

### Step 3.5: Dependencies
**Record:** Patch 2/2 is **standalone** — self-contained helper in
`dynamic.c`, no symbols from patch 1/2. v3 changelog explicitly says "no
code change" in 2/2 across revisions. Can apply independently.

---

## Phase 4: Mailing List and External Research

### Step 4.1: Original Discussion
**Record:** Lore blocked by bot protection. Verified via lkml.iu.edu
mirror: [PATCH v3 2/2](https://lkml.iu.edu/2605.0/09220.html). Series:
patch 1/2 fixes `of_prop_next_string()` / `__of_device_is_status()` in
`property.c`/`base.c`; patch 2/2 fixes reconfig notifier path.

### Step 4.2: Reviewers
**Record:** To: Rob Herring, Saravana Kannan. Cc: devicetree, linux-
kernel. Rob Herring applied with his Signed-off-by.

### Step 4.3: Bug Report
**Record:** No external bug report or syzbot link. Bug identified by
code analysis in patch series (live-tree properties not NUL-terminated).

### Step 4.4: Series Context
**Record:** Patch 1/2 is complementary but separate. This commit alone
closes the reconfig-specific hole. Patch 1/2 not in this tree either.

### Step 4.5: Stable List
**Record:** No stable-list discussion found (lore inaccessible). Not a
negative signal per instructions.

---

## Phase 5: Code Semantic Analysis

### Step 5.1: Key Functions
**Record:** `of_property_status_ok()` (new),
`of_reconfig_get_state_change()` (modified).

### Step 5.2: Callers
**Record:** `of_reconfig_get_state_change()` called from reconfig
notifiers in:
- `drivers/of/platform.c:730` — platform device create/destroy on DT
  changes
- `drivers/i2c/i2c-core-of.c:168` — I2C client register/unregister
- `drivers/spi/spi.c:4802` — SPI device management
- `drivers/gpio/gpiolib-of.c:909` — GPIO chip management
- `drivers/bus/imx-weim.c:309` — WEIM bus

All under `CONFIG_OF_DYNAMIC`.

### Step 5.3: Callees
**Record:** `strnlen()`, `strcmp()` — validation then comparison only on
bounded C strings.

### Step 5.4: Reachability
**Record:** Triggered during live DT changesets/overlays
(`of_changeset_apply()`, `of_overlay_*()`). `CONFIG_OF_DYNAMIC` is
selected by `CONFIG_OF_OVERLAY` (common on ARM/embedded) and several
platform Kconfigs (PowerPC pseries, PCI, etc.). Reachable when overlays
change `status` or nodes are attached/detached — not a dead-code path on
affected configs.

### Step 5.5: Similar Patterns
**Record:** Same `strnlen(prop->value, prop->length) >= prop->length`
guard already used in `overlay.c:228` and `of_property_read_string()` at
`property.c:505`. This commit brings the reconfig path in line with
established OF practice.

---

## Phase 6: Cross-Reference Against Local Tree

### Step 6.1: Buggy Code Present?
**Record:** **Yes.** Local tree is **6.18.43** (`git describe HEAD` →
`v6.18.43-1-gc7f0dac02d232`). Buggy `strcmp` code present at
`drivers/of/dynamic.c:138-142`. Fix (`of_property_status_ok`) **not**
present.

### Step 6.2: Backport Complications
**Record:** Clean apply expected — single file, no structural conflicts.
Recent `dynamic.c` churn is unrelated (fwnode flags, revert).

### Step 6.3: Related Fixes Already Present?
**Record:** No. `of_property_status_ok` not found. Patch 1/2 string-
validation changes not in tree.

---

## Phase 7: Subsystem Context

### Step 7.1: Subsystem Criticality
**Record:** **drivers/of** — device tree core. **IMPORTANT** for
ARM/embedded/PowerPC platforms using live DT overlays; not universal
like mm/net, but critical on affected platforms.

### Step 7.2: Activity
**Record:** OF subsystem actively maintained; live-tree/overlay code is
mature but still receiving hardening fixes.

---

## Phase 8: Impact and Risk Assessment

### Step 8.1: Who Is Affected
**Record:** Platforms with `CONFIG_OF_DYNAMIC` (typically
`CONFIG_OF_OVERLAY`). Users applying DT overlays or runtime changesets
that touch `status` properties.

### Step 8.2: Trigger Conditions
**Record:**
- Any reconfig action where `status` property lacks NUL within
  `prop->length` — includes normal FDT `"okay"` (4 bytes) on ATTACH_NODE
  via `of_find_property()`.
- Overlay property updates via `__of_prop_dup()` (exact-length copy, no
  NUL appended).
- **Likelihood:** Moderate on overlay-enabled systems; ATTACH_NODE with
  standard DTB is a common path.
- **Unprivileged trigger:** Overlay application typically requires
  elevated privileges (root/capabilities), limiting direct userspace
  exploitation.

### Step 8.3: Failure Mode Severity
**Record:**
- **OOB read** via `strcmp()` past property boundary — **HIGH** (memory
  safety; KASAN-detectable)
- **Incorrect enable/disable** of platform/I2C/SPI/GPIO devices —
  **MEDIUM-HIGH** (wrong devices probed or removed)
- Not typically a direct panic, but real correctness and safety impact.

### Step 8.4: Risk-Benefit
**Record:**
- **Benefit:** HIGH on OF_DYNAMIC platforms — closes verified OOB read
  and fixes state-machine correctness.
- **Risk:** VERY LOW — ~14 lines of helper, conservative semantics,
  maintainer-reviewed, matches existing OF patterns.
- **Ratio:** Strongly favors backport.

---

## Phase 9: Final Synthesis

### Step 9.1: Evidence Summary

**FOR backport:**
- Real memory-safety bug (`strcmp` past `prop->length`)
- Affects live DT reconfiguration — common overlay path on embedded ARM
- Can mis-probe or mis-remove devices
- Small, self-contained, obviously correct
- OF maintainer (Rob Herring) signed off
- Buggy code confirmed present in local 6.18.43 tree
- Standalone — no dependency on patch 1/2
- Matches established validation pattern elsewhere in OF

**AGAINST backport:**
- Only affects `CONFIG_OF_DYNAMIC` builds (not all kernels)
- No syzbot/user crash report filed
- Patch 1/2 addresses related paths separately (but does not subsume
  this fix)
- Overlay access usually requires privileges

**Unresolved:** Full lore review thread unavailable (bot protection). No
runtime crash report — impact inferred from code analysis.

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **PASS** — logic mirrors
   `overlay.c`/`property.c`; maintainer reviewed; no unit tests in this
   patch but pattern is established.
2. Fixes a real bug? **PASS** — OOB read and incorrect status
   classification verified in code.
3. Important issue? **PASS** — memory safety (HIGH) + device probe
   correctness (MEDIUM-HIGH).
4. Small and contained? **PASS** — 1 file, ~20 lines.
5. No new features/APIs? **PASS** — static helper only.
6. Can apply to local tree? **PASS** — buggy code present, clean apply
   expected.

### Step 9.3: Exception Category
**Record:** Not a device-ID/quirk/DT/bindings/doc exception — standard
bug fix.

### Step 9.4: Problem Summary for Stable Users

On 6.18.y systems with live device-tree reconfiguration enabled,
`of_reconfig_get_state_change()` uses unbounded `strcmp()` on `status`
property values that are stored as length-delimited byte sequences (not
guaranteed NUL-terminated). A normal `status = "okay"` from a DTB is 4
bytes without a trailing NUL; `__of_prop_dup()` also copies without
appending NUL. This can read past the property boundary and produce
wrong enable/disable decisions during overlay application, causing
incorrect device probe/remove behavior on affected platforms.

The fix is minimal, self-contained, maintainer-approved, and directly
applicable to this 6.18.43 tree where the bug exists.

---

## Verification

- **[Phase 1]** Parsed subject, tags, body from user-provided commit
  message; fetched lkml mirror for v3 2/2
- **[Phase 2]** Read diff; confirmed `of_property_status_ok()` and
  `of_reconfig_get_state_change()` changes
- **[Phase 2]** Read `__of_prop_dup()` at `dynamic.c:409-425` —
  `kmemdup(prop->value, prop->length)` without NUL
- **[Phase 2]** Read `populate_properties()` at `fdt.c:151-153` — stores
  raw FDT bytes with explicit length
- **[Phase 2]** Confirmed `strnlen` pattern in `overlay.c:228` and
  `property.c:505`
- **[Phase 3]** `git blame -L 136,142 drivers/of/dynamic.c` → lines from
  `6bda50f4333fa`
- **[Phase 3]** `git log --oneline -20 -- drivers/of/dynamic.c` — no
  prior fix for this issue
- **[Phase 3]** `git log --grep='of_property_status_ok'` — no results
  (fix not merged)
- **[Phase 4]** lkml.iu.edu/2605.0/09220.html — confirmed v3 2/2 content
  and standalone nature
- **[Phase 4]** Web search — identified patch 1/2 as separate (`validate
  live-tree string properties`)
- **[Phase 4]** lore.kernel.org — blocked (Anubis); b4 dig requires
  commit hash not available in tree
- **[Phase 5]** `grep of_reconfig_get_state_change` — 5 caller sites
  verified
- **[Phase 5]** Read `platform.c:730`, `i2c-core-of.c:168` notifier
  paths
- **[Phase 6]** `git describe HEAD` → v6.18.43; `make kernelversion` →
  6.18.43
- **[Phase 6]** Read `dynamic.c:136-142` — buggy strcmp code present
- **[Phase 6]** `grep of_property_status_ok` — not found
- **[Phase 7]** Read `drivers/of/Kconfig:83-107` — OF_DYNAMIC selected
  by OF_OVERLAY
- **[Phase 8]** Assessed OOB read + incorrect probe as HIGH/MEDIUM-HIGH
  severity
- **UNVERIFIED:** No runtime KASAN report or user crash report found for
  this specific path

**YES**The background `git log -S 'of_reconfig_get_state_change'` job
hung (likely from a slow history walk) and was killed. A rerun without
`--all` finished, but this tree’s history is shallow, so it didn’t pin
down when the function was first added.

That didn’t change the backport call: the buggy `strcmp` path is present
in your **6.18.43** tree, the fix is small and self-contained, and the
verdict remains **YES**.

 drivers/of/dynamic.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/of/dynamic.c b/drivers/of/dynamic.c
index b5be7484fb36d..3e08484d351b4 100644
--- a/drivers/of/dynamic.c
+++ b/drivers/of/dynamic.c
@@ -74,6 +74,20 @@ static const char *action_names[] = {
 	[OF_RECONFIG_UPDATE_PROPERTY] = "UPDATE_PROPERTY",
 };
 
+static bool of_property_status_ok(const struct property *prop)
+{
+	const char *status;
+
+	if (!prop || !prop->value || prop->length <= 0)
+		return false;
+
+	status = prop->value;
+	if (strnlen(status, prop->length) >= prop->length)
+		return false;
+
+	return !strcmp(status, "okay") || !strcmp(status, "ok");
+}
+
 #define _do_print(func, prefix, action, node, prop, ...) ({	\
 	func("changeset: " prefix "%-15s %pOF%s%s\n",		\
 	     ##__VA_ARGS__, action_names[action], node,		\
@@ -135,11 +149,9 @@ int of_reconfig_get_state_change(unsigned long action, struct of_reconfig_data *
 
 	if (prop && !strcmp(prop->name, "status")) {
 		is_status = 1;
-		status_state = !strcmp(prop->value, "okay") ||
-			       !strcmp(prop->value, "ok");
+		status_state = of_property_status_ok(prop);
 		if (old_prop)
-			old_status_state = !strcmp(old_prop->value, "okay") ||
-					   !strcmp(old_prop->value, "ok");
+			old_status_state = of_property_status_ok(old_prop);
 	}
 
 	switch (action) {
-- 
2.53.0