| 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: <ffae13ae-ed92-44a0-ae15-b6b941620c8a@gmx.de>
Date: Wed, 6 Nov 2024 22:31:23 +0100
From: Armin Wolf <W_Armin@....de>
To: Mario Limonciello <mario.limonciello@....com>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: "Rafael J . Wysocki" <rafael@...nel.org>, Len Brown <lenb@...nel.org>,
Maximilian Luz <luzmaximilian@...il.com>, Lee Chun-Yi <jlee@...e.com>,
Shyam Sundar S K <Shyam-sundar.S-k@....com>,
Corentin Chary <corentin.chary@...il.com>, "Luke D . Jones"
<luke@...nes.dev>, Ike Panhc <ike.pan@...onical.com>,
Henrique de Moraes Holschuh <hmh@....eng.br>,
Alexis Belmonte <alexbelm48@...il.com>,
Uwe Kleine-König <u.kleine-koenig@...gutronix.de>,
Ai Chao <aichao@...inos.cn>, Gergo Koteles <soyer@....hu>,
open list <linux-kernel@...r.kernel.org>,
"open list:ACPI" <linux-acpi@...r.kernel.org>,
"open list:MICROSOFT SURFACE PLATFORM PROFILE DRIVER"
<platform-driver-x86@...r.kernel.org>,
"open list:THINKPAD ACPI EXTRAS DRIVER"
<ibm-acpi-devel@...ts.sourceforge.net>,
Mark Pearson <mpearson-lenovo@...ebb.ca>,
Matthew Schwartz <matthew.schwartz@...ux.dev>
Subject: Re: [PATCH v4 16/20] ACPI: platform_profile: Make sure all profile
handlers agree on profile
Am 06.11.24 um 22:25 schrieb Mario Limonciello:
> On 11/6/2024 14:58, Armin Wolf wrote:
>> Am 05.11.24 um 16:33 schrieb Mario Limonciello:
>>
>>> If for any reason multiple profile handlers don't agree on the profile
>>> return the custom profile.
>>>
>>> Reviewed-by: Mark Pearson <mpearson-lenovo@...ebb.ca>
>>> Signed-off-by: Mario Limonciello <mario.limonciello@....com>
>>> ---
>>> drivers/acpi/platform_profile.c | 84
>>> +++++++++++++++++++++------------
>>> 1 file changed, 53 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/acpi/platform_profile.c b/drivers/acpi/
>>> platform_profile.c
>>> index d8c2d195106c2..7861fccc2e58c 100644
>>> --- a/drivers/acpi/platform_profile.c
>>> +++ b/drivers/acpi/platform_profile.c
>>> @@ -280,55 +280,77 @@ static ssize_t
>>> platform_profile_choices_show(struct device *dev,
>>> return _commmon_choices_show(aggregate, buf);
>>> }
>>>
>>> -static ssize_t platform_profile_show(struct device *dev,
>>> - struct device_attribute *attr,
>>> - char *buf)
>>> +/**
>>> + * _aggregate_profiles - Aggregate the profiles for legacy sysfs
>>> interface
>>> + * @dev: The device
>>> + * @data: The profile to return
>>> + * Return: 0 on success, -errno on failure
>>> + */
>>> +static int _aggregate_profiles(struct device *dev, void *data)
>>> {
>>> - enum platform_profile_option profile = PLATFORM_PROFILE_BALANCED;
>>> + enum platform_profile_option *profile = data;
>>> + enum platform_profile_option val;
>>> int err;
>>>
>>> - scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> - if (!cur_profile)
>>> - return -ENODEV;
>>> + err = get_class_profile(dev, &val);
>>> + if (err)
>>> + return err;
>>>
>>> - err = cur_profile->profile_get(cur_profile, &profile);
>>> - if (err)
>>> - return err;
>>> - }
>>> + if (*profile != PLATFORM_PROFILE_LAST && *profile != val)
>>> + *profile = PLATFORM_PROFILE_CUSTOM;
>>> + else
>>> + *profile = val;
>>>
>>> - /* Check that profile is valid index */
>>> - if (WARN_ON((profile < 0) || (profile >=
>>> ARRAY_SIZE(profile_names))))
>>> - return -EIO;
>>
>> Please check the returned value from profile_get() first before doing
>> the custom platform
>> profile check.
>
> You might have missed it - that's part of get_class_profile() already.
>
You are right, my bad.
>>
>>> + return 0;
>>> +}
>>> +
>>> +/**
>>> + * platform_profile_show - Show the current profile for legacy
>>> sysfs interface
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to write to
>>> + * Return: The number of bytes written
>>> + */
>>> +static ssize_t platform_profile_show(struct device *dev,
>>> + struct device_attribute *attr,
>>> + char *buf)
>>> +{
>>> + enum platform_profile_option profile = PLATFORM_PROFILE_LAST;
>>> + int err;
>>> +
>>> + err = class_for_each_device(&platform_profile_class, NULL,
>>> + &profile, _aggregate_profiles);
>>
>> Missing error handling.
>>
>
> Right, got it.
>
>>>
>>> return sysfs_emit(buf, "%s\n", profile_names[profile]);
>>> }
>>>
>>> +/**
>>> + * platform_profile_store - Set the profile for legacy sysfs interface
>>> + * @dev: The device
>>> + * @attr: The attribute
>>> + * @buf: The buffer to read from
>>> + * @count: The number of bytes to read
>>> + * Return: The number of bytes read
>>> + */
>>> static ssize_t platform_profile_store(struct device *dev,
>>> - struct device_attribute *attr,
>>> - const char *buf, size_t count)
>>> + struct device_attribute *attr,
>>> + const char *buf, size_t count)
>>> {
>>> - int err, i;
>>> + int ret;
>>> + int i;
>>>
>>> /* Scan for a matching profile */
>>> i = sysfs_match_string(profile_names, buf);
>>> if (i < 0)
>>> return -EINVAL;
>>> -
>>> - scoped_cond_guard(mutex_intr, return -ERESTARTSYS,
>>> &profile_lock) {
>>> - if (!cur_profile)
>>> - return -ENODEV;
>>> -
>>> - /* Check that platform supports this profile choice */
>>> - if (!test_bit(i, cur_profile->choices))
>>> - return -EOPNOTSUPP;
>>> -
>>> - err = cur_profile->profile_set(cur_profile, i);
>>> - if (err)
>>> - return err;
>>> + if (i == PLATFORM_PROFILE_CUSTOM) {
>>> + pr_warn("Custom profile not supported for legacy sysfs
>>> interface\n");
>>
>> This would allow userspace applications to spam the kernel log. Please
>> just return -EINVAL here
>> and document this special case inside the interface specification.
>>
> OK.
>
>> Thanks,
>> Armin Wolf
>>
>>> + return -EINVAL;
>>> }
>>>
>>> - sysfs_notify(acpi_kobj, NULL, "platform_profile");
>>> - return count;
>>> + ret = class_for_each_device(&platform_profile_class, NULL, &i,
>>> _store_class_profile);
>>> +
>>> + return ret ? ret : count;
>>> }
>>>
>>> static DEVICE_ATTR_RO(platform_profile_choices);
>
>
Powered by blists - more mailing lists