All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH AUTOSEL 6.18-5.10] powercap: fix sscanf() error return value handling
@ 2025-12-23 10:05 Sasha Levin
  2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.6] netfilter: nf_tables: avoid chain re-validation if possible Sasha Levin
                   ` (9 more replies)
  0 siblings, 10 replies; 14+ messages in thread
From: Sasha Levin @ 2025-12-23 10:05 UTC (permalink / raw)
  To: patches, stable
  Cc: Sumeet Pawnikar, Rafael J. Wysocki, Sasha Levin, rafael, linux-pm

From: Sumeet Pawnikar <sumeet4linux@gmail.com>

[ Upstream commit efc4c35b741af973de90f6826bf35d3b3ac36bf1 ]

Fix inconsistent error handling for sscanf() return value check.

Implicit boolean conversion is used instead of explicit return
value checks. The code checks if (!sscanf(...)) which is incorrect
because:
 1. sscanf returns the number of successfully parsed items
 2. On success, it returns 1 (one item passed)
 3. On failure, it returns 0 or EOF
 4. The check 'if (!sscanf(...))' is wrong because it treats
    success (1) as failure

All occurrences of sscanf() now uses explicit return value check.
With this behavior it returns '-EINVAL' when parsing fails (returns
0 or EOF), and continues when parsing succeeds (returns 1).

Signed-off-by: Sumeet Pawnikar <sumeet4linux@gmail.com>
[ rjw: Subject and changelog edits ]
Link: https://patch.msgid.link/20251207151549.202452-1-sumeet4linux@gmail.com
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

Looking at this commit carefully to assess its suitability for stable
backporting.

## Commit Analysis

### 1. COMMIT MESSAGE ANALYSIS

The commit fixes incorrect error handling for `sscanf()` return value
checks in the powercap subsystem. The message claims that `if
(!sscanf(...))` is wrong because:
- `sscanf()` returns the number of successfully parsed items
- On success: returns 1
- On failure: returns 0 or EOF (typically -1)

### 2. CODE CHANGE ANALYSIS

Three identical changes in `drivers/powercap/powercap_sys.c`:
```c
// Before:
if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id))

// After:
if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1)
```

Let me trace through the bug:
- When `sscanf()` returns 0 (no match): `!0` = 1 (true) → error path
  taken ✓
- When `sscanf()` returns 1 (success): `!1` = 0 (false) → continues ✓
- When `sscanf()` returns EOF (-1): `!(-1)` = 0 (false) → **error path
  NOT taken** ✗

The critical issue is the EOF case. If `sscanf()` returns EOF (e.g.,
empty string input), the check passes incorrectly and execution
continues with an **uninitialized `id` variable**. This then leads to:
```c
if (id >= power_zone->const_id_cnt)  // garbage comparison
    return -EINVAL;
pconst = &power_zone->constraints[id];  // POTENTIAL OUT-OF-BOUNDS
ACCESS
```

### 3. CLASSIFICATION

This is a **bug fix** that prevents:
- Potential out-of-bounds array access
- Use of uninitialized variable
- Possible kernel crash or memory corruption in edge cases

Not a feature addition, code cleanup, or optimization.

### 4. SCOPE AND RISK ASSESSMENT

- **Lines changed**: 3 lines (identical pattern)
- **Files affected**: 1 file
- **Risk**: Extremely low - the change only makes the check stricter and
  more explicit
- **Could break anything?**: No - the new check `!= 1` is strictly more
  conservative than `!`

### 5. USER IMPACT

The powercap subsystem manages:
- Intel RAPL power capping
- Power domain constraints
- Used by tools like powertop, thermald

While the EOF triggering condition is rare (would require malformed
attribute names), the consequence (OOB access) could be severe.

### 6. STABILITY INDICATORS

- Signed-off by Rafael J. Wysocki (Intel power management maintainer)
- Self-contained fix with no dependencies
- The powercap subsystem has existed since kernel 3.13

### 7. DEPENDENCY CHECK

- No dependencies on other commits
- Simple standalone fix
- Code exists in all active stable trees

## Summary

**What it fixes**: Incorrect sscanf error handling that could allow
execution with uninitialized data, potentially leading to out-of-bounds
array access.

**Why it matters for stable**: While the triggering condition (EOF from
sscanf) is rare, the fix prevents a potential memory safety issue. The
change is trivially correct with zero regression risk.

**Meets stable criteria**:
- ✓ Obviously correct (explicit `!= 1` check is cleaner and more robust)
- ✓ Fixes a real bug (uninitialized variable use, potential OOB access)
- ✓ Small and contained (3 identical one-line changes)
- ✓ No new features
- ✓ No dependencies

**Risk vs benefit**: The benefit (preventing potential memory
corruption) outweighs the near-zero risk of regression.

**YES**

 drivers/powercap/powercap_sys.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/powercap/powercap_sys.c b/drivers/powercap/powercap_sys.c
index d14b36b75189..1ff369880beb 100644
--- a/drivers/powercap/powercap_sys.c
+++ b/drivers/powercap/powercap_sys.c
@@ -68,7 +68,7 @@ static ssize_t show_constraint_##_attr(struct device *dev, \
 	int id; \
 	struct powercap_zone_constraint *pconst;\
 	\
-	if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) \
+	if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1) \
 		return -EINVAL; \
 	if (id >= power_zone->const_id_cnt)	\
 		return -EINVAL; \
@@ -93,7 +93,7 @@ static ssize_t store_constraint_##_attr(struct device *dev,\
 	int id; \
 	struct powercap_zone_constraint *pconst;\
 	\
-	if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id)) \
+	if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1) \
 		return -EINVAL; \
 	if (id >= power_zone->const_id_cnt)	\
 		return -EINVAL; \
@@ -162,7 +162,7 @@ static ssize_t show_constraint_name(struct device *dev,
 	ssize_t len = -ENODATA;
 	struct powercap_zone_constraint *pconst;
 
-	if (!sscanf(dev_attr->attr.name, "constraint_%d_", &id))
+	if (sscanf(dev_attr->attr.name, "constraint_%d_", &id) != 1)
 		return -EINVAL;
 	if (id >= power_zone->const_id_cnt)
 		return -EINVAL;
-- 
2.51.0


^ permalink raw reply related	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-01-14 20:00 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-12-23 10:05 [PATCH AUTOSEL 6.18-5.10] powercap: fix sscanf() error return value handling Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.6] netfilter: nf_tables: avoid chain re-validation if possible Sasha Levin
2025-12-23 10:12   ` Florian Westphal
2026-01-14 20:00     ` Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.12] spi: mt65xx: Use IRQF_ONESHOT with threaded IRQ Sasha Levin
2025-12-23 10:05   ` Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-5.10] can: j1939: make j1939_session_activate() fail if device is no longer registered Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-5.10] powercap: fix race condition in register_control_type() Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18] block: validate pi_offset integrity limit Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.6] drm/amd/display: Fix DP no audio issue Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.12] ata: libata-core: Disable LPM on ST2000DM008-2FR102 Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18] accel/amdxdna: Block running under a hypervisor Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.12] drm/amdkfd: Fix improper NULL termination of queue restore SMI event string Sasha Levin
2025-12-23 10:05 ` [PATCH AUTOSEL 6.18-6.12] net: sfp: extend Potron XGSPON quirk to cover additional EEPROM variant Sasha Levin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.