[PATCH AUTOSEL 6.17-5.4] exec: Fix incorrect type for ret

[Sasha Levin <sashal@kernel.org>]

From: Xichao Zhao <zhao.xichao@vivo.com>

[ Upstream commit 5e088248375d171b80c643051e77ade6b97bc386 ]

In the setup_arg_pages(), ret is declared as an unsigned long.
The ret might take a negative value. Therefore, its type should
be changed to int.

Signed-off-by: Xichao Zhao <zhao.xichao@vivo.com>
Reviewed-by: Jan Kara <jack@suse.cz>
Link: https://lore.kernel.org/r/20250825073609.219855-1-zhao.xichao@vivo.com
Signed-off-by: Kees Cook <kees@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---

LLM Generated explanations, may be completely bogus:

## Backport Analysis: exec: Fix incorrect type for ret

**Backport Status: YES**

### Executive Summary

This commit fixes a long-standing type correctness bug in
`setup_arg_pages()` where the `ret` variable was incorrectly declared as
`unsigned long` instead of `int`. While this bug has existed since 2007
(commit b6a2fea39318e4) and has not caused known user-facing issues, it
should be backported to stable trees as a low-risk code correctness
improvement.

### Detailed Analysis

#### 1. Nature of the Bug

In fs/exec.c:602, the `ret` variable in `setup_arg_pages()` was declared
as:
```c
unsigned long ret;
```

However, `ret` is used to store return values from functions that return
`int` with standard Linux error codes:
- `mprotect_fixup()` - returns `int` (0 on success, negative error codes
  like -EPERM, -ENOMEM)
- `relocate_vma_down()` - returns `int`
- `expand_stack_locked()` - returns `int`

The function `setup_arg_pages()` itself returns `int`, and all error
paths return negative error codes through `ret`.

#### 2. Why This Bug Hasn't Caused Issues

Despite being present for 18 years, this bug hasn't caused observable
problems due to C's type conversion rules:

1. When a negative `int` (e.g., -ENOMEM = -12) is assigned to `unsigned
   long`, it gets sign-extended to a large positive value
   (0xFFFFFFFFFFFFFFF4 on 64-bit)
2. When this `unsigned long` is returned as `int`, the truncation
   preserves the bit pattern, resulting in the correct negative value
3. All error checks in the function (`if (ret)`) work correctly because
   non-zero is still non-zero regardless of signedness

#### 3. Why It Should Still Be Fixed

Despite working "by accident," this is a genuine bug that should be
fixed:

1. **Code correctness**: Error codes should always be stored in signed
   types - this is a fundamental Linux kernel convention
2. **Type safety violation**: Storing signed error codes in unsigned
   variables violates type safety principles
3. **Compiler warnings**: Modern compilers with stricter type checking
   may warn about sign mismatches
4. **Future-proofing**: If code is added that relies on `ret` being
   signed (e.g., `if (ret < 0)`), it would break with `unsigned long`
5. **Code clarity**: It's confusing and error-prone to store error codes
   in unsigned variables
6. **Static analysis**: Tools like Coverity or Clang static analyzer may
   flag this as a potential issue

#### 4. Risk Assessment

The fix carries **minimal to zero risk**:

- **Change scope**: Single line change (fs/exec.c:602)
- **Change complexity**: Trivial type change from `unsigned long` to
  `int`
- **Testing**: The code paths are well-exercised by every execve()
  system call
- **Review quality**: Reviewed by Jan Kara <jack@suse.cz>, a respected
  kernel filesystem developer
- **Regression potential**: Extremely low - the conversion from
  `unsigned long` to `int` is safe for all existing error code paths

#### 5. Historical Context

- **Bug introduced**: 2007-07-19 by commit b6a2fea39318e4 (Ollie Wild)
- **Bug duration**: Present in kernel for ~18 years
- **Fix author**: Xichao Zhao <zhao.xichao@vivo.com>
- **Fix date**: 2025-08-25
- **Maintainer**: Kees Cook (security/hardening maintainer)

The original bug was introduced as part of a large refactoring that
added variable-length argument support. The incorrect type likely
slipped through because other variables in the function (stack_shift,
stack_base, etc.) are `unsigned long`, and the type mismatch didn't
cause immediate problems.

#### 6. Backport Justification

This commit meets the stable kernel criteria:

✅ **Fixes a bug**: Yes - type correctness bug
✅ **Small and contained**: Yes - single line change
✅ **No architectural changes**: Yes - simple type fix
✅ **Low regression risk**: Yes - extremely low risk
✅ **Improves code quality**: Yes - follows kernel conventions
✅ **Clear benefit**: Yes - prevents potential future issues

While the commit doesn't have a `Fixes:` or `Cc: stable` tag (suggesting
the author saw it as low priority), it's an ideal stable tree candidate
because:
1. It's completely safe
2. It improves correctness
3. It may prevent issues with stricter compiler checks in the future
4. It aligns with kernel coding standards

### Recommendation

**YES - Backport to all active stable trees**

Priority: Low (code quality fix, not a critical bug)

This is a "good housekeeping" fix that improves code correctness without
any meaningful risk. While not urgent, it should be included in stable
trees to maintain code quality and consistency across kernel versions.

 fs/exec.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/exec.c b/fs/exec.c
index e861a4b7ffda9..4a89918b761f6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -599,7 +599,7 @@ int setup_arg_pages(struct linux_binprm *bprm,
 		    unsigned long stack_top,
 		    int executable_stack)
 {
-	unsigned long ret;
+	int ret;
 	unsigned long stack_shift;
 	struct mm_struct *mm = current->mm;
 	struct vm_area_struct *vma = bprm->vma;
-- 
2.51.0