Re: [PATCH] platform/x86: intel-vbtn: Protect ACPI notify handler against recursion

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 29 Jul 2024, Hans de Goede wrote:

> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on
> all CPUs") ACPI notify handlers like the intel-vbtn notify_handler() may
> run on multiple CPU cores racing with themselves.
> 
> This race gets hit on Dell Venue 7140 tablets when undocking from
> the keyboard, causing the handler to try and register priv->switches_dev
> twice, as can be seen from the dev_info() message getting logged twice:
> 
> [ 83.861800] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
> [ 83.861858] input: Intel Virtual Switches as /devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17
> [ 83.861865] intel-vbtn INT33D6:00: Registering Intel Virtual Switches input-dev after receiving a switch event
> 
> After which things go seriously wrong:
> [ 83.861872] sysfs: cannot create duplicate filename '/devices/pci0000:00/0000:00:1f.0/PNP0C09:00/INT33D6:00/input/input17'
> ...
> [ 83.861967] kobject: kobject_add_internal failed for input17 with -EEXIST, don't try to register things with the same name in the same directory.
> [ 83.877338] BUG: kernel NULL pointer dereference, address: 0000000000000018
> ...
> 
> Protect intel-vbtn notify_handler() from racing with itself with a mutex
> to fix this.
> 
> Fixes: e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run on all CPUs")
> Reported-by: En-Wei Wu <en-wei.wu@canonical.com>
> Closes: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2073001
> Tested-by: Kostadin Stoilov <kmstoilov@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/platform/x86/intel/vbtn.c | 21 ++++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/vbtn.c b/drivers/platform/x86/intel/vbtn.c
> index 9b7ce03ba085..93deda7daac4 100644
> --- a/drivers/platform/x86/intel/vbtn.c
> +++ b/drivers/platform/x86/intel/vbtn.c
> @@ -12,6 +12,7 @@
>  #include <linux/input/sparse-keymap.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
>  #include <linux/platform_device.h>
>  #include <linux/suspend.h>
>  #include "../dual_accel_detect.h"
> @@ -66,6 +67,7 @@ static const struct key_entry intel_vbtn_switchmap[] = {
>  };
>  
>  struct intel_vbtn_priv {
> +	struct mutex mutex; /* Avoid notify_handler() racing with itself */
>  	struct input_dev *buttons_dev;
>  	struct input_dev *switches_dev;
>  	bool dual_accel;
> @@ -155,30 +157,32 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	bool autorelease;
>  	int ret;
>  
> +	mutex_lock(&priv->mutex);
> +
>  	if ((ke = sparse_keymap_entry_from_scancode(priv->buttons_dev, event))) {
>  		if (!priv->has_buttons) {
>  			dev_warn(&device->dev, "Warning: received 0x%02x button event on a device without buttons, please report this.\n",
>  				 event);
> -			return;
> +			goto out_unlock;
>  		}
>  		input_dev = priv->buttons_dev;
>  	} else if ((ke = sparse_keymap_entry_from_scancode(priv->switches_dev, event))) {
>  		if (!priv->has_switches) {
>  			/* See dual_accel_detect.h for more info */
>  			if (priv->dual_accel)
> -				return;
> +				goto out_unlock;
>  
>  			dev_info(&device->dev, "Registering Intel Virtual Switches input-dev after receiving a switch event\n");
>  			ret = input_register_device(priv->switches_dev);
>  			if (ret)
> -				return;
> +				goto out_unlock;
>  
>  			priv->has_switches = true;
>  		}
>  		input_dev = priv->switches_dev;
>  	} else {
>  		dev_dbg(&device->dev, "unknown event index 0x%x\n", event);
> -		return;
> +		goto out_unlock;
>  	}
>  
>  	if (priv->wakeup_mode) {
> @@ -189,7 +193,7 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  		 * mirroring how the drivers/acpi/button.c code skips this too.
>  		 */
>  		if (ke->type == KE_KEY)
> -			return;
> +			goto out_unlock;
>  	}
>  
>  	/*
> @@ -200,6 +204,9 @@ static void notify_handler(acpi_handle handle, u32 event, void *context)
>  	autorelease = val && (!ke_rel || ke_rel->type == KE_IGNORE);
>  
>  	sparse_keymap_report_event(input_dev, event, val, autorelease);
> +
> +out_unlock:
> +	mutex_unlock(&priv->mutex);

Please use guard() and keep the return statements as is + add cleanup.h 
include if not yet there.

-- 
 i.


>  }
>  
>  /*
> @@ -290,6 +297,10 @@ static int intel_vbtn_probe(struct platform_device *device)
>  		return -ENOMEM;
>  	dev_set_drvdata(&device->dev, priv);
>  
> +	err = devm_mutex_init(&device->dev, &priv->mutex);
> +	if (err)
> +		return err;
> +
>  	priv->dual_accel = dual_accel;
>  	priv->has_buttons = has_buttons;
>  	priv->has_switches = has_switches;
>