Linux Serial subsystem development
 help / color / mirror / Atom feed
From: Sasha Levin <sashal@kernel.org>
To: patches@lists.linux.dev, stable@vger.kernel.org
Cc: Stepan Ionichev <sozdayvek@gmail.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Sasha Levin <sashal@kernel.org>,
	jirislaby@kernel.org, linux-kernel@vger.kernel.org,
	linux-serial@vger.kernel.org
Subject: [PATCH AUTOSEL 6.18] tty: serial: 8250: protect against NULL uart->port.dev in register
Date: Mon, 31 Aug 2026 09:21:45 -0400	[thread overview]
Message-ID: <20260831133314.4125787-77-sashal@kernel.org> (raw)
In-Reply-To: <20260831133314.4125787-1-sashal@kernel.org>

From: Stepan Ionichev <sozdayvek@gmail.com>

[ Upstream commit 941c9f84c9b6310f7aaa1c8c785dcc634ee33050 ]

serial8250_register_8250_port() conditionally copies uart->port.dev
from up->port.dev only when up->port.dev is non-NULL:

	if (up->port.dev) {
		uart->port.dev = up->port.dev;
		...
	}

So if both the existing uart slot and up have a NULL ->dev,
uart->port.dev remains NULL. The very next ACPI companion check
then dereferences it unconditionally:

	if (!has_acpi_companion(uart->port.dev)) {

has_acpi_companion() reads dev->fwnode without a NULL guard
(include/linux/acpi.h), so this NULL-derefs the kernel for the
remaining no-dev case rather than just skipping the
mctrl_gpio_init() initialisation as intended.

smatch flags the inconsistency:

  drivers/tty/serial/8250/8250_core.c:767
  serial8250_register_8250_port() error: 'uart->port.dev' could be
  null (see line 719)

Guard the call with a NULL check so register continues to work
for callers that legitimately have no parent device (legacy
non-OF/non-ACPI registrations).

No functional change for callers that pass a non-NULL ->dev.

Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
Link: https://patch.msgid.link/20260508181237.11146-1-sozdayvek@gmail.com
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Phase 1: Commit Message Forensics

### Step 1.1: Subject Line
**Record:** `[tty: serial: 8250]` `[protect]` — Guard
`has_acpi_companion()` against a NULL `uart->port.dev` in
`serial8250_register_8250_port()`.

### Step 1.2: Tags
**Record:**
- **Signed-off-by:** Stepan Ionichev `<sozdayvek@gmail.com>` (author)
- **Signed-off-by:** Greg Kroah-Hartman `<gregkh@linuxfoundation.org>`
  (TTY maintainer)
- **Link:**
  `https://patch.msgid.link/20260508181237.11146-1-sozdayvek@gmail.com`
- No Fixes:, Reported-by:, Tested-by:, Reviewed-by:, Acked-by:, or Cc:
  stable tags
- Notable: Greg Kroah-Hartman merged it; smatch static analysis cited in
  body

### Step 1.3: Body Analysis
**Record:**
- **Bug:** `uart->port.dev` is only copied from `up->port.dev` when the
  latter is non-NULL. If both are NULL,
  `has_acpi_companion(uart->port.dev)` dereferences `dev->fwnode`
  unconditionally.
- **Symptom:** Kernel NULL pointer dereference (oops) during port
  registration.
- **Trigger:** Legacy callers that legitimately pass no parent `struct
  device` (non-OF/non-ACPI registration paths).
- **Root cause:** Missing NULL guard before `has_acpi_companion()`,
  which does not handle NULL internally.
- **Version info:** Not specified in message; analysis shows the ACPI
  check dates to 2019 in this tree.

### Step 1.4: Hidden Bug Fix?
**Record:** No — this is an explicit NULL-dereference fix, not disguised
cleanup.

---

## Phase 2: Diff Analysis

### Step 2.1: Inventory
**Record:**
- **Files:** `drivers/tty/serial/8250/8250_core.c` (+1/-1 line)
- **Function:** `serial8250_register_8250_port()`
- **Scope:** Single-file, single-line surgical fix

### Step 2.2: Code Flow Change
**Record:**
- **Before:** `if (!has_acpi_companion(uart->port.dev))` — always calls
  `has_acpi_companion()`, even when `uart->port.dev` is NULL.
- **After:** `if (uart->port.dev &&
  !has_acpi_companion(uart->port.dev))` — skips ACPI check and
  `mctrl_gpio_init()` when there is no device.
- **Path affected:** Port registration when `up->port.dev` is NULL and
  the target uart slot also has NULL `dev`.

### Step 2.3: Bug Mechanism
**Record:** **Category:** NULL pointer dereference (memory safety).
- `has_acpi_companion()` in `include/linux/acpi.h` does `return
  is_acpi_device_node(dev->fwnode);` with no NULL check.
- `uart->port.dev` is only assigned inside `if (up->port.dev) {
  uart->port.dev = up->port.dev; ... }`.
- When both are NULL, unconditional `has_acpi_companion()` crashes on
  CONFIG_ACPI builds.

### Step 2.4: Fix Quality
**Record:** Obviously correct and minimal. Matches the existing
conditional-copy pattern for `uart->port.dev`. No functional change when
`dev` is non-NULL. Very low regression risk.

---

## Phase 3: Git History Investigation

### Step 3.1: Blame
**Record:**
- `has_acpi_companion()` check introduced in `4a96895f74c96`
  ("tty/serial/8250: use mctrl_gpio helpers", 2019-06-20).
- Conditional `up->port.dev` copy and ACPI check reorganized in
  `05b537a175442c` (2025-06-11 refactor); bug pattern unchanged.
- `4a96895f74c96` is an ancestor of HEAD in this tree — bug present
  since 2019 here.

### Step 3.2: Fixes: Tag
**Record:** N/A — no Fixes: tag. Bug introduced by `4a96895f74c96`,
confirmed present in v6.18.44.

### Step 3.3: Related File History
**Record:** Recent `8250_core.c` changes are style/refactor (guard(),
hashtable, CIR condition). No prior fix for this NULL-deref. Standalone
one-liner.

### Step 3.4: Author Context
**Record:** Stepan Ionichev has other 8250 patches (e.g. `8250_dw` clk
notifier fix). Greg Kroah-Hartman merged this one.

### Step 3.5: Dependencies
**Record:** None. Self-contained; no prerequisite commits. `git apply
--check` succeeds cleanly on this tree.

---

## Phase 4: Mailing List and External Research

### Step 4.1: Original Discussion
**Record:** `b4 dig -c` could not be run (commit not in local tree).
Lore/patch.msgid.link blocked (403/Anubis). Could not retrieve thread
content.

### Step 4.2: Reviewers
**Record:** UNVERIFIED — `b4 dig -w` not possible without commit in
tree. Greg Kroah-Hartman Signed-off-by confirms maintainer acceptance.

### Step 4.3: Bug Report
**Record:** smatch static analysis cited in commit message. No syzbot or
user crash report. Real bug confirmed by code inspection.

### Step 4.4: Series Context
**Record:** Standalone patch, not part of a series.

### Step 4.5: Stable List History
**Record:** UNVERIFIED — lore.kernel.org inaccessible from this
environment.

---

## Phase 5: Code Semantic Analysis

### Step 5.1: Key Functions
**Record:** `serial8250_register_8250_port()` modified.

### Step 5.2: Callers
**Record:** Called from 30+ drivers during probe/init. Callers that do
**not** set `port.dev`:
- `drivers/char/mwave/mwavedd.c` — `memset(&uart, 0, ...)`, no `dev`
- `drivers/misc/ibmasm/uart.c` — same pattern
- `drivers/tty/serial/8250/8250_hp300.c` — FRODO path (line ~262), no
  `dev`
- `drivers/tty/serial/8250/8250_men_mcb.c` — `memset`, no `dev`
- `drivers/tty/serial/8250/8250_dfl.c` — `uart = { }`, no `dev`

Also: `serial8250_unregister_port()` sets `uart->port.dev = NULL` when
no ISA devs (line 884), so re-registration without `dev` hits the bug
path.

### Step 5.3: Callees
**Record:** `has_acpi_companion()` → `is_acpi_device_node(dev->fwnode)`;
`mctrl_gpio_init()` for GPIO modem-control lines.

### Step 5.4: Reachability
**Record:** Triggered during driver probe/module init on CONFIG_ACPI
systems when legacy 8250 callers register ports without a `struct
device`. Not a syscall path, but real on ACPI x86/ARM servers using
those drivers.

### Step 5.5: Similar Patterns
**Record:** The `if (up->port.dev)` guard at line 749 already shows the
author knew `dev` can be NULL; the ACPI check was the inconsistent
omission smatch flagged.

---

## Phase 6: Cross-Reference Against Local Tree

### Step 6.1: Buggy Code in Tree?
**Record:** **YES.** Local tree is **v6.18.44** (`git describe HEAD`,
`make kernelversion`). Buggy line at `8250_core.c:763`:
```763:763:drivers/tty/serial/8250/8250_core.c
        if (!has_acpi_companion(uart->port.dev)) {
```
Fix is **not** yet applied (`git log --grep` found nothing).

### Step 6.2: Backport Complications
**Record:** Clean apply verified (`git apply --check` exit 0). No
conflicts expected.

### Step 6.3: Related Fixes Already Present?
**Record:** None found for this issue.

---

## Phase 7: Subsystem Context

### Step 7.1: Subsystem
**Record:** `drivers/tty/serial/8250` — IMPORTANT (widely used serial
core; many platform/PCI drivers depend on it).

### Step 7.2: Activity
**Record:** Actively maintained; recent refactors in 2025, but core
registration logic stable since 2019 ACPI check.

---

## Phase 8: Impact and Risk Assessment

### Step 8.1: Who Is Affected
**Record:** Users of legacy 8250 registration paths on **CONFIG_ACPI**
kernels — IBM mwave, IBM ASM service processor UART, MEN MCB, DFL UART,
HP300 FRODO, and similar callers without `port.dev`.

### Step 8.2: Trigger Conditions
**Record:** Port registration with both `up->port.dev` and
`uart->port.dev` NULL. Uncommon but legitimate. Requires CONFIG_ACPI
(non-ACPI stub returns `false` without dereferencing).

### Step 8.3: Failure Mode
**Record:** NULL pointer dereference in `has_acpi_companion()` → kernel
oops during driver probe. **Severity: HIGH** (system crash on affected
hardware/config).

### Step 8.4: Risk-Benefit
**Record:**
- **Benefit:** Prevents kernel oops on ACPI systems using legacy 8250
  registration; fixes latent bug since 2019.
- **Risk:** Very low — one-line NULL guard, no behavior change when
  `dev` is set.
- **Ratio:** Strongly favors backport.

---

## Phase 9: Final Synthesis

### Step 9.1: Evidence Summary

**FOR backport:**
- Real NULL pointer dereference, confirmed by code and smatch
- Kernel oops on probe for legitimate legacy callers
- One-line, obviously correct fix
- Bug present in v6.18.44 since 2019 (`4a96895f74c96`)
- Applies cleanly
- Greg Kroah-Hartman merged
- No new APIs or features

**AGAINST backport:**
- Affects only CONFIG_ACPI + NULL-dev registration paths (narrower than
  universal crash)
- No user crash reports or syzbot hits in message
- Lore review thread not retrievable

**Unresolved:** Full mailing-list review discussion; explicit stable
nomination by reviewers.

### Step 9.2: Stable Rules Checklist
1. Obviously correct and tested? **PASS** — logic is trivial; smatch-
   driven; maintainer merged.
2. Fixes a real bug? **PASS** — NULL deref on ACPI builds.
3. Important issue? **PASS** — kernel oops (HIGH severity).
4. Small and contained? **PASS** — 1 line, 1 file.
5. No new features/APIs? **PASS**.
6. Can apply to local tree? **PASS** — verified clean apply.

### Step 9.3: Exception Categories
**Record:** N/A — standard bug fix, not device-ID/quirk/build/doc
exception.

### Step 9.4: Decision Rationale
This is a textbook stable backport: a latent NULL-dereference in widely-
used 8250 core code, present in v6.18.44 since 2019, triggered by
legitimate legacy registration paths on ACPI systems, fixed by a single
obviously-correct NULL guard with no behavior change for normal callers.

---

## Verification

- [Phase 1] Parsed subject, tags, body from user-provided commit message
- [Phase 2] Read diff: 1-line change in
  `serial8250_register_8250_port()`
- [Phase 2] Read `has_acpi_companion()` at `include/linux/acpi.h:85-88`
  — no NULL guard
- [Phase 3] `git describe HEAD` → v6.18.44; `make kernelversion` →
  6.18.44
- [Phase 3] `git blame -L 750,770` — ACPI check from `05b537a175442c`,
  original from `4a96895f74c96`
- [Phase 3] `git show 4a96895f74c96` — introduced `has_acpi_companion()`
  without NULL guard (2019)
- [Phase 3] `git show 05b537a175442c^` — same bug pattern before
  refactor
- [Phase 3] `git merge-base --is-ancestor 4a96895f74c96 HEAD` → ancestor
  confirmed
- [Phase 3] `git log --grep` — fix not in tree
- [Phase 4] `b4 dig` — failed (commit not in tree; wrong args for
  search)
- [Phase 4] Lore/patch.msgid.link fetch — blocked (403/Anubis)
- [Phase 5] `grep serial8250_register_8250_port` — 30+ callers found
- [Phase 5] Verified NULL-dev callers: `mwavedd.c`, `ibmasm/uart.c`,
  `8250_hp300.c`, `8250_men_mcb.c`, `8250_dfl.c`
- [Phase 5] `serial8250_unregister_port()` sets `uart->port.dev = NULL`
  at line 884
- [Phase 6] Buggy code at `8250_core.c:763` without NULL guard —
  confirmed present
- [Phase 6] `git apply --check` — patch applies cleanly (exit 0)
- [Phase 8] Non-ACPI stub at `acpi.h:868-870` returns false without
  dereference — bug ACPI-only

**YES**The background `git log` lookup finished; it matched the rest of
the investigation.

**Summary:** For **v6.18.44**, this commit should be backported.

It fixes a real NULL pointer dereference in
`serial8250_register_8250_port()` on CONFIG_ACPI builds: when both
`up->port.dev` and `uart->port.dev` are NULL, `has_acpi_companion()`
dereferences `dev->fwnode`. That can oops during probe for legacy
callers (mwave, ibmasm, MEN MCB, DFL UART, etc.) that register without a
parent device. The bug has been present since the 2019 `mctrl_gpio` ACPI
check (`4a96895f74c96`); the fix is a one-line NULL guard and applies
cleanly to this tree.

**YES**

 drivers/tty/serial/8250/8250_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index bfa421ab32536..8c169756710b5 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -760,7 +760,7 @@ int serial8250_register_8250_port(const struct uart_8250_port *up)
 	 * Only call mctrl_gpio_init(), if the device has no ACPI
 	 * companion device
 	 */
-	if (!has_acpi_companion(uart->port.dev)) {
+	if (uart->port.dev && !has_acpi_companion(uart->port.dev)) {
 		struct mctrl_gpios *gpios = mctrl_gpio_init(&uart->port, 0);
 		if (IS_ERR(gpios)) {
 			ret = PTR_ERR(gpios);
-- 
2.53.0


  parent reply	other threads:[~2026-08-31 13:35 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20260831133314.4125787-1-sashal@kernel.org>
2026-08-31 13:21 ` [PATCH AUTOSEL 6.18-5.10] serial: 8250: fix possible ISR soft lockup Sasha Levin
2026-08-31 13:58   ` sashiko-bot
2026-08-31 13:21 ` Sasha Levin [this message]
2026-08-31 14:21   ` [PATCH AUTOSEL 6.18] tty: serial: 8250: protect against NULL uart->port.dev in register sashiko-bot
2026-08-31 13:21 ` [PATCH AUTOSEL 6.18] serial: 8250_port: recognize UPIO_AU Sasha Levin
2026-08-31 14:19   ` sashiko-bot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260831133314.4125787-77-sashal@kernel.org \
    --to=sashal@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jirislaby@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-serial@vger.kernel.org \
    --cc=patches@lists.linux.dev \
    --cc=sozdayvek@gmail.com \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox