Re: [PATCH v12 10/16] platform/x86: dell-smbios: Introduce dispatcher for SMM calls

["Pali Rohár" <pali.rohar@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3768 bytes --]

Hi!

On Wednesday 01 November 2017 14:25:31 Mario Limonciello wrote:
> This splits up the dell-smbios driver into two drivers:
> * dell-smbios
> * dell-smbios-smm
> 
> dell-smbios can operate with multiple different dispatcher drivers to
> perform SMBIOS operations.
> 
> Also modify the interface that dell-laptop and dell-wmi use align to this
> model more closely.  Rather than a single global buffer being allocated
> for all drivers, each driver will allocate and be responsible for it's own
> buffer. The pointer will be passed to the calling function and each
> dispatcher driver will then internally copy it to the proper location to
> perform it's call.
> 
> Add defines for calls used by these methods in the dell-smbios.h header
> for tracking purposes.
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@dell.com>
> Reviewed-by: Edward O'Callaghan <quasisec@google.com>
> ---
...
> @@ -85,6 +73,7 @@ static struct platform_driver platform_driver = {
>  	}
>  };
>  
> +static struct calling_interface_buffer *buffer;
>  static struct platform_device *platform_device;
>  static struct backlight_device *dell_backlight_device;
>  static struct rfkill *wifi_rfkill;
> @@ -283,6 +272,27 @@ static const struct dmi_system_id dell_quirks[] __initconst = {
>  	{ }
>  };
>  
> +void dell_set_arguments(u32 arg0, u32 arg1, u32 arg2, u32 arg3)
> +{
> +	memset(buffer, 0, sizeof(struct calling_interface_buffer));
> +	buffer->input[0] = arg0;
> +	buffer->input[1] = arg1;
> +	buffer->input[2] = arg2;
> +	buffer->input[3] = arg3;
> +}
> +
> +int dell_send_request(u16 class, u16 select)
> +{
> +	int ret;
> +
> +	buffer->cmd_class = class;
> +	buffer->cmd_select = select;
> +	ret = dell_smbios_call(buffer);
> +	if (ret != 0)
> +		return ret;
> +	return dell_smbios_error(buffer->output[0]);
> +}
> +
>  /*
>   * Derived from information in smbios-wireless-ctl:
>   *
...
> @@ -413,20 +422,16 @@ static int dell_rfkill_set(void *data, bool blocked)
>  	int status;
>  	int ret;
>  
> -	buffer = dell_smbios_get_buffer();
> -
> -	dell_smbios_send_request(17, 11);
> -	ret = buffer->output[0];
> +	dell_set_arguments(0, 0, 0, 0);
> +	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
> +	if (ret)
> +		return ret;
>  	status = buffer->output[1];

Now I'm looking at this patch again and I think it introduced a new race
condition.

Prior this patch, dell_smbios_get_buffer() acquired mutex and returned
pointer to buffer which caller used and then released.

Now buffer is allocated at module load time and is shared for all
functions in this module. And hen is used, there is no mutex protection
for it.

First call is to dell_set_arguments() which modifies that shared buffer
and then dell_send_request() is used to modify and pass buffer to
another function dell_smbios_call().

And function below dell_update_rfkill() is called work queue which also
modifies buffer by dell_set_arguments() function.

Therefore it looks like when dell_update_rfkill() is scheduled at the
same time as dell_rfkill_set() is executed, then both functions
overwrite one shared buffer and something unexpected would be sent to
SMM.

> @@ -613,46 +596,36 @@ static const struct file_operations dell_debugfs_fops = {
>  
>  static void dell_update_rfkill(struct work_struct *ignored)
>  {
> -	struct calling_interface_buffer *buffer;
>  	int hwswitch = 0;
>  	int status;
>  	int ret;
>  
> -	buffer = dell_smbios_get_buffer();
> -
> -	dell_smbios_send_request(17, 11);
> -	ret = buffer->output[0];
> +	dell_set_arguments(0, 0, 0, 0);
> +	ret = dell_send_request(CLASS_INFO, SELECT_RFKILL);
>  	status = buffer->output[1];

-- 
Pali Rohár
pali.rohar@gmail.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]