linux-kernel - Re: [PATCH] platform/x86: think-lmi: Add bulk save feature

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-Id: <8ca1d0f1-4064-4804-aa0a-d8633e0e6361@app.fastmail.com>
Date:   Tue, 05 Sep 2023 07:24:33 -0400
From:   "Mark Pearson" <mpearson-lenovo@...ebb.ca>
To:     "Hans de Goede" <hdegoede@...hat.com>
Cc:     "markgross@...nel.org" <markgross@...nel.org>,
        "platform-driver-x86@...r.kernel.org" 
        <platform-driver-x86@...r.kernel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] platform/x86: think-lmi: Add bulk save feature

Thanks Hans,

On Tue, Sep 5, 2023, at 7:17 AM, Hans de Goede wrote:
> Hi Mark,
>
> On 8/29/23 15:15, Mark Pearson wrote:
<snip>
>> +
>> +	/* Check if user is trying to change the save mode */
>> +	if (!strncmp(buf, "bulk", 4) || !strncmp(buf, "single", 6)) {
>> +		tlmi_priv.save_mode = strncmp(buf, "bulk", 4) ? TLMI_SAVE_SINGLE : TLMI_SAVE_BULK;
>> +		return count;
>> +	}
>> +	if (strncmp(buf, "save", 4))
>> +		return -EINVAL;
>
> Things look good up to this point, but I'm not happy
> with the string parsing here. Using strncmp to avoid
> a possible '\n' means that writing
> "bulk extra special with onions" will also match "bulk".
>
> Instead I suggest the following (better names
> for the enum are welcome):
>
> 	enum { save_single, save_bulk, save_save };
>
> 	const char * const save_strings[] = {
> 		[save_single] = "single",
> 		[save_bulk] = "bulk",
> 		[save_save] = "save",
> 	};
>
> 	int ret = 0;
> 	int cmd;
>
> 	cmd = sysfs_match_string(save_strings, buf);
> 	if (cmd < 0)
> 		return cmd;
>
> 	mutex_lock(&tlmi_mutex);
>
> 	switch (cmd) {
> 	case save_single:
> 		tlmi_priv.save_mode = TLMI_SAVE_SINGLE;
> 		goto out;
> 	case save_bulk:
> 		tlmi_priv.save_mode = TLMI_SAVE_BULK;
> 		goto out;
> 	case save_save:
> 		break; /* Continue with saving settings */
> 	}
>
> 	/* The user is triggering a save - if supported */
> 	if (!tlmi_priv.can_set_bios_settings ||
> 	    tlmi_priv.save_mode == TLMI_SAVE_SINGLE)
> 		return -EOPNOTSUPP;
>
> 	...
>
> This lets sysfs_match_string() do the string parsing work
> for us, getting rid of having to do this ourselves.
>
Agreed - this is much better. Thanks for the suggestion and I'll make the change.

> Notice I have also moved the mutex_lock() up, so that
> it is also done for updating the save_mode since we
> don't want that the change halfway through a possibly
> racing current_value_store() call.
>
Ack

Mark