| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <cc7f3a74-5cb3-4e53-a941-2fc907c765f1@tuxedocomputers.com>
Date: Sat, 28 Sep 2024 09:36:53 +0200
From: Werner Sembach <wse@...edocomputers.com>
To: Armin Wolf <W_Armin@....de>, Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: bentiss@...nel.org, dri-devel@...ts.freedesktop.org, jelle@...aa.nl,
jikos@...nel.org, lee@...nel.org, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
miguel.ojeda.sandonis@...il.com, ojeda@...nel.org, onitake@...il.com,
pavel@....cz, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH 1/1] platform/x86/tuxedo: Add virtual LampArray for TUXEDO
NB04 devices
Hi Armin,
Am 27.09.24 um 19:15 schrieb Armin Wolf:
> [...]
> If so, please mark your patches as "RFC" if they are not considered as
> a potentially "final" release.
> Otherwise they might get accepted with the debug printing still inside.
Talking about noob mistakes ... I'm sorry, will do this with the next patch.
>
>>>
>>>> +
>>>> + mutex_lock(&driver_data->wmi_access_mutex);
>>>
>>> Does the underlying ACPI method really require external locking? If
>>> not, then please remove this mutex.
>> Taken from the out of tree driver written by Christoffer, I will ask
>> him about this.
>>>
>>>> + acpi_status status = wmidev_evaluate_method(wdev, 0,
>>>> wmi_method_id, &acpi_buffer_in,
>>>> + &acpi_buffer_out);
>>>> + mutex_unlock(&driver_data->wmi_access_mutex);
>>>> + if (ACPI_FAILURE(status)) {
>>>> + pr_err("Failed to evaluate WMI method.\n");
>>>> + return -EIO;
>>>> + }
>>>> + if (!acpi_buffer_out.pointer) {
>>>> + pr_err("Unexpected empty out buffer.\n");
>>>> + return -ENODATA;
>>>> + }
>>>
>>> I believe that printing error messages should be done by the callers
>>> of this method.
>>>
>>>> +
>>>> + *out = acpi_buffer_out.pointer;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int __wmi_method_buffer_out(struct wmi_device *wdev,
>>>> uint32_t wmi_method_id, uint8_t *in,
>>>> + acpi_size in_len, uint8_t *out, acpi_size out_len)
>>>
>>> Please use size_t instead of acpi_size.
>>>
>>>> +{
>>>> + int ret;
>>>> + union acpi_object *acpi_object_out = NULL;
>>>
>>> union acpi_object *obj;
>>> int ret;
>> ack ack ack
>>>
>>>> +
>>>> + ret = __wmi_method_acpi_object_out(wdev, wmi_method_id, in,
>>>> in_len, &acpi_object_out);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (acpi_object_out->type != ACPI_TYPE_BUFFER) {
>>>> + pr_err("Unexpected out buffer type. Expected: %u Got:
>>>> %u\n", ACPI_TYPE_BUFFER,
>>>> + acpi_object_out->type);
>>>> + kfree(acpi_object_out);
>>>> + return -EIO;
>>>> + }
>>>> + if (acpi_object_out->buffer.length != out_len) {
>>>
>>> The Windows ACPI-WMI mappers accepts oversized buffers and ignores
>>> any additional data,
>>> so please change this code to also accept oversized buffers.
>> Only for input or also for output?
>
> Only forbuffers coming from the ACPI firmware.
ack
>
>>>
>>>> + pr_err("Unexpected out buffer length.\n");
>>>> + kfree(acpi_object_out);
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + memcpy(out, acpi_object_out->buffer.pointer, out_len);
>>>> + kfree(acpi_object_out);
>>>> +
>>>> + return ret;
>>>> +}
>>>> +
>>>> +int tuxedo_nb04_wmi_8_b_in_80_b_out(struct wmi_device *wdev,
>>>> + enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods
>>>> method,
>>>> + union tuxedo_nb04_wmi_8_b_in_80_b_out_input
>>>> *input,
>>>> + union tuxedo_nb04_wmi_8_b_in_80_b_out_output
>>>> *output)
>>>> +{
>>>> + return __wmi_method_buffer_out(wdev, method, input->raw, 8,
>>>> output->raw, 80);
>>>> +}
>>>> +
>>>> +int tuxedo_nb04_wmi_496_b_in_80_b_out(struct wmi_device *wdev,
>>>> + enum
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_methods method,
>>>> + union
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_input *input,
>>>> + union
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_output *output)
>>>> +{
>>>> + return __wmi_method_buffer_out(wdev, method, input->raw, 496,
>>>> output->raw, 80);
>>>> +}
>>>
>>> Those two functions seem useless to me, please use
>>> wmi_method_buffer_out() directly by passing
>>> a pointer to the underlying struct as data and the output of
>>> sizeof() as length.
>> They are thought of bringing some type safety into the mix so that
>> for any method id the input/output size is correct.
>
> I do not think that this brings any real benefits when it comes to
> type safety. Using predefined structs and sizeof()
> already takes care that the buffer size is correct, and choosing the
> correct method id already needs to be done by
> the driver itself.
ack
>
>>>
>>>> diff --git a/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
>>>> b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
>>>> new file mode 100644
>>>> index 0000000000000..2765cbe9fcfef
>>>> --- /dev/null
>>>> +++ b/drivers/platform/x86/tuxedo/tuxedo_nb04_wmi_util.h
>>>> @@ -0,0 +1,112 @@
>>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>>> +/*
>>>> + * This code gives functions to avoid code duplication while
>>>> interacting with
>>>> + * the TUXEDO NB04 wmi interfaces.
>>>> + *
>>>> + * Copyright (C) 2024 Werner Sembach wse@...edocomputers.com
>>>> + */
>>>> +
>>>> +#ifndef TUXEDO_NB04_WMI_UTIL_H
>>>> +#define TUXEDO_NB04_WMI_UTIL_H
>>>> +
>>>> +#include <linux/wmi.h>
>>>> +
>>>> +#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_TOUCHPAD 1
>>>> +#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_KEYBOARD 2
>>>> +#define WMI_AB_GET_DEVICE_STATUS_DEVICE_ID_APP_PAGES 3
>>>> +
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_NONE 0
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_PER_KEY 1
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_FOUR_ZONE 2
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KBL_TYPE_WHITE_ONLY 3
>>>> +
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ANSII 0
>>>> +#define WMI_AB_GET_DEVICE_STATUS_KEYBOARD_LAYOUT_ISO 1
>>>> +
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_RED 1
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_GREEN 2
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_YELLOW 3
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_BLUE 4
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_PURPLE 5
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_INDIGO 6
>>>> +#define WMI_AB_GET_DEVICE_STATUS_COLOR_ID_WHITE 7
>>>> +
>>>> +#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_DASHBOARD BIT(0)
>>>> +#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_SYSTEMINFOS BIT(1)
>>>> +#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_KBL BIT(2)
>>>> +#define WMI_AB_GET_DEVICE_STATUS_APP_PAGES_HOTKEYS BIT(3)
>>>> +
>>>> +
>>>> +union tuxedo_nb04_wmi_8_b_in_80_b_out_input {
>>>> + uint8_t raw[8];
>>>> + struct __packed {
>>>> + uint8_t device_type;
>>>> + uint8_t reserved_0[7];
>>>> + } get_device_status_input;
>>>> +};
>>>> +
>>>> +union tuxedo_nb04_wmi_8_b_in_80_b_out_output {
>>>> + uint8_t raw[80];
>>>> + struct __packed {
>>>> + uint16_t return_status;
>>>> + uint8_t device_enabled;
>>>> + uint8_t kbl_type;
>>>> + uint8_t kbl_side_bar_supported;
>>>> + uint8_t keyboard_physical_layout;
>>>> + uint8_t app_pages;
>>>> + uint8_t per_key_kbl_default_color;
>>>> + uint8_t four_zone_kbl_default_color_1;
>>>> + uint8_t four_zone_kbl_default_color_2;
>>>> + uint8_t four_zone_kbl_default_color_3;
>>>> + uint8_t four_zone_kbl_default_color_4;
>>>> + uint8_t light_bar_kbl_default_color;
>>>> + uint8_t reserved_0[1];
>>>> + uint16_t dedicated_gpu_id;
>>>> + uint8_t reserved_1[64];
>>>> + } get_device_status_output;
>>>> +};
>>>> +
>>>> +enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods {
>>>> + WMI_AB_GET_DEVICE_STATUS = 2,
>>>> +};
>>>> +
>>>> +
>>>> +#define WMI_AB_KBL_SET_MULTIPLE_KEYS_LIGHTING_SETTINGS_COUNT_MAX 120
>>>> +
>>>> +union tuxedo_nb04_wmi_496_b_in_80_b_out_input {
>>>> + uint8_t raw[496];
>>>> + struct __packed {
>>>> + uint8_t reserved_0[15];
>>>> + uint8_t lighting_setting_count;
>>>> + struct {
>>>> + uint8_t key_id;
>>>> + uint8_t red;
>>>> + uint8_t green;
>>>> + uint8_t blue;
>>>> + }
>>>> lighting_settings[WMI_AB_KBL_SET_MULTIPLE_KEYS_LIGHTING_SETTINGS_COUNT_MAX];
>>>> + } kbl_set_multiple_keys_input;
>>>> +};
>>>> +
>>>> +union tuxedo_nb04_wmi_496_b_in_80_b_out_output {
>>>> + uint8_t raw[80];
>>>> + struct __packed {
>>>> + uint8_t return_value;
>>>> + uint8_t reserved_0[79];
>>>> + } kbl_set_multiple_keys_output;
>>>> +};
>>>> +
>>>> +enum tuxedo_nb04_wmi_496_b_in_80_b_out_methods {
>>>> + WMI_AB_KBL_SET_MULTIPLE_KEYS = 6,
>>>> +};
>>>> +
>>>> +
>>>> +int tuxedo_nb04_wmi_8_b_in_80_b_out(struct wmi_device *wdev,
>>>> + enum tuxedo_nb04_wmi_8_b_in_80_b_out_methods
>>>> method,
>>>> + union tuxedo_nb04_wmi_8_b_in_80_b_out_input
>>>> *input,
>>>> + union tuxedo_nb04_wmi_8_b_in_80_b_out_output
>>>> *output);
>>>> +int tuxedo_nb04_wmi_496_b_in_80_b_out(struct wmi_device *wdev,
>>>> + enum
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_methods method,
>>>> + union
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_input *input,
>>>> + union
>>>> tuxedo_nb04_wmi_496_b_in_80_b_out_output *output);
>>>> +
>>>> +#endif
Powered by blists - more mailing lists