From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wm1-f47.google.com (mail-wm1-f47.google.com [209.85.128.47]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 15E653A544D for ; Tue, 4 Aug 2026 10:08:36 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.128.47 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785838118; cv=none; b=CxKytnC+srMWiVjElY0OxWddhNL9bPXdifN+cAl3Mupt40o2bWX38WkuSl0jjxr615P3L0JWkIcVX3mGgUgByntp58J0EHJrQd+mQAfhZbcM5YjgUerT2gWebhOFKzOnlLMwftsscvdlbGc+BV80bD2/0Byv5iOsvmulorMb6CU= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1785838118; c=relaxed/simple; bh=dJGND05g9U6TJBGvoJHo9FrRRr+W+VUL9or/3OTtXWs=; h=Message-ID:Subject:From:To:Cc:Date:In-Reply-To:References: Content-Type:MIME-Version; b=a3cfGh9eQ4VzJoj4EgRvkdc47ONBkKiyjznI8nmGMMow9qtyxQtnHQ/9qHxMMuZyKPpWozxtqgmnPWJbCTmmFXMMJ9Or1mn87Obohr5I1gSwxSI28YBocVT2c7Cjh6ic1+IWwfsPkGsRhadSvhmA8+/yuSBtjj2NWHOYElUS7+Y= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=m2wJYoUT; arc=none smtp.client-ip=209.85.128.47 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="m2wJYoUT" Received: by mail-wm1-f47.google.com with SMTP id 5b1f17b1804b1-4954a2dba6cso2690235e9.1 for ; Tue, 04 Aug 2026 03:08:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1785838115; x=1786442915; darn=vger.kernel.org; h=mime-version:user-agent:content-transfer-encoding:content-type :disposition-notification-to:references:in-reply-to:date:cc:to:from :subject:message-id:from:to:cc:subject:date:message-id:reply-to :content-type; bh=eKvQegePYqOrYt0JNkWU6KOxbIgN4O4nE+seVNDXYDc=; b=m2wJYoUTLBTjFciWRalmO3/9AOpnNDWr9ZIQjb1Tt1yDqQfCol72NN8xjPcMy9toa0 gG9HwPc2J5he8gTLWQp57lVojLUqfDAfh4vxEzNwzS7lEXT8IUdtIoC7d0k9NzwOq1el /I/el4+VK1ahBdfwD8Vmo5CHEENK5Lw5oJdX18IvwswsX+lxCE2S2cwnn301DM2+/JG7 ANHSAwH4/nPGzfn+gBrxlMTFkj86jar+R0HqHkM4L2pjRX6ftnNum0Xm9G1DEY8jaOEk /YtC2TvjQirfEVaGQLLkIv6+eKXDPJvB3AH3Ott7Hiw79T0LRHvWG6pee8kAzEd5uG7w GJfQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1785838115; x=1786442915; h=mime-version:user-agent:content-transfer-encoding:content-type :disposition-notification-to:references:in-reply-to:date:cc:to:from :subject:message-id:x-gm-gg:x-gm-message-state:from:to:cc:subject :date:message-id:reply-to:content-type; bh=eKvQegePYqOrYt0JNkWU6KOxbIgN4O4nE+seVNDXYDc=; b=PRUEno9JQnE5lRbujVLYms+Ba8WhortdfAzcFSbOCnrvCCLOlLrzIkFRwTmYl16J/k xE7XIr5W2wQ6rp2idCYLg0bnCwz5RnrniiHoQYgB5uww4Gb0UbXezF2kOdtJo/IvK5zm IvSuMDHSC9uytI+XswcRX/4R7pBrqXiEeSxTzWs6CorFbm364/8kMTu2/wiKucn1vIeQ UPHus1blYde8le2ia1kaSiOPX5Ubew4b5ShkSTRmQUXavutgESmU1XlhVz5SncHq6cXP EPXTX6bD6JjbVamb0WNsPPA5bdy39fEV2MyKj/36jPTIH3AU4BrUmRhVHq7QqlB2SUeg wPVA== X-Forwarded-Encrypted: i=1; AHgh+RrZ3L/g5BincrISJ1sp5dlSpcAh/+UYERTtkIbe75C/lJdCca0I+jqQUNX6Pdjb4daslzL9txQnkX+TI98=@vger.kernel.org X-Gm-Message-State: AOJu0Yx7JAu7bsqFtSk3ulwnQjtx3VZ6N2K6+h6/Fx6jl++tXT5uOkFs ZzF69ddcJxv50hr8dyHZ1drNPdwZ8t6w7cBKP0hE+DWqYdrBvGUeCcVU X-Gm-Gg: AR+sD113YaxRvJDbwfMh3sPhoZ+KsZt5B6P8qqqaPOni7wgJ+DQzWe6U37JWz/aemyw F9JtMr6jduwjsXk9VKZDELcKzsn0LL5gUuMzEobL3zD79m824G20hY9PPK4wa46eQI6Ddz9snfg /46cTFp/tJCAAIsp5bcRQt0KB6pXhNhoBeMNCEI30KoU+Lh3ctt0rwrPmtHOBy+hAJx2JjSaGal 7pJijJr+schGKU76l9CIgdxtS05wJV/TuVdY8Io6V0gIERlKqYLZdLtP5Ng7t/woDu4QYgkl024 Sz5athq/eNO/K8IJANroJuARVx4sbvy7T+rMiOKfxGkBFNDjUqoRsfSo/x6/mMZra7TyzJA95Lq aUNyimgZ6OJrR5lRwMqL05//A2Kn76aGnIy+8SKjqX06x+mj53H6u5MqyL+o2fMK2KfQnB7hotd 3DO0Gl++6gXlZPoAvs+/Ts/0Y6jj+Ke2MNf3s2IgA3/BkeJh0AQvaMiDHDco1zUu8U1a7e3tdux cxmwgU91iL0AWHHwAnV0xhifw7rgOQu9ecScjFLRrMvFuCI X-Received: by 2002:a05:600c:a01:b0:493:bea7:6b67 with SMTP id 5b1f17b1804b1-4980c6a1082mr153581505e9.3.1785838115061; Tue, 04 Aug 2026 03:08:35 -0700 (PDT) Received: from [192.168.1.239] (87-205-5-123.static.ip.netia.com.pl. [87.205.5.123]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-47fd42d91b3sm44990590f8f.14.2026.08.04.03.08.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 04 Aug 2026 03:08:34 -0700 (PDT) Message-ID: <12023556fec63125e8daaf0c732926568ce742ec.camel@gmail.com> Subject: Re: [PATCH] HID: universal-pidff: stop the device when force-feedback init fails From: Tomasz =?UTF-8?Q?Paku=C5=82a?= To: Baul Lee Cc: linux-input@vger.kernel.org, linux-kernel@vger.kernel.org, Oleg Makarenko , Jiri Kosina , Benjamin Tissoires Date: Tue, 04 Aug 2026 12:08:31 +0200 In-Reply-To: References: <20260729142432.16184-1-baul.lee@xbow.com> <9ce239858b358c60a243e34f2d61def2f986966a.camel@gmail.com> Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.60.2 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 On Tue, 2026-08-04 at 14:38 +0900, Baul Lee wrote: > Thanks for looking at it. I agree with the rule, but I don't think those= two > exits can take the stop, and I would rather explain why than send a v2 th= at > changes them. >=20 > After hid_hw_start() there is exactly one error return in > universal_pidff_probe(), the init_function() one this patch changes. The= two > exits on lines 92 and 101 return 0, which means the probe succeeds, and t= hat > is the whole difference between them. Because they succeed, hdev->driver > stays set, and since universal_pidff has no .remove callback, > hid_device_remove() falls back to the default remove, which is hid_hw_sto= p(). > The device those two exits leave running is therefore already stopped for= us > when it is unbound or unplugged, and nothing leaks there. The leak exist= s > only on the path where the probe fails, because __hid_device_probe() clea= rs > hdev->driver on failure and hid_device_remove() then skips the unwind > entirely. >=20 > If we call hid_hw_stop() on those two exits as well, it ends up running t= wice > on the same device, and usbhid_stop() does not survive that. It clears t= he > three URB pointers on the way out - that is what the "don't mess up next > start" comment is about - but the buffers it releases immediately afterwa= rds > through hid_free_buffers() are left dangling: inbuf, outbuf and ctrlbuf g= o > back through usb_free_coherent() and usbhid->cr through kfree(), and none= of > those four pointers is cleared. There is no HID_STARTED check at the top= of > usbhid_stop() either, so the second call frees all four again. Every unp= lug > of a device that took one of those exits would be a double free, which is= a > good deal worse than the leak this patch is fixing. >=20 > The other problem is that stopping there takes down the very device those > exits exist for. hid_hw_stop() calls hid_disconnect(), which unregisters= the > input device through hidinput_disconnect() and the character device throu= gh > hidraw_disconnect(). The additional "device" you mention, the one that i= s > there just for the extra buttons and axes, is by definition the one witho= ut a > PID usage page in its descriptor, so it is exactly the one that takes the= exit > on line 92. Stopping it would leave this driver bound to a device with n= o > input node and no hidraw node at all, and returning an error there instea= d of > 0 does not help either, because hid-generic will not adopt the interface: > hid_generic_match() walks the bus and gives up as soon as it finds anothe= r > driver that matches the device. Either way the buttons and axes disappea= r, > which I do not think is what you are after. >=20 > 2026=EB=85=84 8=EC=9B=94 3=EC=9D=BC (=EC=9B=94) =EC=98=A4=EC=A0=84 5:41, = Tomasz Paku=C5=82a =EB=8B=98=EC=9D=B4 = =EC=9E=91=EC=84=B1: > >=20 > > On Wed, 2026-07-29 at 23:24 +0900, Baul Lee wrote: > > > universal_pidff_probe() starts the device with hid_hw_start() and the= n, if > > > force-feedback initialisation fails, returns the error through a labe= l that > > > only does "return error". The device is left started. > > >=20 > > > The HID core does not unwind on the driver's behalf. __hid_device_pr= obe() > > > releases the devres group, closes the report and clears hdev->driver: > > >=20 > > > if (ret) { > > > devres_release_group(&hdev->dev, hdev->devres_group_id)= ; > > > hid_close_report(hdev); > > > hdev->driver =3D NULL; > > > } > > >=20 > > > The hidraw character device that hid_hw_start() registered through > > > hid_connect() is allocated with kzalloc() and added with cdev_device_= add(), > > > so it is not devres-managed and survives that. With hdev->driver NUL= L, > > > hid_device_remove() skips hid_hw_stop() as well, because it only unwi= nds > > > while a driver is still attached. The registration therefore outlive= s the > > > device on both paths. > > >=20 > > > Opening the surviving /dev/hidrawX writes into freed memory. KASAN r= eports > > > a use-after-free write from hidraw_open() -> hid_hw_open() -> the > > > transport's open callback, which takes a spinlock inside the freed ob= ject. > > > A descriptor that carries a PID usage page and no input reports is en= ough: > > > hidraw claims the device so hid_hw_start() succeeds, while hid->input= s > > > stays empty so force-feedback init fails. The other failure returns = in > > > hid_pidff_init_with_quirks() - no output reports, an allocation failu= re, > > > pidff_init_fields(), pidff_check_autocenter(), an unusable effect cou= nt, > > > input_ff_create() - all reach the same label. > > >=20 > > > Stop the device on that path. hid-dr.c and hid-emsff.c, which start = the > > > device with the same HID_CONNECT_DEFAULT & ~HID_CONNECT_FF mask, alre= ady do > > > this. The two earlier gotos must keep returning without hid_hw_stop(= ), > > > since neither has a started device, so give the path that fails after= the > > > start its own label. > > >=20 > > > Discovered by XBOW, triaged by Baul Lee > > >=20 > > > Fixes: f06bf8d94fff ("HID: Add hid-universal-pidff driver and support= ed device ids") > > > Cc: stable@vger.kernel.org > > > Signed-off-by: Baul Lee > > > --- > > > drivers/hid/hid-universal-pidff.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > >=20 > > > diff --git a/drivers/hid/hid-universal-pidff.c b/drivers/hid/hid-univ= ersal-pidff.c > > > index 549dac555d40..60180467a0bf 100644 > > > --- a/drivers/hid/hid-universal-pidff.c > > > +++ b/drivers/hid/hid-universal-pidff.c > > > @@ -104,12 +104,14 @@ static int universal_pidff_probe(struct hid_dev= ice *hdev, > > > error =3D init_function(hdev, id->driver_data); > > > if (error) { > > > hid_warn(hdev, "Error initialising force feedback\n"); > > > - goto err; > > > + goto err_stop; > > > } > > >=20 > > > hid_info(hdev, "Universal pidff driver loaded successfully!"); > > >=20 > > > return 0; > > > +err_stop: > > > + hid_hw_stop(hdev); > > > err: > > > return error; > > > } > >=20 > > Actually, now that I look at it, this isn't the only place where the > > stop should happen. Any error after successful hid_hw_start() should > > trigger hid_hw_stop(). > >=20 > > So for this to truly matter, you should replace > > return 0; > > with > > goto err_stop; > > on lines 92 and 101. Just add error value clearing so error =3D 0 becau= se > > even though we're exiting there, it's not an error. Some devices just > > show up as multiple separate HID devices. > >=20 > > Tomasz > >=20 Okay! Thanks for the detailed explanation and for the contribution. I tested this by mangling PID descriptor and indeed, like you said, it would completely disable input from the steering wheelso the current version is proper. --=20 Reviewed-by: Tomasz Paku=C5=82a