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

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

[Guo Zihao]

Keycodes arrive from the shared ring the backend writes:

	if (test_bit(key->keycode, info->ptr->keybit)) {
		dev = info->ptr;
	} else if (test_bit(key->keycode, info->kbd->keybit)) {
		...
	} else {
		pr_warn("unhandled keycode 0x%x\n", key->keycode);
		return;
	}

The keycode is a plain u32 from the ring page, while the keybit
bitmaps it is tested against are sized for the KEY_CNT keycode bits
declared to the input core. A backend that sends a keycode at or
above KEY_CNT makes test_bit() read past the bitmap to decide which
device gets the event, and no keycode at or above KEY_MAX is a valid
EV_KEY code in the first place.

Ignore keycodes the bitmaps cannot hold.

Fixes: 4ee36dc08e5c ("xen pvfb: Para-virtual framebuffer, keyboard and pointer driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---
 drivers/input/misc/xen-kbdfront.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index 6471d836f..e9092a788 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)) {
-- 
2.50.1

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

[sashiko-bot]

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

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

[Guo Zihao]

Keycodes arrive from the shared ring the backend writes:

	if (test_bit(key->keycode, info->ptr->keybit)) {
		dev = info->ptr;
	} else if (test_bit(key->keycode, info->kbd->keybit)) {
		...
	} else {
		pr_warn("unhandled keycode 0x%x\n", key->keycode);
		return;
	}

The keycode is a plain u32 from the ring page, while the keybit
bitmaps it is tested against are sized for the KEY_CNT keycode bits
declared to the input core. A backend that sends a keycode at or
above KEY_CNT makes test_bit() read past the bitmap to decide which
device gets the event, and no keycode at or above KEY_MAX is a valid
EV_KEY code in the first place.

Since the event sits in memory the backend keeps writing access to,
the keycode must also be fetched exactly once: re-reading key->keycode
after the bounds check would let a concurrent backend write feed an
unchecked value to test_bit() and input_event(). Snapshot the keycode
into a local variable with READ_ONCE() before validating it, and use
the snapshot (as well as the already snapshotted pressed value)
throughout.

Ignore keycodes the bitmaps cannot hold.

Fixes: 4ee36dc08e5c ("xen pvfb: Para-virtual framebuffer, keyboard and pointer driver")
Cc: stable@vger.kernel.org
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---

v2: Snapshot the keycode with READ_ONCE() so a concurrent backend
write to the shared ring page cannot feed an unchecked value past the
KEY_MAX check (double-fetch, sashiko review); also reuse the already
snapshotted pressed value instead of re-reading key->pressed.

 drivers/input/misc/xen-kbdfront.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/input/misc/xen-kbdfront.c b/drivers/input/misc/xen-kbdfront.c
index 6471d836f6fe..f95aefedf026 100644
--- a/drivers/input/misc/xen-kbdfront.c
+++ b/drivers/input/misc/xen-kbdfront.c
@@ -91,22 +91,28 @@ static void xenkbd_handle_key_event(struct xenkbd_info *info,
 {
 	struct input_dev *dev;
 	int value = key->pressed;
+	u32 keycode = READ_ONCE(key->keycode);
 
-	if (test_bit(key->keycode, info->ptr->keybit)) {
+	if (keycode >= KEY_MAX) {
+		pr_warn("unhandled keycode 0x%x\n", keycode);
+		return;
+	}
+
+	if (test_bit(keycode, info->ptr->keybit)) {
 		dev = info->ptr;
-	} else if (test_bit(key->keycode, info->kbd->keybit)) {
+	} else if (test_bit(keycode, info->kbd->keybit)) {
 		dev = info->kbd;
-		if (key->pressed && test_bit(key->keycode, info->kbd->key))
+		if (value && test_bit(keycode, info->kbd->key))
 			value = 2; /* Mark as autorepeat */
 	} else {
-		pr_warn("unhandled keycode 0x%x\n", key->keycode);
+		pr_warn("unhandled keycode 0x%x\n", keycode);
 		return;
 	}
 
 	if (unlikely(!dev))
 		return;
 
-	input_event(dev, EV_KEY, key->keycode, value);
+	input_event(dev, EV_KEY, keycode, value);
 	input_sync(dev);
 }
 
-- 
2.50.1

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

[Dmitry Torokhov]

On Fri, Nov 06, 2026 at 10:02:17AM +0800, Guo Zihao wrote:
> Keycodes arrive from the shared ring the backend writes:
> 
> 	if (test_bit(key->keycode, info->ptr->keybit)) {
> 		dev = info->ptr;
> 	} else if (test_bit(key->keycode, info->kbd->keybit)) {
> 		...
> 	} else {
> 		pr_warn("unhandled keycode 0x%x\n", key->keycode);
> 		return;
> 	}
> 
> The keycode is a plain u32 from the ring page, while the keybit
> bitmaps it is tested against are sized for the KEY_CNT keycode bits
> declared to the input core. A backend that sends a keycode at or
> above KEY_CNT makes test_bit() read past the bitmap to decide which
> device gets the event, and no keycode at or above KEY_MAX is a valid
> EV_KEY code in the first place.
> 
> Since the event sits in memory the backend keeps writing access to,
> the keycode must also be fetched exactly once: re-reading key->keycode
> after the bounds check would let a concurrent backend write feed an
> unchecked value to test_bit() and input_event(). Snapshot the keycode
> into a local variable with READ_ONCE() before validating it, and use
> the snapshot (as well as the already snapshotted pressed value)
> throughout.
> 
> Ignore keycodes the bitmaps cannot hold.
> 
> Fixes: 4ee36dc08e5c ("xen pvfb: Para-virtual framebuffer, keyboard and pointer driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>

Please explain your threat model.

Thanks.

-- 
Dmitry

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

[Guo Zihao]

Hi Dmitry,

The threat model is a compromised backend: the xenbus device model gives
the backend write access to the shared ring page for the entire lifetime
of the frontend, and the guest has no way to distinguish well-formed
input from forged input once the peer on the other end is malicious.

Two scenarios where that peer is not fully trusted:

- Multi-tenant hosts. The backend lives in dom0, so a kernel compromise
  of the host should not immediately translate into kernel memory
  corruption inside the guests. Frontends are the last line of defence
  there: every frontend that consumes ring data without validation hands
  the attacker a structured memory-corruption primitive in the guest,
  which is a meaningful step beyond "the host can already do anything".

- Driver domain disaggregation. Backends can also run in unprivileged
  driver domains, where compromising the backend is much cheaper than
  compromising dom0.

This is the same model under which mainline keeps accepting frontend
hardening, for example:

  9b4cf60c6b73 ("9p/xen: validate the response size from the backend")
  d33846c8dcc0 ("xen/pvcalls: bound backend response req_id before indexing rsp[]")
  992df081bc88 ("xen/scsifront: check for a NULL shadow entry on a backend response")
  808e530654a5 ("misc: nsm: bound the device-reported response length")
  e3046eeada29 ("hwrng: virtio: clamp device-reported used.len at copy_data()")

and the READ_ONCE() snapshot in v2 follows the same convention as
8135cf8b0927 ("xen/pciback: Save xen_pci_op commands before processing it").

The keycode here is the kbdfront instance of that pattern: a plain u32
read from the shared page and used as a bitmap index, with nothing
between the fetch and the use.

Thanks,
Guo Zihao