Re: [PATCH] Input: joydev - avoid signed overflow in the broken-axis correction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Input: joydev - 32-bit overflow in joydev_correct() subtraction
- [Medium] Input: joydev - s64 truncation before clamp() in joydev_correct()
--

commit 1426ece8bea8b467a9629c0559c54b6693b90b43
Author: Guanglei Zhu <zhugl3@xiaopeng.com>

Input: joydev - avoid signed overflow in the broken-axis correction

This commit casts the coefficient operands in the JS_CORR_BROKEN calculation
in joydev_correct() to s64 to avoid 32-bit signed integer overflow during
multiplication when user-controlled coefficients are set via JSIOCSCORR.

> diff --git a/drivers/input/joydev.c b/drivers/input/joydev.c
> index 32459fd8a7c18..947d7671a59f6 100644
> --- a/drivers/input/joydev.c
> +++ b/drivers/input/joydev.c
> @@ -74,8 +74,8 @@ static int joydev_correct(int value, struct js_corr *corr)
>  
>  	case JS_CORR_BROKEN:
>  		value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
> -			((corr->coef[3] * (value - corr->coef[1])) >> 14)) :
> -			((corr->coef[2] * (value - corr->coef[0])) >> 14);
> +			(((s64)corr->coef[3] * (value - corr->coef[1])) >> 14)) :
> +			(((s64)corr->coef[2] * (value - corr->coef[0])) >> 14);

[Severity: Medium]
Can the parenthesized subtractions (value - corr->coef[1]) and
(value - corr->coef[0]) in joydev_correct() in drivers/input/joydev.c still
overflow 32-bit signed arithmetic before being promoted to s64 for the
multiplication?

When joydev_ioctl_common() processes JSIOCSCORR or joydev_event() handles an
EV_ABS event and calls joydev_correct(), both value (int) and corr->coef[i]
(__s32) are 32-bit signed integers. In the first branch, where
value > corr->coef[0] and value >= corr->coef[1], if value >= 0 and
corr->coef[0] and corr->coef[1] are large negative values such as INT_MIN,
evaluating value - corr->coef[1] overflows a 32-bit signed int before
promoting to s64. Likewise, in the second branch where value <= corr->coef[0],
if value < 0 and corr->coef[0] is a large positive value such as INT_MAX,
value - corr->coef[0] underflows a 32-bit signed int.

Would casting value to s64 inside the subtraction, such as
(s64)value - corr->coef[1] and (s64)value - corr->coef[0], avoid this
overflow as well?

[Severity: Medium]
What happens in joydev_correct() in drivers/input/joydev.c when the shifted
64-bit product exceeds the 32-bit signed int range before reaching clamp()?

Because two 32-bit signed values multiplied in s64 can have a magnitude up to
roughly 2^62, the result after >> 14 can still have a magnitude up to roughly
2^48. Assigning that s64 expression back into the 32-bit int variable value:

drivers/input/joydev.c:joydev_correct() {
	...
	case JS_CORR_BROKEN:
		value = value > corr->coef[0] ? (value < corr->coef[1] ? 0 :
			(((s64)corr->coef[3] * (value - corr->coef[1])) >> 14)) :
			(((s64)corr->coef[2] * (value - corr->coef[0])) >> 14);
		break;

	default:
		return 0;
	}

	return clamp(value, -32767, 32767);
}

truncates the upper bits before clamp(value, -32767, 32767) is evaluated,
causing large positive or negative results to wrap around rather than saturate
to 32767 or -32767. Should value or the intermediate result be kept as s64
until clamp_val() or clamp_t(s64, ...) is applied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103084135.426629-2-zhugl3@xiaopeng.com?part=1