[PATCH AUTOSEL 6.19-5.10] media: solo6x10: Check for out of bounds chip_id

[Sasha Levin <sashal@kernel.org>]

From: Kees Cook <kees@kernel.org>

[ Upstream commit 0fdf6323c35a134f206dcad5babb4ff488552076 ]

Clang with CONFIG_UBSAN_SHIFT=y noticed a condition where a signed type
(literal "1" is an "int") could end up being shifted beyond 32 bits,
so instrumentation was added (and due to the double is_tw286x() call
seen via inlining), Clang decides the second one must now be undefined
behavior and elides the rest of the function[1]. This is a known problem
with Clang (that is still being worked on), but we can avoid the entire
problem by actually checking the existing max chip ID, and now there is
no runtime instrumentation added at all since everything is known to be
within bounds.

Additionally use an unsigned value for the shift to remove the
instrumentation even without the explicit bounds checking.

Link: https://github.com/ClangBuiltLinux/linux/issues/2144 [1]
Suggested-by: Nathan Chancellor <nathan@kernel.org>
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Hans Verkuil <hverkuil+cisco@kernel.org>
[hverkuil: fix checkpatch warning for is_tw286x]
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Analysis of media: solo6x10: Check for out of bounds chip_id

### 1. Commit Message Analysis

The commit addresses two related issues:
- **UBSAN shift sanitizer warning**: With `CONFIG_UBSAN_SHIFT=y`, Clang
  detects that a signed `int` literal `1` could be shifted beyond 32
  bits in the `is_tw286x()` macro, which is undefined behavior (UB).
- **Clang miscompilation due to UB**: Because of the UB, Clang's
  optimizer concludes the second `is_tw286x()` call (after inlining)
  must be UB and **elides the rest of the function**. This means code is
  silently dropped by the compiler.
- The fix adds explicit bounds checking (`chip_num >= TW_NUM_CHIP`) and
  changes the shift to use an unsigned value (`1U`).

### 2. Code Change Analysis

Three distinct changes:

**a) Macro fix (`1` → `1U` and added parentheses):**
```c
-#define is_tw286x(__solo, __id) (!(__solo->tw2815 & (1 << __id)))
+#define is_tw286x(__solo, __id) (!((__solo)->tw2815 & (1U << (__id))))
```
- Changes signed shift to unsigned, eliminating undefined behavior for
  shifts ≥ 31
- Adds proper parenthesization (macro hygiene)
- This is a real UB fix — shifting a signed `1` by ≥ 31 bits is
  undefined in C

**b) Bounds check in `tw28_set_ctrl_val()`:**
```c
+       if (chip_num >= TW_NUM_CHIP)
+               return -EINVAL;
```
- `chip_num` is derived from `ch / 4` where `ch` is a `u8` parameter
- `TW_NUM_CHIP` defines the maximum number of chips
- Without this check, an out-of-bounds `chip_num` would cause UB in the
  `is_tw286x()` shift and potentially out-of-bounds array access in
  `TW_CHIP_OFFSET_ADDR()`

**c) Same bounds check in `tw28_get_ctrl_val()`:**
```c
+       if (chip_num >= TW_NUM_CHIP)
+               return -EINVAL;
```
- Same protection for the read path

### 3. Bug Classification

This fixes **undefined behavior** that leads to **compiler-caused
miscompilation**. The Clang compiler, upon detecting that UB is
possible, optimizes away portions of the function. This is not a
theoretical concern — the linked ClangBuiltLinux issue (#2144) documents
the actual problem.

The consequences of the UB:
1. **Silent code elision**: The compiler removes code paths it deems
   unreachable due to UB, meaning the function silently does nothing in
   some cases
2. **Potential out-of-bounds access**: Without bounds checking, invalid
   `chip_id` values cause shifts beyond type width
3. **UBSAN runtime warnings**: Noise in kernel logs for users with
   sanitizers enabled

### 4. Scope and Risk Assessment

- **Files changed**: 1 file (`solo6x10-tw28.c`)
- **Lines changed**: ~6 lines of actual logic (macro change + 2 bounds
  checks)
- **Risk**: Very low — the macro change preserves semantics while
  removing UB; bounds checks add early returns for invalid inputs
- **Subsystem**: Media PCI driver (solo6x10) — contained, no core kernel
  impact
- **Could break something**: Extremely unlikely — the macro change only
  affects behavior for previously-UB cases, and the bounds checks only
  reject invalid chip IDs

### 5. User Impact

- Users building with Clang + UBSAN (increasingly common, especially on
  Android and ChromeOS) hit this directly
- The miscompilation affects correctness of the driver — functions may
  be silently broken
- The solo6x10 is a surveillance/DVR capture card used in production
  systems

### 6. Stable Kernel Criteria

| Criterion | Assessment |
|-----------|------------|
| Obviously correct | Yes — straightforward UB fix and bounds check |
| Fixes real bug | Yes — compiler miscompilation due to UB |
| Important issue | Medium — affects Clang users, prevents silent code
elision |
| Small and contained | Yes — 6 lines of logic in 1 file |
| No new features | Correct — pure bug fix |
| Tested | Yes — authored by Kees Cook, reviewed/applied by Hans Verkuil
|

### 7. Dependencies

The patch is self-contained. It only references `TW_NUM_CHIP` which is
already defined in the driver header. No dependencies on other commits.

### Decision

This is a legitimate bug fix addressing undefined behavior that causes
real miscompilation with Clang. The fix is small (6 lines of logic),
surgical, self-contained, obviously correct, and authored/reviewed by
well-known kernel developers. It prevents silent code elision by the
compiler and adds proper bounds checking. The risk of regression is
negligible.

**YES**

 drivers/media/pci/solo6x10/solo6x10-tw28.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/media/pci/solo6x10/solo6x10-tw28.c b/drivers/media/pci/solo6x10/solo6x10-tw28.c
index 1b7c22a9bc94f..8f53946c67928 100644
--- a/drivers/media/pci/solo6x10/solo6x10-tw28.c
+++ b/drivers/media/pci/solo6x10/solo6x10-tw28.c
@@ -166,7 +166,7 @@ static const u8 tbl_tw2865_pal_template[] = {
 	0x64, 0x51, 0x40, 0xaf, 0xFF, 0xF0, 0x00, 0xC0,
 };
 
-#define is_tw286x(__solo, __id) (!(__solo->tw2815 & (1 << __id)))
+#define is_tw286x(__solo, __id) (!((__solo)->tw2815 & (1U << (__id))))
 
 static u8 tw_readbyte(struct solo_dev *solo_dev, int chip_id, u8 tw6x_off,
 		      u8 tw_off)
@@ -686,6 +686,9 @@ int tw28_set_ctrl_val(struct solo_dev *solo_dev, u32 ctrl, u8 ch,
 	chip_num = ch / 4;
 	ch %= 4;
 
+	if (chip_num >= TW_NUM_CHIP)
+		return -EINVAL;
+
 	if (val > 255 || val < 0)
 		return -ERANGE;
 
@@ -758,6 +761,9 @@ int tw28_get_ctrl_val(struct solo_dev *solo_dev, u32 ctrl, u8 ch,
 	chip_num = ch / 4;
 	ch %= 4;
 
+	if (chip_num >= TW_NUM_CHIP)
+		return -EINVAL;
+
 	switch (ctrl) {
 	case V4L2_CID_SHARPNESS:
 		/* Only 286x has sharpness */
-- 
2.51.0