Re: [PATCH] Input: evdev - sanitize event type index when fetching event masks

Thank you for your contribution! Sashiko AI review found 1 potential issue(=
s) to consider:

Pre-existing issues:
- [High] Incomplete Spectre v1 mitigation due to misunderstanding of specul=
ative execution on branches.
--

--- Patch [1]: [PATCH] Input: evdev - sanitize event type index when fetchi=
ng event masks ---
commit e9eb78cd8dbba1d32ef31f1eec9a5c00e54b5aad
Author: Dmitry Torokhov <[email protected]>

Input: evdev - sanitize event type index when fetching event masks

This commit uses array_index_mask_nospec() in evdev_get_mask_cnt() to clamp=
=20
out-of-bounds event types to 0. It asserts that this is sufficient to preve=
nt=20
speculative out-of-bounds memory accesses in functions like evdev_get_mask(=
)=20
because the returned count is speculatively forced to 0.

> We do not need additional array_index_nospec() calls in evdev_get_mask()
> because evdev_get_mask_cnt() speculatively forces the count (and
> resulting xfer_size) to 0 for out-of-bounds types, preventing any
> speculative memory access to client evmasks array.

[Severity: High]
This is a pre-existing issue, but does relying on the returned count being =
0=20
fully prevent speculative out-of-bounds accesses in callers like=20
evdev_get_mask() and evdev_set_mask()?

When CPU branch predictors mispredict the conditions checking the count, th=
ey
do not wait for the condition to evaluate.=20

For example, if the if (cnt > 0) branch in evdev_get_mask() is mispredicted:

drivers/input/evdev.c:evdev_get_mask() {
    ...
    cnt =3D evdev_get_mask_cnt(type);
    ...
    if (cnt > 0) {
        mask =3D client->evmasks[type];
    ...
}

The branch predictor will speculatively execute the block using the=20
original, unclamped type variable, performing an out-of-bounds read on=20
client->evmasks[type]. The retrieved pointer is then accessed by=20
bits_to_user().

A similar pattern exists in evdev_set_mask(), where a mispredicted if (!cnt=
)=20
branch can lead to using the unclamped type:

drivers/input/evdev.c:evdev_set_mask() {
    ...
    cnt =3D evdev_get_mask_cnt(type);
    if (!cnt)
        return 0;
    ...
    oldmask =3D client->evmasks[type];
    ...
    bitmap_free(oldmask);
}

The speculative read of oldmask is followed by a dereference in bitmap_free=
(),
which creates an exploitable side-channel gadget.

This pattern also appears in __evdev_is_filtered(). A mispredicted bounds=20
check can lead to speculative out-of-bounds accesses for test_bit(type, mas=
k)=20
or mask =3D client->evmasks[type].

Do we still need to explicitly use array_index_nospec(type, EV_CNT) inside
these caller functions to prevent the unclamped type from being used during=
=20
speculative execution?

> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index c7325226cb866..32804d6965340 100644
> --- a/drivers/input/evdev.c
> +++ b/drivers/input/evdev.c

[ ... ]

> @@ -67,8 +68,10 @@ static size_t evdev_get_mask_cnt(unsigned int type)
>  		[EV_SND]	=3D SND_CNT,
>  		[EV_FF]		=3D FF_CNT,
>  	};
> +	unsigned long mask =3D array_index_mask_nospec(type, EV_CNT);
> =20
> -	return (type < EV_CNT) ? counts[type] : 0;
> +	/* Returns 0 for out-of-bounds types, including speculatively */
> +	return counts[type & mask] & mask;
>  }
> =20
>  /* requires the buffer lock to be held */

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/anFCAfvxwXB5eJF1@go=
ogle.com?part=3D1