Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8651f747-b932-4cbc-52ca-046f7b1d700e@redhat.com>
Date:   Wed, 26 Apr 2023 15:13:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.10.0
Subject: Re: [PATCH v11 06/14] HP BIOSCFG driver - passwdobj-attributes
Content-Language: en-US, nl
To:     thomas@t-8ch.de, Jorge Lopez <jorgealtxwork@gmail.com>
Cc:     platform-driver-x86@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230420165454.9517-1-jorge.lopez2@hp.com>
 <20230420165454.9517-7-jorge.lopez2@hp.com>
 <016a9a6a-cff1-444d-b96a-63eded1ac58a@t-8ch.de>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <016a9a6a-cff1-444d-b96a-63eded1ac58a@t-8ch.de>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi,

On 4/23/23 11:07, thomas@t-8ch.de 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