[(resend)] Input: uinput - Set name/phys to NULL before kfree().
diff mbox series

Message ID b937b59c-22b8-9113-229f-056738819467@I-love.SAKURA.ne.jp
State New
Headers show
Series
  • [(resend)] Input: uinput - Set name/phys to NULL before kfree().
Related show

Commit Message

Tetsuo Handa Feb. 8, 2019, 10:25 a.m. UTC
syzbot is hitting use-after-free bug in uinput module [1]. This is because
uinput_destroy_device() sometimes kfree()s dev->name and dev->phys at
uinput_destroy_device() before dev_uevent() is triggered by dropping the
refcount to 0. Since the timing of triggering last input_put_device() is
uncontrollable, this patch prepares for such race by setting dev->name and
dev->phys to NULL before doing operations which might drop the refcount
to 0.

[1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d

Reported-by: syzbot <syzbot+f648cfb7e0b52bf7ae32@syzkaller.appspotmail.com>
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 drivers/input/misc/uinput.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

dmitry.torokhov@gmail.com Feb. 17, 2019, 9:07 p.m. UTC | #1
Hi Tetsuo,

On Fri, Feb 08, 2019 at 07:25:52PM +0900, Tetsuo Handa wrote:
> syzbot is hitting use-after-free bug in uinput module [1]. This is because
> uinput_destroy_device() sometimes kfree()s dev->name and dev->phys at
> uinput_destroy_device() before dev_uevent() is triggered by dropping the
> refcount to 0. Since the timing of triggering last input_put_device() is
> uncontrollable, this patch prepares for such race by setting dev->name and
> dev->phys to NULL before doing operations which might drop the refcount
> to 0.
> 
> [1] https://syzkaller.appspot.com/bug?id=8b17c134fe938bbddd75a45afaa9e68af43a362d

Sorry it took me so long to sort out the issue and unfortunately I
disagree with your analysis. The issue here is not that we do not know
when last reference is being dropped (because we expect that KOBJ_REMOVE
uevent will be sent out when we call input_unregister_device, which is
quite deterministic) but the kobject cleanup logic added in commit
0f4dafc0563c6c49e17fe14b3f5f356e4c4b8806 ("Kobject: auto-cleanup on
final unref") coupled with the fault injected by the syzcaller.
The commit tries to send final uevent for objects for which "add" uevent
has been sent, but not "remove" event. However in uinput (and general
input case) we always take care of sending uevent at unregister, and do
not expect to have uevent sent out at the final "put" time.

I believe the real fix is to have kobj->state_remove_uevent_sent be set
to true as soon as we enter kobject_uevent(kobj, KOBJ_REMOVE) so that
it is being set even if memory allocation fails. Doing anything else may
violate expectations of subsystem owning the kobject.

Thanks.
Tetsuo Handa Feb. 18, 2019, 10:10 a.m. UTC | #2
Thank you for responding.

On 2019/02/18 6:07, Dmitry Torokhov wrote:
> The commit tries to send final uevent for objects for which "add" uevent
> has been sent, but not "remove" event. However in uinput (and general
> input case) we always take care of sending uevent at unregister, and do
> not expect to have uevent sent out at the final "put" time.

Then, we want to keep dev->name and dev->phys when calling "unregister" time.

> 
> I believe the real fix is to have kobj->state_remove_uevent_sent be set
> to true as soon as we enter kobject_uevent(kobj, KOBJ_REMOVE) so that
> it is being set even if memory allocation fails. Doing anything else may
> violate expectations of subsystem owning the kobject.

If we want to keep dev->name and dev->phys when calling "unregister" time,
we could do something like below. Does calling kobject_uevent(KOBJ_REMOVE)
without dev->name and dev->phys (to some degree) help (compared to not
triggering kobject_uevent(KOBJ_REMOVE) at all) ?

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3304aaa..da39a23 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1587,6 +1587,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 {
 	struct input_dev *dev = to_input_dev(device);
 
+	rcu_read_lock();
 	INPUT_ADD_HOTPLUG_VAR("PRODUCT=%x/%x/%x/%x",
 				dev->id.bustype, dev->id.vendor,
 				dev->id.product, dev->id.version);
@@ -1618,6 +1619,7 @@ static int input_dev_uevent(struct device *device, struct kobj_uevent_env *env)
 		INPUT_ADD_HOTPLUG_BM_VAR("SW=", dev->swbit, SW_MAX);
 
 	INPUT_ADD_HOTPLUG_MODALIAS_VAR(dev);
+	rcu_read_unlock();
 
 	return 0;
 }
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 26ec603f..6689312 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -308,9 +308,12 @@ static void uinput_destroy_device(struct uinput_device *udev)
 		} else {
 			input_free_device(dev);
 		}
+		dev->name = NULL;
+		dev->phys = NULL;
+		udev->dev = NULL;
+		synchronize_rcu();
 		kfree(name);
 		kfree(phys);
-		udev->dev = NULL;
 	}
 }
dmitry.torokhov@gmail.com Feb. 19, 2019, 6:58 p.m. UTC | #3
On Mon, Feb 18, 2019 at 07:10:23PM +0900, Tetsuo Handa wrote:
> Thank you for responding.
> 
> On 2019/02/18 6:07, Dmitry Torokhov wrote:
> > The commit tries to send final uevent for objects for which "add" uevent
> > has been sent, but not "remove" event. However in uinput (and general
> > input case) we always take care of sending uevent at unregister, and do
> > not expect to have uevent sent out at the final "put" time.
> 
> Then, we want to keep dev->name and dev->phys when calling "unregister" time.
> 
> > 
> > I believe the real fix is to have kobj->state_remove_uevent_sent be set
> > to true as soon as we enter kobject_uevent(kobj, KOBJ_REMOVE) so that
> > it is being set even if memory allocation fails. Doing anything else may
> > violate expectations of subsystem owning the kobject.
> 
> If we want to keep dev->name and dev->phys when calling "unregister" time,
> we could do something like below. Does calling kobject_uevent(KOBJ_REMOVE)
> without dev->name and dev->phys (to some degree) help (compared to not
> triggering kobject_uevent(KOBJ_REMOVE) at all) ?

We are talking about handling pretty bad failure (I am not sure if these
allocations can fail in real life) so not getting KOBJ_REMOVE uevent is
not a big deal.

Thanks.

Patch
diff mbox series

diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 8ec483e8688b..131591b5babd 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -300,7 +300,9 @@  static void uinput_destroy_device(struct uinput_device *udev)
 
 	if (dev) {
 		name = dev->name;
+		dev->name = NULL;
 		phys = dev->phys;
+		dev->phys = NULL;
 		if (old_state == UIST_CREATED) {
 			uinput_flush_requests(udev);
 			input_unregister_device(dev);