[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

[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