Re: [PATCH v11 06/14] HP BIOSCFG driver - passwdobj-attributes

[Hans de Goede]

Hi,

On 4/23/23 11:07, thomas@xxxxxxxx wrote:
> On 2023-04-20 11:54:46-0500, Jorge Lopez wrote:
>> ---
>>  .../x86/hp/hp-bioscfg/passwdobj-attributes.c  | 669 ++++++++++++++++++
>>  1 file changed, 669 insertions(+)
>>  create mode 100644 drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
>>
>> diff --git a/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c
>> new file mode 100644
>> index 000000000000..c03b3a71e9c4
>> --- /dev/null
>> +++ b/drivers/platform/x86/hp/hp-bioscfg/passwdobj-attributes.c

<snip>

>> +static ssize_t current_password_store(struct kobject *kobj,
>> +				      struct kobj_attribute *attr,
>> +				      const char *buf, size_t count)
>> +{
>> +	char *p, *buf_cp;
>> +	int id, ret = 0;
>> +
>> +	buf_cp = kstrdup(buf, GFP_KERNEL);
>> +	if (!buf_cp) {
>> +		ret = -ENOMEM;
>> +		goto exit_password;
>> +	}
>> +
>> +	p = memchr(buf_cp, '\n', count);
>> +
>> +	if (p != NULL)
>> +		*p = '\0';
> 
> This will also accept input like "foo\nbar" and truncate away the "bar".

That is true, but stripping '\n' at the end is a pretty standard
pattern for sysfs attr store functions since users will e.g.
often do:

echo one-string-out-of-a-few-valid-strings > /sys/.../some-enum-attr

Where to actually write the real valid string the user should do:

echo -n one-string-out-of-a-few-valid-strings > /sys/.../some-enum-attr

See e.g.:

drivers/platform/x86/dell/dell-wmi-sysman/passobj-attributes.c new_password_store()

which does the exact same thing.

The stripping of '\n' is often taken care of by various kernel
helpers for sysfs attr.

> For something like a password it seems errorprone to try to munge the
> value.

Almost all password input dialogs including the actual BIOS password
input dialog will consider the enter key / a new-line to mean
"end-of-password, please validate the password inputted so far"

So I don't think this is really a problem. With that said we
could make this more robust (and maybe also change the Dell
code to match) by doing:

	len = strlen(buf_cp);
	if (len && buf_cp[len - 1] == '\n')
		buf_cp[len - 1] = 0;

To ensure that we only ever strip a leading  '\n'
and not one in the middle of the buffer.

Regards,

Hans