* [PATCH] Check whether getkeycode and setkeycode are still valide @ 2010-03-21 2:56 Yong Wang 2010-03-21 3:11 ` Dmitry Torokhov 0 siblings, 1 reply; 5+ messages in thread From: Yong Wang @ 2010-03-21 2:56 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input If sparse keymap is freed before unregistering the device, there is a window where userspace can issue EVIOCGKEYCODE or EVIOCSKEYCODE. When that happens, kernel will crash. Noticed by Dmitry Torokhov. Signed-off-by: Yong Wang <yong.y.wang@intel.com> --- drivers/input/input.c | 6 ++++++ drivers/input/sparse-keymap.c | 6 ++++++ 2 files changed, 12 insertions(+) diff --git a/drivers/input/input.c b/drivers/input/input.c index e2aad0a..bfe4bdc 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -659,6 +659,9 @@ static int input_default_setkeycode(struct input_dev *dev, int input_get_keycode(struct input_dev *dev, unsigned int scancode, unsigned int *keycode) { + if (!dev->getkeycode) + return -ENODEV; + return dev->getkeycode(dev, scancode, keycode); } EXPORT_SYMBOL(input_get_keycode); @@ -682,6 +685,9 @@ int input_set_keycode(struct input_dev *dev, if (keycode > KEY_MAX) return -EINVAL; + if (!dev->getkeycode || !dev->setkeycode) + return -ENODEV; + spin_lock_irqsave(&dev->event_lock, flags); retval = dev->getkeycode(dev, scancode, &old_keycode); diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c index f64e004..25b8d7d 100644 --- a/drivers/input/sparse-keymap.c +++ b/drivers/input/sparse-keymap.c @@ -34,6 +34,9 @@ struct key_entry *sparse_keymap_entry_from_scancode(struct input_dev *dev, { struct key_entry *key; + if (!dev->keycode) + return NULL; + for (key = dev->keycode; key->type != KE_END; key++) if (code == key->code) return key; @@ -55,6 +58,9 @@ struct key_entry *sparse_keymap_entry_from_keycode(struct input_dev *dev, { struct key_entry *key; + if (!dev->keycode) + return NULL; + for (key = dev->keycode; key->type != KE_END; key++) if (key->type == KE_KEY && keycode == key->keycode) return key; ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Check whether getkeycode and setkeycode are still valide 2010-03-21 2:56 [PATCH] Check whether getkeycode and setkeycode are still valide Yong Wang @ 2010-03-21 3:11 ` Dmitry Torokhov 2010-03-22 2:48 ` Yong Wang 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Torokhov @ 2010-03-21 3:11 UTC (permalink / raw) To: Yong Wang; +Cc: linux-input On Sun, Mar 21, 2010 at 10:56:48AM +0800, Yong Wang wrote: > If sparse keymap is freed before unregistering the device, there is a > window where userspace can issue EVIOCGKEYCODE or EVIOCSKEYCODE. When > that happens, kernel will crash. Noticed by Dmitry Torokhov. > I'd rather require that getkeycode and setkeycode be mandatory. I will change sparse-kepmap module to stop setting these to NULL when freeing keymap. Thanks. -- Dmitry ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Check whether getkeycode and setkeycode are still valide 2010-03-21 3:11 ` Dmitry Torokhov @ 2010-03-22 2:48 ` Yong Wang 2010-03-22 4:22 ` Dmitry Torokhov 0 siblings, 1 reply; 5+ messages in thread From: Yong Wang @ 2010-03-22 2:48 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input On Sat, Mar 20, 2010 at 08:11:49PM -0700, Dmitry Torokhov wrote: > On Sun, Mar 21, 2010 at 10:56:48AM +0800, Yong Wang wrote: > > If sparse keymap is freed before unregistering the device, there is a > > window where userspace can issue EVIOCGKEYCODE or EVIOCSKEYCODE. When > > that happens, kernel will crash. Noticed by Dmitry Torokhov. > > > > I'd rather require that getkeycode and setkeycode be mandatory. I will > change sparse-kepmap module to stop setting these to NULL when freeing > keymap. > OK, I agree it would be better to require getkeycode and setkeycode be mandatory. Then what about setting getkeycode and setkeycode to the default handlers input_default_getkeycode and input_default_setkeycode in sparse_keymap_free? This way it will not pass the check below in the transient period time after calling sparse_keymap_free and before unregistering input device since dev->keycodemax is set to 0 in sparse_keymap_free. if (scancode >= dev->keycodemax) return -EINVAL; Thanks -Yong --- diff --git a/drivers/input/input.c b/drivers/input/input.c index e2aad0a..18c1d0b 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -581,7 +581,7 @@ static int input_fetch_keycode(struct input_dev *dev, int scancode) } } -static int input_default_getkeycode(struct input_dev *dev, +int input_default_getkeycode(struct input_dev *dev, unsigned int scancode, unsigned int *keycode) { @@ -595,8 +595,9 @@ static int input_default_getkeycode(struct input_dev *dev, return 0; } +EXPORT_SYMBOL(input_default_getkeycode); -static int input_default_setkeycode(struct input_dev *dev, +int input_default_setkeycode(struct input_dev *dev, unsigned int scancode, unsigned int keycode) { @@ -645,6 +646,7 @@ static int input_default_setkeycode(struct input_dev *dev, return 0; } +EXPORT_SYMBOL(input_default_setkeycode); /** * input_get_keycode - retrieve keycode currently mapped to a given scancode diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c index f64e004..2e30887 100644 --- a/drivers/input/sparse-keymap.c +++ b/drivers/input/sparse-keymap.c @@ -181,8 +181,8 @@ void sparse_keymap_free(struct input_dev *dev) kfree(dev->keycode); dev->keycode = NULL; dev->keycodemax = 0; - dev->getkeycode = NULL; - dev->setkeycode = NULL; + dev->getkeycode = input_default_getkeycode; + dev->setkeycode = input_default_setkeycode; } EXPORT_SYMBOL(sparse_keymap_free); diff --git a/include/linux/input.h b/include/linux/input.h index 7ed2251..873c250 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -1417,6 +1417,13 @@ static inline void input_set_abs_params(struct input_dev *dev, int axis, int min dev->absbit[BIT_WORD(axis)] |= BIT_MASK(axis); } +int input_default_getkeycode(struct input_dev *dev, + unsigned int scancode, + unsigned int *keycode); +int input_default_setkeycode(struct input_dev *dev, + unsigned int scancode, + unsigned int keycode); + int input_get_keycode(struct input_dev *dev, unsigned int scancode, unsigned int *keycode); int input_set_keycode(struct input_dev *dev, ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Check whether getkeycode and setkeycode are still valide 2010-03-22 2:48 ` Yong Wang @ 2010-03-22 4:22 ` Dmitry Torokhov 2010-03-22 5:39 ` Yong Wang 0 siblings, 1 reply; 5+ messages in thread From: Dmitry Torokhov @ 2010-03-22 4:22 UTC (permalink / raw) To: Yong Wang; +Cc: linux-input On Mon, Mar 22, 2010 at 10:48:11AM +0800, Yong Wang wrote: > On Sat, Mar 20, 2010 at 08:11:49PM -0700, Dmitry Torokhov wrote: > > On Sun, Mar 21, 2010 at 10:56:48AM +0800, Yong Wang wrote: > > > If sparse keymap is freed before unregistering the device, there is a > > > window where userspace can issue EVIOCGKEYCODE or EVIOCSKEYCODE. When > > > that happens, kernel will crash. Noticed by Dmitry Torokhov. > > > > > > > I'd rather require that getkeycode and setkeycode be mandatory. I will > > change sparse-kepmap module to stop setting these to NULL when freeing > > keymap. > > > > OK, I agree it would be better to require getkeycode and setkeycode be > mandatory. Then what about setting getkeycode and setkeycode to the > default handlers input_default_getkeycode and input_default_setkeycode > in sparse_keymap_free? This way it will not pass the check below in the > transient period time after calling sparse_keymap_free and before > unregistering input device since dev->keycodemax is set to 0 in > sparse_keymap_free. > > if (scancode >= dev->keycodemax) > return -EINVAL; > I'd rather not export the default handlers how about the patch below instead? -- Dmitry Input: sparse-keymap - implement safer freeing of the keymap Allow calling sparse_keymap_free() before unregistering input device whithout risk of racing with EVIOCGETKEYCODE and EVIOCSETKEYCODE. This makes life of drivers writers easier. Signed-off-by: Dmitry Torokhov <dtor@mail.ru> --- drivers/input/input.c | 9 +++++++ drivers/input/sparse-keymap.c | 50 ++++++++++++++++++++++++++--------------- 2 files changed, 40 insertions(+), 19 deletions(-) diff --git a/drivers/input/input.c b/drivers/input/input.c index e2aad0a..be18fa9 100644 --- a/drivers/input/input.c +++ b/drivers/input/input.c @@ -659,7 +659,14 @@ static int input_default_setkeycode(struct input_dev *dev, int input_get_keycode(struct input_dev *dev, unsigned int scancode, unsigned int *keycode) { - return dev->getkeycode(dev, scancode, keycode); + unsigned long flags; + int retval; + + spin_lock_irqsave(&dev->event_lock, flags); + retval = dev->getkeycode(dev, scancode, keycode); + spin_unlock_irqrestore(&dev->event_lock, flags); + + return retval; } EXPORT_SYMBOL(input_get_keycode); diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c index f64e004..f896f24 100644 --- a/drivers/input/sparse-keymap.c +++ b/drivers/input/sparse-keymap.c @@ -67,12 +67,14 @@ static int sparse_keymap_getkeycode(struct input_dev *dev, unsigned int scancode, unsigned int *keycode) { - const struct key_entry *key = - sparse_keymap_entry_from_scancode(dev, scancode); + const struct key_entry *key; - if (key && key->type == KE_KEY) { - *keycode = key->keycode; - return 0; + if (dev->keycode) { + key = sparse_keymap_entry_from_scancode(dev, scancode); + if (key && key->type == KE_KEY) { + *keycode = key->keycode; + return 0; + } } return -EINVAL; @@ -85,17 +87,16 @@ static int sparse_keymap_setkeycode(struct input_dev *dev, struct key_entry *key; int old_keycode; - if (keycode < 0 || keycode > KEY_MAX) - return -EINVAL; - - key = sparse_keymap_entry_from_scancode(dev, scancode); - if (key && key->type == KE_KEY) { - old_keycode = key->keycode; - key->keycode = keycode; - set_bit(keycode, dev->keybit); - if (!sparse_keymap_entry_from_keycode(dev, old_keycode)) - clear_bit(old_keycode, dev->keybit); - return 0; + if (dev->keymap) { + key = sparse_keymap_entry_from_scancode(dev, scancode); + if (key && key->type == KE_KEY) { + old_keycode = key->keycode; + key->keycode = keycode; + set_bit(keycode, dev->keybit); + if (!sparse_keymap_entry_from_keycode(dev, old_keycode)) + clear_bit(old_keycode, dev->keybit); + return 0; + } } return -EINVAL; @@ -175,14 +176,27 @@ EXPORT_SYMBOL(sparse_keymap_setup); * * This function is used to free memory allocated by sparse keymap * in an input device that was set up by sparse_keymap_setup(). + * NOTE: It is safe to cal this function while input device is + * still registered (however the drivers should care not to try to + * use freed keymap and thus have to shut off interrups/polling + * before freeing the keymap). */ void sparse_keymap_free(struct input_dev *dev) { + unsigned long flags; + + /* + * Take event lock to prevent racing with input_get_keycode() + * and input_set_keycode() if we are called while input device + * is still registered. + */ + spin_lock_irqsave(&dev->event_lock, flags); + kfree(dev->keycode); dev->keycode = NULL; dev->keycodemax = 0; - dev->getkeycode = NULL; - dev->setkeycode = NULL; + + spin_unlock_irqrestore(&dev->event_lock, flags); } EXPORT_SYMBOL(sparse_keymap_free); ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Check whether getkeycode and setkeycode are still valide 2010-03-22 4:22 ` Dmitry Torokhov @ 2010-03-22 5:39 ` Yong Wang 0 siblings, 0 replies; 5+ messages in thread From: Yong Wang @ 2010-03-22 5:39 UTC (permalink / raw) To: Dmitry Torokhov; +Cc: linux-input On Sun, Mar 21, 2010 at 09:22:34PM -0700, Dmitry Torokhov wrote: > On Mon, Mar 22, 2010 at 10:48:11AM +0800, Yong Wang wrote: > > On Sat, Mar 20, 2010 at 08:11:49PM -0700, Dmitry Torokhov wrote: > > > On Sun, Mar 21, 2010 at 10:56:48AM +0800, Yong Wang wrote: > > > > If sparse keymap is freed before unregistering the device, there is a > > > > window where userspace can issue EVIOCGKEYCODE or EVIOCSKEYCODE. When > > > > that happens, kernel will crash. Noticed by Dmitry Torokhov. > > > > > > > > > > I'd rather require that getkeycode and setkeycode be mandatory. I will > > > change sparse-kepmap module to stop setting these to NULL when freeing > > > keymap. > > > > > > > OK, I agree it would be better to require getkeycode and setkeycode be > > mandatory. Then what about setting getkeycode and setkeycode to the > > default handlers input_default_getkeycode and input_default_setkeycode > > in sparse_keymap_free? This way it will not pass the check below in the > > transient period time after calling sparse_keymap_free and before > > unregistering input device since dev->keycodemax is set to 0 in > > sparse_keymap_free. > > > > if (scancode >= dev->keycodemax) > > return -EINVAL; > > > > I'd rather not export the default handlers how about the patch below > instead? > > -- > Dmitry > > > Input: sparse-keymap - implement safer freeing of the keymap > > Allow calling sparse_keymap_free() before unregistering input device > whithout risk of racing with EVIOCGETKEYCODE and EVIOCSETKEYCODE. > This makes life of drivers writers easier. > > Signed-off-by: Dmitry Torokhov <dtor@mail.ru> > --- > > drivers/input/input.c | 9 +++++++ > drivers/input/sparse-keymap.c | 50 ++++++++++++++++++++++++++--------------- > 2 files changed, 40 insertions(+), 19 deletions(-) > > > diff --git a/drivers/input/input.c b/drivers/input/input.c > index e2aad0a..be18fa9 100644 > --- a/drivers/input/input.c > +++ b/drivers/input/input.c > @@ -659,7 +659,14 @@ static int input_default_setkeycode(struct input_dev *dev, > int input_get_keycode(struct input_dev *dev, > unsigned int scancode, unsigned int *keycode) > { > - return dev->getkeycode(dev, scancode, keycode); > + unsigned long flags; > + int retval; > + > + spin_lock_irqsave(&dev->event_lock, flags); > + retval = dev->getkeycode(dev, scancode, keycode); > + spin_unlock_irqrestore(&dev->event_lock, flags); > + > + return retval; > } > EXPORT_SYMBOL(input_get_keycode); > > diff --git a/drivers/input/sparse-keymap.c b/drivers/input/sparse-keymap.c > index f64e004..f896f24 100644 > --- a/drivers/input/sparse-keymap.c > +++ b/drivers/input/sparse-keymap.c > @@ -67,12 +67,14 @@ static int sparse_keymap_getkeycode(struct input_dev *dev, > unsigned int scancode, > unsigned int *keycode) > { > - const struct key_entry *key = > - sparse_keymap_entry_from_scancode(dev, scancode); > + const struct key_entry *key; > > - if (key && key->type == KE_KEY) { > - *keycode = key->keycode; > - return 0; > + if (dev->keycode) { > + key = sparse_keymap_entry_from_scancode(dev, scancode); > + if (key && key->type == KE_KEY) { > + *keycode = key->keycode; > + return 0; > + } > } > > return -EINVAL; > @@ -85,17 +87,16 @@ static int sparse_keymap_setkeycode(struct input_dev *dev, > struct key_entry *key; > int old_keycode; > > - if (keycode < 0 || keycode > KEY_MAX) > - return -EINVAL; > - > - key = sparse_keymap_entry_from_scancode(dev, scancode); > - if (key && key->type == KE_KEY) { > - old_keycode = key->keycode; > - key->keycode = keycode; > - set_bit(keycode, dev->keybit); > - if (!sparse_keymap_entry_from_keycode(dev, old_keycode)) > - clear_bit(old_keycode, dev->keybit); > - return 0; > + if (dev->keymap) { > + key = sparse_keymap_entry_from_scancode(dev, scancode); > + if (key && key->type == KE_KEY) { > + old_keycode = key->keycode; > + key->keycode = keycode; > + set_bit(keycode, dev->keybit); > + if (!sparse_keymap_entry_from_keycode(dev, old_keycode)) > + clear_bit(old_keycode, dev->keybit); > + return 0; > + } > } > > return -EINVAL; > @@ -175,14 +176,27 @@ EXPORT_SYMBOL(sparse_keymap_setup); > * > * This function is used to free memory allocated by sparse keymap > * in an input device that was set up by sparse_keymap_setup(). > + * NOTE: It is safe to cal this function while input device is > + * still registered (however the drivers should care not to try to > + * use freed keymap and thus have to shut off interrups/polling > + * before freeing the keymap). > */ > void sparse_keymap_free(struct input_dev *dev) > { > + unsigned long flags; > + > + /* > + * Take event lock to prevent racing with input_get_keycode() > + * and input_set_keycode() if we are called while input device > + * is still registered. > + */ > + spin_lock_irqsave(&dev->event_lock, flags); > + > kfree(dev->keycode); > dev->keycode = NULL; > dev->keycodemax = 0; > - dev->getkeycode = NULL; > - dev->setkeycode = NULL; > + > + spin_unlock_irqrestore(&dev->event_lock, flags); > } > EXPORT_SYMBOL(sparse_keymap_free); > Cool! It is better to use lock to prevent such race conditions as you did. Thanks for fixing this! Acked-by: Yong Wang <yong.y.wang@intel.com> -Yong ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-03-22 5:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-03-21 2:56 [PATCH] Check whether getkeycode and setkeycode are still valide Yong Wang 2010-03-21 3:11 ` Dmitry Torokhov 2010-03-22 2:48 ` Yong Wang 2010-03-22 4:22 ` Dmitry Torokhov 2010-03-22 5:39 ` Yong Wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).