uhid has to run hid_add_device() from workqueue context while allowing
parallel use of the userspace API (which is protected with ->devlock).
But hid_add_device() can fail. Currently, that is handled by immediately
destroying the associated HID device, without using ->devlock - but if
there are concurrent requests from userspace, that's wrong and leads to
NULL dereferences and/or memory corruption (via use-after-free).
Fix it by leaving the HID device as-is in the worker. We can clean it up
later, either in the UHID_DESTROY command handler or in the ->release()
handler.
Cc: stable(a)vger.kernel.org
Fixes: 67f8ecc550b5 ("HID: uhid: fix timeout when probe races with IO")
Signed-off-by: Jann Horn <jannh(a)google.com>
---
Notes:
This crasher triggers an ASAN UAF warning:
int main(void) {
int dev = open("/dev/uhid", O_RDWR);
if (dev == -1) err(1, "open");
while (1) {
struct uhid_event ev_create = {
.type = UHID_CREATE2,
/* choose vendor+product IDs that will be rejected by hid_add_device() */
.u.create2 = { .rd_size = 1, .vendor = 0x07c0, .product = 0x1500 }
};
if (write(dev, &ev_create, sizeof(ev_create)) <= 0)
err(1, "write CREATE");
while (1) {
struct uhid_event ev_input = {
.type = UHID_INPUT2,
.u.input2 = {
.data = { 0xff },
.size = 1
}
};
int res = write(dev, &ev_input, sizeof(ev_input));
if (res == -1 && errno == EINVAL)
break;
}
}
}
It results in a splat like this:
BUG: KASAN: use-after-free in __lock_acquire+0x3eb9/0x5550
Read of size 8 at addr ffff88800c2218f8 by task uhid_uaf/588
CPU: 1 PID: 588 Comm: uhid_uaf Not tainted 5.16.0-08301-gfb3b0673b7d5 #886
[...]
Call Trace:
[...]
lock_acquire+0x1b9/0x4e0
_raw_spin_lock_irqsave+0x3e/0x60
down_trylock+0x13/0x70
hid_input_report+0x3d/0x500
uhid_char_write+0x210/0xdb0
vfs_write+0x1c7/0x920
ksys_write+0x176/0x1d0
do_syscall_64+0x43/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
[...]
drivers/hid/uhid.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8fe3efcb8327..fc06d8bb42e0 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -28,11 +28,22 @@
struct uhid_device {
struct mutex devlock;
+
+ /* This flag tracks whether the HID device is usable for commands from
+ * userspace. The flag is already set before hid_add_device(), which
+ * runs in workqueue context, to allow hid_add_device() to communicate
+ * with userspace.
+ * However, if hid_add_device() fails, the flag is cleared without
+ * holding devlock.
+ * We guarantee that if @running changes from true to false while you're
+ * holding @devlock, it's still fine to access @hid.
+ */
bool running;
__u8 *rd_data;
uint rd_size;
+ /* When this is NULL, userspace may use UHID_CREATE/UHID_CREATE2. */
struct hid_device *hid;
struct uhid_event input_buf;
@@ -63,9 +74,18 @@ static void uhid_device_add_worker(struct work_struct *work)
if (ret) {
hid_err(uhid->hid, "Cannot register HID device: error %d\n", ret);
- hid_destroy_device(uhid->hid);
- uhid->hid = NULL;
+ /* We used to call hid_destroy_device() here, but that's really
+ * messy to get right because we have to coordinate with
+ * concurrent writes from userspace that might be in the middle
+ * of using uhid->hid.
+ * Just leave uhid->hid as-is for now, and clean it up when
+ * userspace tries to close or reinitialize the uhid instance.
+ *
+ * However, we do have to clear the ->running flag and do a
+ * wakeup to make sure userspace knows that the device is gone.
+ */
uhid->running = false;
+ wake_up_interruptible(&uhid->report_wait);
}
}
@@ -474,7 +494,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
void *rd_data;
int ret;
- if (uhid->running)
+ if (uhid->hid)
return -EALREADY;
rd_size = ev->u.create2.rd_size;
@@ -556,7 +576,7 @@ static int uhid_dev_create(struct uhid_device *uhid,
static int uhid_dev_destroy(struct uhid_device *uhid)
{
- if (!uhid->running)
+ if (!uhid->hid)
return -EINVAL;
uhid->running = false;
@@ -565,6 +585,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
cancel_work_sync(&uhid->worker);
hid_destroy_device(uhid->hid);
+ uhid->hid = NULL;
kfree(uhid->rd_data);
return 0;
base-commit: fb3b0673b7d5b477ed104949450cd511337ba3c6
--
2.34.1.703.g22d0c6ccf7-goog
May the peace of God be with you ,
This letter might be a surprise to you, But I believe that you will
be honest to fulfill my final wish. I bring peace and love to you. It
is by the grace of god, I had no choice than to do what is lawful and
right in the sight of God for eternal life and in the sight of man for
witness of god’s mercy and glory upon my life. My dear, I sent this
mail praying it will find you in a good condition, since I myself am
in a very critical health condition in which I sleep every night
without knowing if I may be alive to see the next day. I am Mrs.Sophia
Erick, a widow suffering from a long time illness. I have some funds I
inherited from my late husband, the sum of ($11,000,000.00, Eleven
Million Dollars) my Doctor told me recently that I have serious
sickness which is a cancer problem. What disturbs me most is my stroke
sickness. Having known my condition, I decided to donate this fund to
a good person that will utilize it the way I am going to instruct
herein. I need a very honest and God fearing person who can claim this
money and use it for Charity works, for orphanages and gives justice
and help to the poor, needy and widows says The Lord." Jeremiah
22:15-16.“ and also build schools for less privilege that will be
named after my late husband if possible and to promote the word of god
and the effort that the house of god is maintained.
I do not want a situation where this money will be used in an ungodly
manner. That's why I'm taking this decision. I'm not afraid of death,
so I know where I'm going. I accept this decision because I do not
have any child who will inherit this money after I die. Please I want
your sincere and urgent answer to know if you will be able to execute
this project, and I will give you more information on how the fund
will be transferred to your bank account. May the grace, peace, love
and the truth in the Word of god be with you and all those that you
love and care for.
I am waiting for your reply.
May God Bless you,
Mrs. Sophia Erick.
On some servers with MGA G200_SE_A (rev 42), booting with Legacy BIOS,
the hardware hangs when using kdump and kexec into the kdump kernel.
This happens when the uncompress code tries to write "Decompressing Linux"
to the VGA Console.
It can be reproduced by writing to the VGA console (0xB8000) after
booting to graphic mode, it generates the following error:
kernel:NMI: PCI system error (SERR) for reason a0 on CPU 0.
kernel:Dazed and confused, but trying to continue
The root cause is the configuration of the MGA GCTL6 register
According to the GCTL6 register documentation:
bit 0 is gcgrmode:
0: Enables alpha mode, and the character generator addressing system is
activated.
1: Enables graphics mode, and the character addressing system is not
used.
bit 1 is chainodd even:
0: The A0 signal of the memory address bus is used during system memory
addressing.
1: Allows A0 to be replaced by either the A16 signal of the system
address (ifmemmapsl is ‘00’), or by the hpgoddev (MISC<5>, odd/even
page select) field, described on page 3-294).
bit 3-2 are memmapsl:
Memory map select bits 1 and 0. VGA.
These bits select where the video memory is mapped, as shown below:
00 => A0000h - BFFFFh
01 => A0000h - AFFFFh
10 => B0000h - B7FFFh
11 => B8000h - BFFFFh
bit 7-4 are reserved.
Current code set it to 0x05 => memmapsl to b01 => 0xa0000 (graphic mode)
But on x86, the VGA console is at 0xb8000 (text mode)
In arch/x86/boot/compressed/misc.c debug strings are written to 0xb8000
As the driver doesn't use this mapping at 0xa0000, it is safe to set it to
0xb8000 instead, to avoid kernel hang on G200_SE_A rev42, with kexec/kdump.
Thus changing the value 0x05 to 0x0d
Signed-off-by: Jocelyn Falempe <jfalempe(a)redhat.com>
Reviewed-by: Javier Martinez Canillas <javierm(a)redhat.com>
Acked-by: Lyude Paul <lyude(a)redhat.com>
Cc: stable(a)vger.kernel.org
---
v2: Add clear statement that it's not the right configuration, but it
prevents an annoying bug with kexec/kdump.
drivers/gpu/drm/mgag200/mgag200_mode.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index b983541a4c53..cd9ba13ad5fc 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -529,7 +529,10 @@ static void mgag200_set_format_regs(struct mga_device *mdev,
WREG_GFX(3, 0x00);
WREG_GFX(4, 0x00);
WREG_GFX(5, 0x40);
- WREG_GFX(6, 0x05);
+ /* GCTL6 should be 0x05, but we configure memmapsl to 0xb8000 (text mode),
+ * so that it doesn't hang when running kexec/kdump on G200_SE rev42.
+ */
+ WREG_GFX(6, 0x0d);
WREG_GFX(7, 0x0f);
WREG_GFX(8, 0x0f);
--
2.34.1
hallo Greg
5.16.2-rc1
compiles, boots, suspends on my x86_64
(Intel i5-11400, Fedora 35)
Thanks
Tested-by: Ronald Warsow <rwarsow(a)gmx.de>
regards
Ronald