Skip to contentSkip to conversation

Input: fix two kernel stack disclosures via ioctl

PATCH In review Iván Ezequiel Rodriguez 2 participants 4 messages Sep 1

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover Input: fix two kernel stack disclosures via ioctl

Two places in the input layer copy a user supplied, possibly short buffer into an uninitialized on-stack structure and then keep the whole structure, including the part that was never written. In both cases the stale stack bytes can be read back from userspace.

Patch 1: EVIOCSABS copies min(_IOC_SIZE(cmd), sizeof(struct input_absinfo)) bytes into an uninitialized struct and stores the result in dev->absinfo[]. EVIOCGABS returns it. Only the resolution field is currently cleared for short sizes.

Patch 2: the compat path of input_ff_effect_from_user() aliases the native struct ff_effect with the smaller struct ff_effect_compat and copies only the compat sized prefix, leaving the tail untouched. input_ff_upload() stores the full structure, which a uinput based force feedback daemon can read back via UI_BEGIN_FF_UPLOAD.

Both are fixed by zeroing the structure before the copy. The patches are independent of each other.

Compile tested on x86_64 with CONFIG_INPUT_EVDEV, CONFIG_INPUT_UINPUT and CONFIG_IA32_EMULATION enabled.

Iván Ezequiel Rodriguez (2):
Input: evdev: zero absinfo before partial copy in EVIOCSABS
Input: zero ff_effect before compat copy in input_ff_effect_from_user

 drivers/input/evdev.c        | 2 ++
 drivers/input/input-compat.c | 2 ++
 2 files changed, 4 insertions(+)
Signature · 1 line
2.43.0
#1 lore raw

PATCH 1/2 Input: evdev: zero absinfo before partial copy in EVIOCSABS

The EVIOCSABS handler copies at most the user supplied ioctl size into an uninitialized on-stack struct input_absinfo:

	if (copy_from_user(&abs, p, min_t(size_t,
			size, sizeof(struct input_absinfo))))

The size comes from _IOC_SIZE() of the ioctl command and is therefore fully controlled by userspace. A short size leaves the trailing part of the structure holding whatever was on the kernel stack, and the whole structure is then stored into the device:

	dev->absinfo[t] = abs;

EVIOCGABS hands that back to userspace, disclosing the stale stack bytes. Only the resolution field is currently cleared, which covers the legacy struct layout but not an arbitrarily short size.

Zero the structure before the copy so any part not supplied by the caller reads back as zero. The existing resolution fixup is kept, since it also handles a size that partially overlaps that field.

Fixes 448cd1664a57 ("Input: evdev - rearrange ioctl handling")
---
 drivers/input/evdev.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/input/evdev.c +2 -0
index 3a718d600006..8bfaaa45e0b9 100644
@@ -1229,6 +1229,8 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 
 			t = _IOC_NR(cmd) & ABS_MAX;
 
+			memset(&abs, 0, sizeof(abs));
+
 			if (copy_from_user(&abs, p, min_t(size_t,
 					size, sizeof(struct input_absinfo))))
 				return -EFAULT;
Signature · 1 line
2.43.0
#2 lore raw

PATCH 2/2 Input: zero ff_effect before compat copy in input_ff_effect_from_user

In reply to Iván Ezequiel Rodriguez

In the compat path input_ff_effect_from_user() aliases the caller's native struct ff_effect with the smaller struct ff_effect_compat and copies only the compat sized prefix:

	compat_effect = (struct ff_effect_compat *)effect;

	if (copy_from_user(compat_effect, buffer,
			   sizeof(struct ff_effect_compat)))

The tail of the native structure is never written. Callers pass an uninitialized on-stack object, for example evdev_do_ioctl() for EVIOCSFF, so those bytes keep their previous stack contents. input_ff_upload() then stores the full native structure in ff->effects[id], from where a uinput based force feedback daemon can read it back via UI_BEGIN_FF_UPLOAD, disclosing kernel stack memory to userspace.

Zero the effect before the compat copy.

Fixes 2d56f3a32c0e ("Input: refactor evdev 32bit compat to be shareable with uinput")
---
 drivers/input/input-compat.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/input/input-compat.c +2 -0
index a5043193ead8..8860a0730294 100644
@@ -76,6 +76,8 @@ int input_ff_effect_from_user(const char __user *buffer, size_t size,
 		 */
 		compat_effect = (struct ff_effect_compat *)effect;
 
+		memset(effect, 0, sizeof(*effect));
+
 		if (copy_from_user(compat_effect, buffer,
 				   sizeof(struct ff_effect_compat)))
 			return -EFAULT;
Signature · 1 line
2.43.0
maintainer
#3 lore raw
In reply to Iván Ezequiel Rodriguez
Quoted 16 lines · Iván Ezequiel Rodriguez

Two places in the input layer copy a user supplied, possibly short buffer into an uninitialized on-stack structure and then keep the whole structure, including the part that was never written. In both cases the stale stack bytes can be read back from userspace.

Patch 1: EVIOCSABS copies min(_IOC_SIZE(cmd), sizeof(struct input_absinfo)) bytes into an uninitialized struct and stores the result in dev->absinfo[]. EVIOCGABS returns it. Only the resolution field is currently cleared for short sizes.

Patch 2: the compat path of input_ff_effect_from_user() aliases the native struct ff_effect with the smaller struct ff_effect_compat and copies only the compat sized prefix, leaving the tail untouched. input_ff_upload() stores the full structure, which a uinput based force feedback daemon can read back via UI_BEGIN_FF_UPLOAD.

Quoted 5 lines

Both are fixed by zeroing the structure before the copy. The patches are independent of each other.

Compile tested on x86_64 with CONFIG_INPUT_EVDEV, CONFIG_INPUT_UINPUT and CONFIG_IA32_EMULATION enabled.

Applied the lot, thank you.

Signature · 1 line
Dmitry