Re: [PATCH] Input: xen-kbdfront - bound the backend-supplied keycode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: xen-kbdfront - double-fetch of keycode in xenkbd_handle_key_event()
--

commit b7a9474ab37fe06db55378df5e31d80df12a42ed
Author: Guo Zihao <guozh23@xiaopeng.com>

Input: xen-kbdfront - bound the backend-supplied keycode

This commit adds a bounds check against KEY_MAX in xenkbd_handle_key_event()
to prevent out-of-bounds bitmap reads in test_bit() when a backend supplies
an invalid keycode via the shared ring page.

> diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
> index 6471d836f6fe6..e9092a7889261 100644
> --- a/drivers/input/misc/xen-kbdfront.c
> +++ b/drivers/input/misc/xen-kbdfront.c
> @@ -92,6 +92,11 @@ static void xenkbd_handle_key_event(struct xenkbd_info *info,
>  	struct input_dev *dev;
>  	int value = key->pressed;
>  
> +	if (key->keycode >= KEY_MAX) {
> +		pr_warn("unhandled keycode 0x%x\n", key->keycode);
> +		return;
> +	}
> +
>  	if (test_bit(key->keycode, info->ptr->keybit)) {
>  		dev = info->ptr;
>  	} else if (test_bit(key->keycode, info->kbd->keybit)) {

[Severity: High]
Can a backend bypass this bounds check by modifying key->keycode in the
shared ring page between the KEY_MAX check and the subsequent test_bit() and
input_event() calls?

In xenkbd_connect_backend(), info->page is granted with read-write foreign
access to the backend domain:

	ret = gnttab_grant_foreign_access(dev->otherend_id,
	                                  virt_to_gfn(info->page), 0);

And input_handler() -> xenkbd_handle_event() -> xenkbd_handle_key_event()
passes a pointer directly into that shared ring page:

	for (cons = page->in_cons; cons != prod; cons++)
		xenkbd_handle_event(info, &XENKBD_IN_RING_REF(page, cons));

Because key->keycode is not read once into a local variable with READ_ONCE()
before validating it against KEY_MAX, the subsequent test_bit() calls on
info->ptr->keybit, info->kbd->keybit, and info->kbd->key (as well as
input_event()) can re-read key->keycode from shared memory after a
concurrent backend write and still read past the bitmaps.

Should key->keycode be snapshotted into a local variable via READ_ONCE()
before performing the check and bitmap lookups?

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