[PATCH] Check whether getkeycode and setkeycode are still valide

[PATCH] Check whether getkeycode and setkeycode are still valide

[Yong Wang]

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;

Re: [PATCH] Check whether getkeycode and setkeycode are still valide

[Dmitry Torokhov]

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

Re: [PATCH] Check whether getkeycode and setkeycode are still valide

[Yong Wang]

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,

Re: [PATCH] Check whether getkeycode and setkeycode are still valide

[Dmitry Torokhov]

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);
 

Re: [PATCH] Check whether getkeycode and setkeycode are still valide

[Yong Wang]

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